-
Notifications
You must be signed in to change notification settings - Fork 51
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
rabl-rails assumes any object that respond to each
is a collection
#55
Comments
FIxed in 781ec8d. |
The referenced commit doesn't actually fix the issue, unfortunately. The fix is specific to Structs but the issue is not. As I wrote above, I discovered it when trying to render an attribute of an ActiveSupport::OrderedOptions. The issue still manifests with any object that responds to |
What about this b2eb5be ? You can now define your own objects that respond_to |
Thanks for taking the time to reopen this, @ccocchi. I think this solution is better, with one reservation. I wonder if specifying class names instead of actual classes (i.e. RablRails.configure do |config|
config.non_collection_classes << MyModel
end ...Rails will autoload MyModel as soon as that block is called. If instead you could do this: RablRails.configure do |config|
config.non_collection_classes << "MyModel"
end ...and then in resource && resource.respond_to?(:each) &&
RablRails.configuration.non_collection_classes.none? { |k| klass <= k.constantize } ...then MyModel will only be loaded when absolutely necessary. While a typical Rails app will load all of its models eventually, often Rake tasks or workers only need one or two of the app's models, and I think those tend to benefit from optimizations like this. |
Using strings results in a big performance drawback, see this https://gist.github.com/ccocchi/b74045f5e3c15db44dce
I guess having one or two models autoloaded at boot time is a lesser matter. This method is at least called once per object rendered so kind of a hot path. Workers and Rails would eventually load all models so we should be good. Maybe some I will test this in a live application to see performance results and I'll release a new version soon. |
Got problem with non_collection_classes in development environment. |
Ok ok, let's go with strings 😞 |
If performance is an issue for some people, we could have our cake and eat it too by using the string technique in development and the class technique in production. Or you could hook into Rails' reloader to make sure rabl-rails' configuration is updated when classes are reloaded. But that still leaves the autoloading issue. I suppose the third way would be to allow either classes or strings, which would let users choose between the autoloading penalty and the |
From |
I found good results with changing the resource && resource.respond_to?(:each) && resource.kind_of?(Enumerable) &&
klass.ancestors.none? { |a| RablRails.configuration.non_collection_classes.include? a.name } What are your thoughts, @ccocchi? |
it is not compatible with sequel-rails. the code |
Hello, I don't understand what your problem is. If a sequel model (I'm assuming it is an equivalent of Where am I mistaken? |
I'd like to use rabl-rails to render an object, but since the object responds to
each
, RablRails::Renderers::Base tries to render it as a collection. Here's code that reproduces the problem:I would expect this to render
{"foo":"bar"}
since@config.foo
returns"bar"
. However, because@config
responds toeach
,Renderers::Base#render
callsrender_collection
on it instead ofrender_resource
, yielding an error:(You can see the complete backtrace here: https://gist.github.com/jrunning/9279963)
This might seem like a contrived example, but I ran into it in a real scenario (I was trying to render attributes of an ActiveSupport::OrderedOptions rather than a struct). The same issue would manifest if someone defined
each
on an ActiveRecord model.I'm not sure what the best solution is. Perhaps
object
could take an option that would explicitly tell it not to render the object as a collection?For now my only option seems to be falling back to the regular rabl gem, which lacks some of the Railsy niceties of rabl-rails.
The text was updated successfully, but these errors were encountered: