Fix Settable: allow setting a nested value to a non-string (for 6.4 stable) #4510

mdehoog · 2018-07-17T03:00:22Z

#4487 introduced a regression: one can no longer $set a nested hash value to anything that doesn't implement empty?.

This PR fixes a couple of bugs with the previous PR:

Pull out the value from field_and_value_hash before checking if it's empty?; this ensures we're checking the actual value that is attempting to be set
Check that it is actually a Hash before checking if it's empty?

The reason previous tests passed is because they only checked that nested values could be set to a string, which implements empty?.

Relevant stacktrace here:

      undefined method `empty?' for 1531794645:Fixnum
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable/settable.rb:29:in `block (2 levels) in set'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable.rb:166:in `block (2 levels) in process_atomic_operations'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/attributes/readonly.rb:36:in `as_writable_attribute!'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable.rb:165:in `block in process_atomic_operations'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable.rb:164:in `each'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable.rb:164:in `process_atomic_operations'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable/settable.rb:24:in `block in set'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable.rb:142:in `prepare_atomic_operation'
./tmp/bundle/ruby-2.3.7/ruby/2.3.0/gems/mongoid-6.4.1/lib/mongoid/persistable/settable.rb:23:in `set'

p-mongo · 2018-07-18T17:44:17Z

lib/mongoid/persistable/settable.rb


-            if fields[field] && fields[field].type == Hash && attributes.key?(field) && !value.empty?
+            if fields[field] && fields[field].type == Hash && attributes.key?(field) && Hash === value && !value.empty?


Do you think respond_to?(:empty?) would make more sense here?

No, I think we specifically want to check for a Hash... for example we don't want to enter this block for a String, even though String implements empty?.

OK that makes sense. Is this behavior documented somewhere?

Not sure. The behavior in this PR seems to make sense: only merge an existing Hash with a new value if that value is a non-empty Hash. The PR that introduced the regression intended to support setting field values to an empty Hash, which remains supported.

gracezhangyaofei · 2018-07-25T02:01:40Z

Hi may I know when is this PR going to be merged? This issue is quite serious though. Many thanks!

p-mongo · 2018-07-25T02:15:36Z

@gracezhangyaofei I need to dig into the code that this PR changes as the change itself is non-obvious. Additionally neither https://docs.mongodb.com/mongoid/master/tutorials/mongoid-persistence/ nor https://docs.mongodb.com/mongoid/master/tutorials/mongoid-nested-attributes/ seem to mention anything that looks like church.set('location.address.city' => 12345), I need to figure out where that is documented.

gracezhangyaofei · 2018-07-25T03:25:13Z

Hi @p-mongo thanks for your fast reply. Didn't dig into these tutorials. But it is a real problem for me right now. And it was caused by !value.empty? not too long ago. Now I was forced to do an unfriendly workaround, hopefully this can be fixed asap. Thanks and looking forward!

mdehoog · 2018-07-25T15:40:51Z

Documentation about how $set should behave in embedded documents is here: https://docs.mongodb.com/manual/reference/operator/update/set/#set-fields-in-embedded-documents. Currently 6.4.0/6.4.1 will throw an error if 'zzz' is changed to anything that doesn't implement empty?. The current spec missed this because Ruby String implements empty?.

The new spec should indicate what this PR is attempting to fix. AFAIK there's no existing behavior change here.

saghm · 2018-07-26T21:02:15Z

spec/mongoid/persistable/settable_spec.rb

+        end
+
+        before do
+          church.set('location.address.city' => 12345)


If I'm understanding correctly, this is where the exception would have occurred before the change? If so, let's put this inside a let block instead and invoke inside the it block below so that any future regression occurs in a place that's more easily identified.

Can do, but this way it's more consistent with all the examples around it. I think an exception raised anywhere in the spec is fine for failing a test; it doesn't necessarily need to be raised in the it block. However, happy to change it if this blocks merging.

Fair enough; I don't feel strongly enough to block merging on this, so it's fine to leave as-is.

saghm · 2018-07-26T21:14:33Z

@p-mongo From what I can tell, this change seems to be correct, and I think we should update our documentation to describe this behavior. That being said, I think the documentation you linked to describes a separate feature from this, namely updating both sides of a relation at the same time (which unfortunately is not at all what the name seems to imply). I'm thinking that the best place for the new documentation would be in the "Fields" section. Thoughts?

p-mongo · 2018-07-26T21:17:43Z

I think the docs should be under persistence/#set, I'm going through the relevant bits in the tutorial to expand them.

p-mongo · 2018-07-26T21:18:29Z

https://jira.mongodb.org/browse/MONGOID-4579 for that, if you want to merge this PR as it is that's ok with me.

p-mongo · 2018-07-30T20:43:11Z

@mdehoog Can you provide a reduced failing example that this PR is fixing?

p-mongo · 2018-07-30T20:46:05Z

Or @gracezhangyaofei if you have an example because so far my examples are working.

p-mongo · 2018-07-30T21:04:57Z

Docs are in #4514.

mdehoog · 2018-07-30T21:23:51Z

@p-mongo The specs I added fail without the change:

Failures:

  1) Mongoid::Persistable::Settable when the field is already set locally when the field is a nested hash when a leaf value in the nested hash is updated to a number updates the nested value to the correct value
     Failure/Error: if fields[field] && fields[field].type == Hash && attributes.key?(field) && !value.empty?

     NoMethodError:
       undefined method `empty?' for 12345:Fixnum
     # ./lib/mongoid/persistable/settable.rb:29:in `block (2 levels) in set'
     # ./lib/mongoid/persistable.rb:166:in `block (2 levels) in process_atomic_operations'
     # ./lib/mongoid/attributes/readonly.rb:36:in `as_writable_attribute!'
     # ./lib/mongoid/persistable.rb:165:in `block in process_atomic_operations'
     # ./lib/mongoid/persistable.rb:164:in `each'
     # ./lib/mongoid/persistable.rb:164:in `process_atomic_operations'
     # ./lib/mongoid/persistable/settable.rb:24:in `block in set'
     # ./lib/mongoid/persistable.rb:142:in `prepare_atomic_operation'
     # ./lib/mongoid/persistable/settable.rb:23:in `set'
     # ./spec/mongoid/persistable/settable_spec.rb:329:in `block (5 levels) in <top (required)>'

Finished in 1.55 seconds (files took 0.88375 seconds to load)
56 examples, 1 failure

Failed examples:

rspec ./spec/mongoid/persistable/settable_spec.rb:332 # Mongoid::Persistable::Settable when the field is already set locally when the field is a nested hash when a leaf value in the nested hash is updated to a number updates the nested value to the correct value

p-mongo · 2018-07-30T21:46:25Z

I see, the bug affects 6.x only. This problem does not exist on master. Have you investigated what makes master work in this case?

mdehoog · 2018-07-30T21:47:32Z

#4487 wasn't merged into master, so master might actually suffer from https://jira.mongodb.org/browse/MONGOID-4525. I haven't validated this.

p-mongo · 2018-07-30T21:59:02Z

OK, then we need to figure out if 4525 affects master, ensure master has an equivalent test to the one added in this PR, then see if the fix can be backported from master or potentially master needs some version of #4487 and/or this PR.

mdehoog · 2018-08-02T18:45:44Z

Yes the original issue affects master and has not been fixed; verified by running the new specs from #4487 on master. #4515 should fix master and includes this change too.

p-mongo · 2018-08-10T18:46:03Z

After #4515 lands it needs to be cherry picked back into this PR.

saghm · 2018-08-14T20:22:26Z

Closed in favor of #4518

mdehoog · 2018-08-14T20:28:21Z

@saghm is it worthwhile getting this fix in to the 6.4.0 branch anyway? It is currently broken on this branch, and #4518 is a different fix that introduces a breaking change, so not sure if it's wise to backport.

saghm · 2018-08-14T21:10:56Z

We don't currently have any bugfix release scheduled for the 6.4.x branch, although we don't have any opposition to doing one. Is this something you need, or would using the 7.0.x branch work for you?

mdehoog · 2018-08-14T23:08:34Z

Sounds good 👍. We're not in a position to move to 7.0.x yet, but we'll put it on the roadmap.

saghm · 2018-08-16T18:46:12Z

After some discussion, we've decided we do in fact want to do a patch release for 6.4, and this seems like a good candidate to be part of that. Unfortunately, it looks like I can't reopen this since the branch is closed. Any chance it's possible to get it pushed back up? (If not, that's totally fine! I can just manually apply the patch, which Github does seem to have cached).

mdehoog · 2018-08-17T02:41:41Z

@saghm I restored the branch, but can't reopen the PR; do you have access to do this?

p-mongo · 2018-08-20T19:29:22Z

Let's cherry-pick this diff on 7.0.0-stable as well.

gracezhangyaofei · 2018-09-03T09:33:51Z

Hi @p-mongo , may I know when this fix can be merged? I saw your last msg was two weeks ago. Keep me posted, many thanks!

p-mongo · 2018-09-05T19:40:38Z

@gracezhangyaofei We are configuring CI internally to get this PR appropriately tested before merging it.

saghm · 2018-09-11T15:47:13Z

Hi @mdehoog! Sorry for the delay in merging this; we had to get our CI set up for the 6.4.0-stable branch, but it's all ready go to now! Is it all right with you if I push a rebase up to your branch with a few backports we made to the 6.4.0 branch to get the CI passing? (Github suggests that I can "add more commits by pushing" to your branch, but I'm not sure about a force push after a rebase, and I don't want to force push to your repo without getting permission first anyhow)

mdehoog · 2018-09-11T23:56:39Z

@saghm completely fine, go for it 👍

saghm · 2018-09-12T19:22:38Z

Awesome, thanks!

saghm · 2018-09-13T16:18:06Z

Just merged this. Thanks again for the contribution!

p-mongo reviewed Jul 18, 2018

View reviewed changes

saghm reviewed Jul 26, 2018

View reviewed changes

saghm approved these changes Jul 30, 2018

View reviewed changes

mdehoog added a commit to mdehoog/mongoid that referenced this pull request Aug 2, 2018

Fixed nested setter (mongodb#4510 with spec from mongodb#4487)

83ea59e

mdehoog mentioned this pull request Aug 2, 2018

MONGOID-4525 Fix ability to reset Hash field to empty Hash (on master) #4515

Closed

mdehoog mentioned this pull request Aug 11, 2018

MONGOID-4593 Modify Settable#set hash merge behavior to match $set #4518

Merged

saghm closed this Aug 14, 2018

mdehoog deleted the fix-nested-non-string-setter branch August 14, 2018 23:08

mdehoog restored the fix-nested-non-string-setter branch August 17, 2018 02:38

p-mongo reopened this Aug 17, 2018

p-mongo approved these changes Aug 17, 2018

View reviewed changes

p-mongo changed the title ~~Fix Settable: allow setting a nested value to a non-string~~ Fix Settable: allow setting a nested value to a non-string (for 6.4 stable) Aug 20, 2018

mdehoog added 2 commits September 12, 2018 15:23

Fix Settable: allow setting a nested value to a non-string

903bfbe

Slight refactor to rspec example string

78607ba

saghm force-pushed the fix-nested-non-string-setter branch from 051ff8b to 78607ba Compare September 12, 2018 19:23

saghm merged commit 48673d7 into mongodb:6.4.0-stable Sep 13, 2018


		if fields[field] && fields[field].type == Hash && attributes.key?(field) && !value.empty?
		if fields[field] && fields[field].type == Hash && attributes.key?(field) && Hash === value && !value.empty?

Fix Settable: allow setting a nested value to a non-string (for 6.4 stable) #4510

Fix Settable: allow setting a nested value to a non-string (for 6.4 stable) #4510

Conversation

mdehoog commented Jul 17, 2018 • edited

p-mongo Jul 18, 2018

Choose a reason for hiding this comment

mdehoog Jul 18, 2018

Choose a reason for hiding this comment

p-mongo Jul 18, 2018

Choose a reason for hiding this comment

mdehoog Jul 18, 2018

Choose a reason for hiding this comment

gracezhangyaofei commented Jul 25, 2018

p-mongo commented Jul 25, 2018

gracezhangyaofei commented Jul 25, 2018 • edited

mdehoog commented Jul 25, 2018 • edited

saghm Jul 26, 2018

Choose a reason for hiding this comment

mdehoog Jul 28, 2018 • edited

Choose a reason for hiding this comment

saghm Jul 30, 2018

Choose a reason for hiding this comment

saghm commented Jul 26, 2018

p-mongo commented Jul 26, 2018

p-mongo commented Jul 26, 2018

p-mongo commented Jul 30, 2018

p-mongo commented Jul 30, 2018

p-mongo commented Jul 30, 2018

mdehoog commented Jul 30, 2018

p-mongo commented Jul 30, 2018

mdehoog commented Jul 30, 2018 • edited

p-mongo commented Jul 30, 2018

mdehoog commented Aug 2, 2018

p-mongo commented Aug 10, 2018

saghm commented Aug 14, 2018

mdehoog commented Aug 14, 2018

saghm commented Aug 14, 2018

mdehoog commented Aug 14, 2018

saghm commented Aug 16, 2018

mdehoog commented Aug 17, 2018

p-mongo commented Aug 20, 2018

gracezhangyaofei commented Sep 3, 2018

p-mongo commented Sep 5, 2018

saghm commented Sep 11, 2018

mdehoog commented Sep 11, 2018

saghm commented Sep 12, 2018

saghm commented Sep 13, 2018

mdehoog commented Jul 17, 2018 •

edited

gracezhangyaofei commented Jul 25, 2018 •

edited

mdehoog commented Jul 25, 2018 •

edited

mdehoog Jul 28, 2018 •

edited

mdehoog commented Jul 30, 2018 •

edited