Updated automatic lookup of representers for entities

[ejlangev]

Possible solution for https://github.com/apotonick/roar-rails/issues/72

[ejlangev]

Feedback welcome, this was just a first thought

[apotonick]

This looks excellent, I remember I wanted to do something similar but then didn't feel like it that day, so thanks a lot of going through this :wink:

A few things.

1. Can we have some more tests with deeper namespaces (when you opened the original ticket #72 you had some good examples)?
2. I wonder if Rails already got some mechanism for that somewhere? Isn't this kind of behaviour an integral part of Rails (go up namespaces and find things)?
3. Will this break existing apps? Can you think of any namespace-controller-combo where this might change the circle of life? We have to be careful here: I usually don't like this kind of magic, but I completely see the benefits as you have some good points. However, this might change things to what people do not expect.

[ejlangev]

1) Will also add a few more tests like my examples in https://github.com/apotonick/roar-rails/issues/72.

2) I'll look into how rails loads constants (if it overrides the ruby way of doing it), but I'm pretty sure it doesn't have any special behavior and often is inconsistent about how missing constants are looked up.

3) I don't think this will break existing apps (but I'll add some more tests to try to verify that). I think people who ran into this problem would have specified their representers explicitly anyway.

[apotonick]

Can we finalise that and merge it in for 1.0?

[ejlangev]

Yeah sorry, I've been meaning to get back to this. Will finish it this week.

[ejlangev]

@apotonick I've added some more tests with deeper namespaces and I cleaned up a bug those tests revealed on ruby 1.9.3 related to lookups with const_get and class names like Inner::SingerRepresenter.

[apotonick]

Extremely cool! Thanks so much! :beers:

I might extract your code and use it for defaults in Trailblazer, soon.

[ejlangev]

Awesome, just checked out trailblazer and it seems pretty cool. I'll have to follow it :+1:

[apotonick]

subject.for(:json, V2::Singer.new, 'v1/singers').must_equal V2::SingerRepresenter

So if the controller is V1::SingersController, the model is V2::Singer, it prefers the model's namespace over the controller's?

subject.for(:json, Inner::Singer.new, 'v1/singers').must_equal V1::Inner::SingerRepresenter

Here, it gives priority to the controller's namespace V1 and then again adheres to the models namespace. Isn't that a bit too much magic? Not sure, as I don't see how people could use this, but I find it dangerous to introduce too much knowledge. Can you give an example how that helps you?

subject.for(:json, Outer::Singer.new, 'v1/singers').must_equal Outer::SingerRepresenter

So, I guess it tries to find V1::Outer, but there's only ::Outer, right?

I generally love the idea that it sticks to the model's namespace as long as possible, allowing true polymorphism. We need to check, though, how the controller's namespace gets prepended (or not). If you could summarise the rules in a few bullet points, we're good to go.

Thanks so much, I really like this!!! :fireworks:

[apotonick]

Wait, I think the 2nd case is exactly what I like!

subject.for(:json, Inner::Singer.new, 'v1/singers').must_equal V1::Inner::SingerRepresenter

So, your rules seem to be:

1. "controller namespace + model namespace + model name"
2. "model namespace + model name"
3. "model name"
4. "controller name" (?)

Might re-arrange the tests a bit...

[apotonick]

And in the process of merging this, we should also take a look at how to deal with multiple media types.

represents :xml, :json

1. I believe calling ::represents in a controller should become obligatory. Internally, this calls ::respond_to anyway, but this would also help us finding out which media formats Roar supports in this particular controller.

2. Currently, if you have multiple formats, you can either specify which representer to use per format:

   represents :xml, Song::Representer::XML

   Or roar-rails computes a representer name. This totally does not include the format's name, e.g. I'd love to automatically find SongRepresenter::XML, then SongRepresenter.

What I'm trying to say is: making represents a must means we can use this for finding representer names according to the current format (for both rendering and parsing).

[apotonick]

We should probably provide a hook where users can insert format-specific names into the representer name segments - this way we can try and find best practices before hardcoding anything into roar-rails.

def process_representer_name(model, segments, format)
  segments << format # will result in SongRepresenter::XML
end

[l8nite]

Sweet, this looks like you guys have the right pattern figured out!

[ekampp]

I'm also seeing related problem in #consume!.

This is my code (simplified for example). As you can see, it's wrapped inside a V1 module namespace. The same goes for all the presenters and models.

module V1
  class Controller < ApplicationController
    def update
      singular = User.find(params[:id]).extend(UserRepresenter)
      consume! singular
    end
  end
end

This is my request:

Content-Type: application/hal+json
PATCH /users/1
{
  "name": "Bob the builder"
}

This is the exception raised on the line where I call consume!

NameError: uninitialized constant V1::V1

As you can see, it seems like it'd doubling up on the namespace.

[ekampp]

I have narrowed the issue to Roar::Rails::Formats#entity_representer, which has this code:

model_name = model.class.name.underscore
if namespace = controller_path.namespace
  model_name = "#{namespace}/#{model_name}"
end

The case is that the model_name is v1/user (because the entire controller and model is wrapped in V1 module). The controller path is v1/users, which triggers adding the namespace to the model, which becomes: v1/v1/user, which is wrong.

The problem is that the code doesn't expect the model to be namespaces, only the controller.