-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Add a check to Entity#serializable_hash to verify an entity exists on a model #203
Conversation
This needs a test, please. |
Will add a test this weekend. Thanks. |
Updated pull request to include tests and fix an edge case that caused an existing case to fail. |
expect{ fresh_class.new(model).serializable_hash }.not_to raise_error | ||
end | ||
|
||
it 'should not expose attributes that don\'t exist on the object' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but you can mix quotes in Ruby. This avoids those backslashes.
it "should not expose attributes that don't exist on the object" do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had it as single quotes for consistency, updated to double.
Updated. Also squashed the commits to keep the history clean. |
You now check |
Updated. Theres a small behavior change, in the original version if the #conditions_met? was false the entity value would be nil. Now the entity won't even exist. Is that behavior okay? |
I am happy to merge this. Question though (and sorry to be a pest) - do we actually want this behavior as opposed to a (better) error? What's a legit scenario where you want to expose something that doesn't exist? |
The whole reason I got into this was because of dynamic schemas in Mongo. If one document has fields: A, B and C and a another doc has B, C and D. You can't expose A and D since it doesn't exist across all the documents. Currently a NoMethodError will be thrown if you do try to expose them. This patch might only be useful to people working with dynamic schemas. People working with fixed schemas might like the error. Maybe the solution is a flag that sets the entity as a dynamic schema which would take on this new behavior? |
I am going to merge this. I forgot to ask you to add a line into CHANGELOG, would you please? Thanks. |
Should this go under a new section header? 0.2.2? |
yes, please |
an object before accessing it. Improved Entity#serializable_hash check for an entity on an object before accessing the value. Converted proc and using condition to #conditions_met
Add a check to Entity#serializable_hash to verify an entity exists on a model
Ran into an issue where I was exposing keys on a Mongo object that has a dynamic schema. When processing the entity it would try and access a field that didn't exist and threw and exception. This will verify that the entity can be accessed before trying.