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.

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

Merged
merged 1 commit into from
Jul 17, 2012

Conversation

adamgotterer
Copy link
Contributor

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.

@dblock
Copy link
Member

dblock commented Jul 14, 2012

This needs a test, please.

@adamgotterer
Copy link
Contributor Author

Will add a test this weekend. Thanks.

@adamgotterer
Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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.

@adamgotterer
Copy link
Contributor Author

Updated. Also squashed the commits to keep the history clean.

@dblock
Copy link
Member

dblock commented Jul 16, 2012

You now check condtions_met twice. Once outside the if and the other inside the if. Pick one :)

@adamgotterer
Copy link
Contributor Author

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?

@dblock
Copy link
Member

dblock commented Jul 16, 2012

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?

@adamgotterer
Copy link
Contributor Author

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?

@dblock
Copy link
Member

dblock commented Jul 16, 2012

I am going to merge this. I forgot to ask you to add a line into CHANGELOG, would you please? Thanks.

@adamgotterer
Copy link
Contributor Author

Should this go under a new section header? 0.2.2?

@dblock
Copy link
Member

dblock commented Jul 16, 2012

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
dblock added a commit that referenced this pull request Jul 17, 2012
Add a check to Entity#serializable_hash to verify an entity exists on a model
@dblock dblock merged commit 1ef1a1f into ruby-grape:master Jul 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants