Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MONGOID-5208 fix error on reloading nil embedded document #5116

Merged
merged 4 commits into from
Dec 30, 2021
Merged

MONGOID-5208 fix error on reloading nil embedded document #5116

merged 4 commits into from
Dec 30, 2021

Conversation

Neilshweky
Copy link
Contributor

No description provided.

p-mongo
p-mongo reviewed Dec 30, 2021
View reviewed changes
lib/mongoid/reloadable.rb Outdated Show resolved Hide resolved
lib/mongoid/reloadable.rb Outdated
@@ -22,7 +22,7 @@ def reload
end

reloaded = _reload
if Mongoid.raise_not_found_error && (!reloaded || reloaded.empty?)
if Mongoid.raise_not_found_error && reloaded.empty?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring on _reload should also then be updated to say that if the embedded document no longer exists in the dabatase, _reload would return nil.

Subsequently, here I think the correct fix is to check reloaded.nil? either instead of or in addition to reloaded.empty?.

I am also curious what it means for reloaded to be empty here - the document exists but has no attributes? How does a user attain such a beast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not exactly sure how this case is achieved, but in general I balk at removing safety checks.

Copy link
Contributor Author

@Neilshweky Neilshweky Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I found an instance where reloaded.empty? is true.

reloadable_spec.rb:112
context "when there is no document matching our id" do
  it "raises an error" do
    expect {
      Person.new.reload
    }.to raise_error(Mongoid::Errors::DocumentNotFound)
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this situation I am not a fan of how that is implemented either, let's leave it alone for now since it's not in scope of your work.

@p-mongo
Copy link
Contributor

p-mongo commented Dec 30, 2021

This PR also doesn't have tests or am I missing how it's tested? Is the reproduce code in the ticket already in the test suite elsewhere?

@Neilshweky
Copy link
Contributor Author

This PR also doesn't have tests or am I missing how it's tested? Is the reproduce code in the ticket already in the test suite elsewhere?

Is the test I added in reloadable_spec.rb not showing up for you? You might have an outdated version of the PR because one of the comments you made was on code that no longer exists in the most recent commit.

@Neilshweky Neilshweky requested a review from p-mongo December 30, 2021 18:05
@p-mongo
Copy link
Contributor

p-mongo commented Dec 30, 2021

This PR needs to be rebased on master.

@Neilshweky
Copy link
Contributor Author

Yup! I just did that

p-mongo
p-mongo approved these changes Dec 30, 2021
View reviewed changes
Copy link
Contributor

@p-mongo p-mongo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge after fixing up the description and waiting for CI.

spec/mongoid/reloadable_spec.rb Show resolved Hide resolved
@Neilshweky Neilshweky merged commit 0a7cf5a into mongodb:master Dec 30, 2021
p-mongo pushed a commit to johnnyshields/mongoid that referenced this pull request Jan 3, 2022
@p
Merge branch 'master' into pr-5109
ea91374 
* master:
  Fix doc syntax
  RUBY-2783 Require bson 4-stable for Mongoid as bson master is 5.0 and not compatible with driver (mongodb#5113)
  MONGOID-5208 fix error on reloading nil embedded document (mongodb#5116)
  MONGOID-5206 fix bug where embedded document is not re-embedded (mongodb#5115)
  MONGOID-5207 Add Ruby 3.1 to GH Actions (mongodb#5114)
  MONGOID-5207 Use YAML.safe_load in specs (mongodb#5112)
  MONGOID-5199 Reloadable#reload_embedded_document doesn't respect session (mongodb#5105)
  Fix MONGOID-5198 Calling children_changed? on a deep cyclical data structure will cause semi-infinite looping (mongodb#5104)
p-mongo pushed a commit to johnnyshields/mongoid that referenced this pull request Jan 3, 2022
@p
Merge branch 'master' into pr-5061
1069e27 
* master:
  MONGOID-5173 Specs should use bang (!) methods (without describe/context change) (mongodb#5109)
  Fix doc syntax
  RUBY-2783 Require bson 4-stable for Mongoid as bson master is 5.0 and not compatible with driver (mongodb#5113)
  MONGOID-5208 fix error on reloading nil embedded document (mongodb#5116)
  MONGOID-5206 fix bug where embedded document is not re-embedded (mongodb#5115)
  MONGOID-5207 Add Ruby 3.1 to GH Actions (mongodb#5114)
  MONGOID-5207 Use YAML.safe_load in specs (mongodb#5112)
  MONGOID-5199 Reloadable#reload_embedded_document doesn't respect session (mongodb#5105)
  Fix MONGOID-5198 Calling children_changed? on a deep cyclical data structure will cause semi-infinite looping (mongodb#5104)
p-mongo pushed a commit that referenced this pull request Jan 7, 2022
@Neilshweky @p
MONGOID-5208 fix error on reloading nil embedded document (#5116)
c8dc61e 
* MONGOID-5208 fix error on reloading nil embedded document

* MONGOID-5208 give default return value

* MONGOID-5208 update function to return nil

* MONGOID-5208 change test description
p-mongo pushed a commit to johnnyshields/mongoid that referenced this pull request Jan 21, 2022
@p
Merge branch 'master' into pr-5050
f4f582c 
* master: (48 commits)
  MONGOID-5098 Bug fix: Range.mongoize should mongoize its members (mongodb#5108)
  MONGOID-5212 Support for Decimal128 type (mongodb#5125)
  MONGOID-5213 Document changes to BigDecimal type and addition of global flag (mongodb#5126)
  MONGOID-5209 Document "Custom Field Options" functionality + various docs improvements (mongodb#5120)
  MONGOID-5193 rails 7 support  (mongodb#5122)
  Fix default topology name - should be standalone (mongodb#5130)
  Update MRSS (mongodb#5129)
  MONGOID-5207 Test Ruby 3.1 on Evergreen (mongodb#5123)
  MONGOID-5186 .with_scope should restore previous scope (mongodb#5127)
  MONGOID-5161 - [Documentation Only] Clarify the policy for working with . and $ named fields which were introduced in MongoDB 5.0 (mongodb#5051)
  MONGOID-5173 Specs should use bang (!) methods (without describe/context change) (mongodb#5109)
  Fix doc syntax
  RUBY-2783 Require bson 4-stable for Mongoid as bson master is 5.0 and not compatible with driver (mongodb#5113)
  MONGOID-5208 fix error on reloading nil embedded document (mongodb#5116)
  MONGOID-5206 fix bug where embedded document is not re-embedded (mongodb#5115)
  MONGOID-5207 Add Ruby 3.1 to GH Actions (mongodb#5114)
  MONGOID-5207 Use YAML.safe_load in specs (mongodb#5112)
  MONGOID-5199 Reloadable#reload_embedded_document doesn't respect session (mongodb#5105)
  Fix MONGOID-5198 Calling children_changed? on a deep cyclical data structure will cause semi-infinite looping (mongodb#5104)
  MONGOID-5203 Add all available auth_mech options to Configuration documentation (mongodb#5103)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@p-mongo p-mongo p-mongo approved these changes

@comandeo comandeo Awaiting requested review from comandeo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Neilshweky @p-mongo