The use of :data in a non-configurable API response #117

Open
ericbrooke opened this Issue Jan 23, 2014 · 10 comments

Comments

Projects
None yet
6 participants

activeresource does not seem to be able to accept incoming data with the :data set and used to store information.

We are building a rails app that receives data from another app and the api is giving is

activeresource TypeError: allocator undefined for Data.

We appear to be getting the same problem as the following posting

http://stackoverflow.com/questions/7306004/having-allocator-undefined-for-data-when-saving-with-activeresource

All the current "hacks" seem to patch active resource. Can we not get this active resource :data namespaced?

wkral commented Jan 23, 2014

The offending code comes in the find_or_create_resource_for function:

  def find_or_create_resource_for(name)
    return reflections[name.to_sym].klass if reflections.key?(name.to_sym)
    resource_name = name.to_s.camelize

    const_args = [resource_name, false]
    if self.class.const_defined?(*const_args)
      self.class.const_get(*const_args)
    else
      ancestors = self.class.name.to_s.split("::")
      if ancestors.size > 1
        find_or_create_resource_in_modules(resource_name, ancestors)
      else
        if Object.const_defined?(*const_args)
          Object.const_get(*const_args)
        else
          create_resource_for(resource_name)
        end
      end
    end
  end

The code breaks in if Object.const_defined?(*const_args) which seems to have all the imported classes
Object.contants shows all these symbols. I'm not sure what that code is for exactly but it creates a moving target of possible namespace collisions especially when you require all sorts of gems.

rails-bot bot added the stale label May 5, 2017

rails-bot bot commented May 5, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.

If it is an issue and you can still reproduce this error on the master branch,
please reply with all of the information you have about it in order to keep the issue open.

If it is a pull request and you are still interested on having it merged please make sure it can be merged clearly.

Thank you for all your contributions.

Owner

rafaelfranca commented May 5, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.

The resources of the Rails team are limited, and so we are asking for your help.

If it is an issue and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.

If it is a pull request and you are still interested on having it merged please make sure it can be merged clearly.

Thank you for all your contributions.

mvz commented May 8, 2017

I think I just encountered this one: one of my resources has nested tags, and tries to instantiate them using the Tag class, which is an ActiveRecord class that happens to have the same name. The resulting error is rather cryptic:

  wrong number of arguments (2 for 0..1) (ArgumentError)
  ./vendor/cache/activeresource-e28f907145c3/lib/active_resource/base.rb:1407:in `block (2 levels) in load'
  [...]

I can see how this behavior can be useful, but perhaps an additional check should be done to see if the found class is an ActiverResource class?

rails-bot bot removed the stale label May 8, 2017

Owner

rafaelfranca commented May 10, 2017

I can see how this behavior can be useful, but perhaps an additional check should be done to see if the found class is an ActiverResource class?

Thank make sense. Could you open a PR?

mvz commented May 18, 2017

Could you open a PR?

Done: #250.

mvz commented May 20, 2017

As @jeremy mentioned in #250, allowing a PORO to be instantiated as a nested object is intentional, because not all nested objects have their own REST API. Maybe we can find some other way to limit the set of classes ARes considers?

Owner

jeremy commented May 21, 2017

Possibilities:

  • define your resource classes within a namespace module, so you always try to resolve other resource classes against that module instead of Object (already works)
  • for toplevel resource classes, allow doing constant lookup on some user-provided namespace module instead (introduces awkward, confusing user-facing config)
  • allow users to turn off or override missing-resource resolution (easier to explain, but still weird)

Think the clearest guidance is to keep resource classes in a namespace module.

mvz commented May 21, 2017

Yes, keeping resource classes in their own namespace is definitely the best option for current users of ARes. However, I think the default behavior should be safer than it currently is. Some options:

  • Do not resolve in the global namespace by default, but allow this to be turned on again for legacy applications.
  • Require users to be more explicit about relationships, similar to how AR works.

Raising a different and clearer error would also help, e.g.: Unable to instantiate nested resource 'foo' using class 'Foo'.

mvz commented May 27, 2017

Any thoughts on this? Sorry for being so persistent, but I think this is a potential security hazard so I'd like to get it fixed.

Also, I've just updated some projects to use a namespace, and it turns out it's quite a lot of work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment