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

do not use any Entity constant that is available in namespace, as it can... #600

Merged
merged 1 commit into from
Mar 20, 2014

Conversation

fuksito
Copy link
Contributor

@fuksito fuksito commented Mar 19, 2014

I experienced problem with grape when I had an active record model in my project with name Entity. This model is not connected with grape at all, but grape looks for any constant with name Entity, and tries to use it to present values that does not have explicit presenter, like simple strings. So it is usual case to see errors like:

undefined method `represent' for #<Class:0x00000102bd1f70>

by this PL I suggest to make at least check that an object has such method before calling it. Ideally I suppose would be ability to config the name of an auto-detect entity class or be able to disable it

@@ -332,7 +332,9 @@ def present(*args)
entity_class ||= (settings[:representations] || {})[potential]
end

entity_class ||= object_instance.class.const_get(:Entity) if object_instance.class.const_defined?(:Entity)
if object_instance.class.const_defined?(:Entity)
entity_class ||= object_instance.class.const_get(:Entity) if object_instance.class.const_get(:Entity).respond_to?(:represent)
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to move the right-hand-side of the if up as well.

if object_instance.class.const_defined?(:Entity) && object_instance.class.const_get(:Entity).respond_to?(:represent)
...

Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes wanted, but it appeared to become very long line

@dblock
Copy link
Member

dblock commented Mar 19, 2014

Please update the CHANGELOG and check the Rubocop violation in https://travis-ci.org/intridea/grape/jobs/21130443.

@fuksito
Copy link
Contributor Author

fuksito commented Mar 20, 2014

Updated

dblock added a commit that referenced this pull request Mar 20, 2014
do not use any Entity constant that is available in namespace, as it can...
@dblock dblock merged commit b74c841 into ruby-grape:master Mar 20, 2014
@dblock
Copy link
Member

dblock commented Mar 20, 2014

Merged.

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