MONGOID-5274 / MONGOID-5142 - Rework #touch with embedded documents

[johnnyshields]

Fixes MONGOID-5142
Fixes MONGOID-5274
Correct fix for MONGOID-5136 (the previous one was non-optimal)
Replaces PR #5039 (includes specs from it, which are passing here)

Changes

• Perform all touch operations on the document in a single write operation (MONGOID-5142)
• Remove touches from the atomic set operations, so that they are no longer there when saving the next time.
• Cleanup/refactor touch method to be much more readable. The code is also now symmetrical for parents and child docs, everything is nicely recursive.

[johnnyshields]

@p-mongo if you can help with the specs here would be greatly appreciated.

[p-mongo]

Hi @johnnyshields , can you please update the title and description of this PR to reflect what it contains? MONGOID-5136 is already fixed.

Can you also clarify whether this PR is ready for review? If it isn't it can be marked [wip] or draft.

[johnnyshields]

I've raised #5202 to point out some strange/incorrect behavior re: #touch method. This PR is on hold until the issues are resolved.

[johnnyshields]

@Neilshweky please review this.

[Neilshweky]

Can you add a test for touching with grandchildren? I would like to see how this works with multiple generations of descendants.

[Neilshweky]

If I understand this correctly, your PR makes the following changes:

• touches are now persisted immediately (backwards breaking).
  • which causes touch callbacks to be called earlier.
• touches are no longer included in the atomic_updates calculation on save.
  • can you put a test for this?
• touch operations are done in one operation.
• embedded touches no longer use after_touch callbacks to update associated documents.

Would you say this is a good summary of the effective changes in this PR?

I think the only backwards breaking change here would be the fact that touches are persisted immediately. Personally this doesn't bother me as a change, although release notes would have to be written about it. @p-mongo is that change okay in your opinion?

[Neilshweky]

Please review my comments above.

[johnnyshields]

@Neilshweky thanks for the review. Let me first respond to your summary of changes so we're on same page:

1. touches are now persisted immediately (backwards breaking). which causes touch callbacks to be called earlier.

AFAIK the old code was the same. The old code has _root.collection.find(selector).update_one(...). Please double check this? I think this should be fixed in a separate PR so touch updates happen as part of the actual write on persistence (at least when doing touch: true on embedded relations.)

2. touches are no longer included in the atomic_updates calculation on save. can you put a test for this?

No. touch_atomic_updates was never actually part of atomic_updates. Its code just happened to be in the same file, where it doesn't belong.

3. touch operations are done in one operation.

Yes. This is the primary intention of the PR.

4. embedded touches no longer use after_touch callbacks to update associated documents.

In the new code, calling the #touch method on an embedded now recursively touches its parent(s) within the #touch method itself, so there's no need for to use after_touch callbacks. So this is not actually a change to the end result behavior. In the future, for save/destroy of embedded relations we should actually be doing the touch operation as part of the persistence itself, or in a before callback so that it happens as part of the main save operation.

[p-mongo]

Neither https://jira.mongodb.org/browse/MONGOID-5142 nor https://jira.mongodb.org/browse/MONGOID-5142 specify making touch perform an immediate write, so I would say this shouldn't be done.

If you believe it is a desired behavior, @johnnyshields please feel free to create a ticket for it describing your use case for it. My thinking is that perhaps touch! could be added which would immediately persist the write (and that would have to deal with validations, for example what if there is a validation requiring updated_at to be older than a certain time, touching such a document could make it invalid).

[p-mongo]

https://jira.mongodb.org/browse/MONGOID-5274 was the other ticket.

[johnnyshields]

perform an immediate write, so I would say this shouldn't be done.

@p-mongo as mentioned in comment above to Neil, I believe the existing code also performs an immediate write. See _root.collection.find(selector).update_one(...) in existing code.

I agree that an immediate write is not desirable, however, here I intentionally tried to keep the behavior consistent. I think we should defer the write in a follow-up ticket.

[johnnyshields]

@Neilshweky I've responded to all your comments here. Please kindly resolve the comments or give further response.

[Neilshweky]

perform an immediate write, so I would say this shouldn't be done.

@p-mongo as mentioned in comment above to Neil, I believe the existing code also performs an immediate write. See _root.collection.find(selector).update_one(...) in existing code.

I agree that an immediate write is not desirable, however, here I intentionally tried to keep the behavior consistent. I think we should defer the write in a follow-up ticket.

I can confirm that the current functionality on master is to persist touch operations immediately.

[Neilshweky]

Can you add a test for touching with grandchildren? I would like to see how this works with multiple generations of descendants.

@johnnyshields can you take care of this and then I'll give this another look?

[Neilshweky]

Please make the testing changes mentioned above:

• multi-generational test
• third model with touch not set

[johnnyshields]

@Neilshweky all comments addressed.

[johnnyshields]

@Neilshweky I appreciate if you can take over this PR from here, if there are further changes needed. I have a busy few weeks ahead of me and won't be able to devote further attention for awhile.

[Neilshweky]

@comandeo I am pretty much done with my review, if you wouldn't mind taking a look here.

[Neilshweky]

Good job @johnnyshields, this really cleans up this part of the code.

[comandeo]

Great job! 👏

[Neilshweky]

Open questions/todos:

• Should this go into 8.1/9.0?

Let's do 9.0 since the repairing of touch: false on embedded associations can be a BC break.

• What is default behavior of touch option for embedded associations?

Does this PR change the default touch behavior for embedded associations?

• Since we have decided to put this into 9.0 there needs to be release notes stating that touch: false for embedded associations has been repaired... @johnnyshields can you handle that? You can even open a separate PR if you like...

[johnnyshields]

@Neilshweky agreed it should be 9.0.

I'm happy to help with release notes, BUT:

Are we also going to change the default value to touch: true for embedded? That will have some bearing on the release notes. AFAIK from this PR the main notable things are:

1. #touch on embedded documents now happen in a single DB call; and
2. #touch on embedded documents no longer automatically touch the parent documents by default, due to a bugfix.

However, if touch: true, then (2) in effect won't change by default. (The change is less impactful if we set touch: true)

[Neilshweky]

I would love to change the default of touch to true for embedded associations, and I will write up a proposal for it in a ticket (MONGOID-5016?)... Assume that we will be changing the default to true, so that (2) will not change... Are there any backwards breaking changes in this PR? Anything that we would need to put in the release notes?

[johnnyshields]

There are no backwards breaking changes. The release notes for the touch: true could have a line like:

In addition, there have been some refactor of the #touch method to generate fewer queries, and edge cases where touch: false was not working previously have been corrected.

Important note - touch: true would only be the new default for embedded_in relations (i.e. touch child causes touch on parent). Referenced relations need to stay as touch: false.

There's also some ticket to remove touch: false support on embedded_in, but I think that's a bridge too far for Mongoid 9.0 in terms of preserving behavior. Should revisit that later.

[Neilshweky]

@johnnyshields take a look at the last commit I made here. Turns out in one of the tests that used a meth, the shared_examples wasn't using it. I changed that and fixed the test (there's a problem in MONGOID-5504 that I will have to fix)... I also explicitly specified the touch option on all models so that when we flip the default, the tests won't fail... I'll add new tests when I do MONGOID-5016

[johnnyshields]

Ah yep, great catch! Last commit is good.

[Neilshweky]

In the interest of keeping my head on straight, here are the TODOs and followup work:

• write 9.0 release notes @johnnyshields is handling this (if necessary)
  • touch: false works for embedded associations
  • made more efficient
• Followup: flip the default of the touch option on embedded associations to true MONGOID-5016
• Followup: verify that the touch: :field functionality works correctly MONGOID-5508

[Neilshweky]

There are no backwards breaking changes. The release notes for the touch: true could have a line like:

I think this might be a good idea but we can do this as part of MONGOID-5016

In addition, there have been some refactor of the #touch method to generate fewer queries, and edge cases where touch: false was not working previously have been corrected.

Important note - touch: true would only be the new default for embedded_in relations (i.e. touch child causes touch on parent). Referenced relations need to stay as touch: false.

This is MONGOID-5016 which I repurposed for switching the default

There's also some ticket to remove touch: false support on embedded_in, but I think that's a bridge too far for Mongoid 9.0 in terms of preserving behavior. Should revisit that later.

[Neilshweky]

@johnnyshields even though there are no backwards breaking changes, the touch: false option has been fixed and I think that warrants a release note, and you can add the note about generating fewer queries with it.

[johnnyshields]

@Neilshweky your spec change adding touch: false seems to have broken a few specs. Suggest to revert it and fix it in the next PR.

[Neilshweky]

I will fix it and merge when its done

[Neilshweky]

@johnnyshields specs should be green now.