MONGOID-5078 Fix shard_key_selector_in_db during post-persist callbacks - DRAFT

[dalton-braze]

Hi @p and @comandeo,

The mongoid#4985 PR allowed me to tweak my existing solution draft for MONGOID-5078 because of shard_key_selector_in_db being split out of shard_key_selector.

shard_key_selector_in_db is defined as:

# Returns the selector that would match the existing version of this
# document in the database.

This supports the conclusion that shard_key_selector_in_db should contain the updated version of a shard key during "post-persist" callbacks (after_create, after_save, after_update, after_upsert, etc). The current incorrect behavior is that shard_key_selector_in_db returns the old value of a shard key during "post-persist" callbacks which are triggered by updating a shard key.

The potential solution I designed adds a single instance variable, @during_post_persist_callbacks, to each Mongoid::Document. This instance variable is read by shard_key_selector_in_db to determine if it should return a sharded field's internally cached previous value or its current value.

This PR is intended as a draft (especially the example tests I wrote). I am hoping to use it as a jumping-off point for discussing the solution to this behavior in more detail. I'd love to hear what you think.

For a much deeper level of detail, check out my original open PR here.

[dalton-braze]

I am converting this PR to "non-draft mode" because it won't assign reviewers otherwise.

[johnnyshields]

@dalton-braze wouldn't it be better to set new_record = false at this stage, since the record is actually persisted? That seems cleaner than introducing a new flag, though it may break some after_save etc. behavior in existing apps. It could be released as part of a major upgrade.

[dalton-braze]

@dalton-braze wouldn't it be better to set new_record = false at this stage, since the record is actually persisted? That seems cleaner than introducing a new flag, though it may break some after_save etc. behavior in existing apps. It could be released as part of a major upgrade.

@johnnyshields, would you be able to clarify your suggestion?

How I'm parsing it, I'm not seeing how changing the point-in-time that new_record is set to false will fix the bug identified by this PR? The goal of the PR is to make shard_key_selector_in_db return the currently persisted value of a Mongoid::Document shard key during post-persist callbacks, after the value of the shard key has been initially-set/modified.

The key section of this PR is the following updated method:

def shard_key_selector_in_db
  selector = {}
  shard_key_fields.each do |field|
    selector[field.to_s] = new_record? || during_post_persist_callbacks ? send(field) : attribute_was(field)
  end
  selector
end

If my explanation feels ambiguous, I'd suggest checking out the repro test cases I've written in spec/mongoid/reloadable_spec.rb, specifically in context "when using sharded documents"

---

As for how new_record? should actually behave, that's a better question for someone a lot more familiar with the Mongoid codebase than I am :)

[johnnyshields]

@dalton-braze sorry my comment was mistaken, please ignore.

[johnnyshields]

@comandeo this might be a good one for you to look at given your expertise with after_* callbacks

[comandeo]

@johnnyshields @dalton-braze The issue seems to be fixed here - 9fa2cc3

I think we can close this PR.

[johnnyshields]

@comandeo can we merge in just the tests from this PR? (or tests that demonstrate the fix, in other words)

[Neilshweky]

@comandeo mind taking a look here again and deciding if we could close this one?

[johnnyshields]

@Neilshweky the problem here is certainly a legitimate one for users who use sharding. Ticket should be kept open.

[johnnyshields]

@jamis @comandeo can we verify the fix here and move forward?