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

Using Draper with Rabl #286

Closed
cmer opened this issue Jul 11, 2012 · 13 comments
Closed

Using Draper with Rabl #286

cmer opened this issue Jul 11, 2012 · 13 comments
Labels

Comments

@cmer
Copy link

cmer commented Jul 11, 2012

I have the following templates:

# autocomplete_suggestions
collection @users
extends "memberships/autocomplete_suggestion"

and

# autocomplete_suggestion
object UserDecorator.decorate(@user)
attributes :id

node(:name)     { |u| u.full_name }
node(:avatar)   { |u| u.avatar_url }
node(:type)     { |u| u.class.to_s }
node(:location) { |u| u.location || '' }

avatar_url is defined in my Draper decorator, but I cannot access it from inside the node. Is there a way to wrap my @user object in a decorator from inside Rabl?

@databyte
Copy link
Collaborator

This may be bleeding into the internals of Rabl so even though I suggest this as a workaround, it makes sense for us to provide a callback to construct your object.

See if this even works:

collection @users
extends "memberships/autocomplete_suggestion", object: UserDecorator.decorate(@_object)

or leave your autocomplete_suggestions as-is and try this in your extended template:

object UserDecorator.decorate(root_object)
attributes :id
# ...

@cmer
Copy link
Author

cmer commented Jul 12, 2012

I used the second option since it looked a bit less hackish and it worked like a charm! Thanks!

@cmer cmer closed this as completed Jul 12, 2012
@cmer cmer reopened this Jul 12, 2012
@cmer
Copy link
Author

cmer commented Jul 12, 2012

Actually I spoke too soon. None of the above work.

@databyte
Copy link
Collaborator

I can look into this later or... if you setup a stupid quick Rails app and link me to it, it'll save me a few mins and I can look at it sooner.

@cmer
Copy link
Author

cmer commented Jul 16, 2012

Here's a sample app of this: http://db.tt/WzCgeOUZ . Just start the Rails server and go to http://0.0.0.0:3000/users.json

I'm aware that I could do something like UserDecorator.decorate(User.all) in my controller but since I heavily cache my JSON, I want to decorate the user object directly in the view to avoid instantiating a bunch of decorator objects whenever possible for performance reasons.

It seems like object UserDecorator.decorate(@user) should perform what I need but it doesn't.

Thanks!

@databyte
Copy link
Collaborator

I'll check it out later tonight.

@databyte
Copy link
Collaborator

So basically it does work with partial the first time through when evaluating the block (via partial#object_to_hash) but screws up when hitting engine#to_json which calls engine#to_hash with the original collection object. The fact that the partial is being expressed twice is an entirely different issue.

Ugh, I think the best way to deal with this permanently will be a more predictable DSL which we still need to get around to. That way, each block itself can just be instance_eval'd such that the results just return up without having to call around between engine and builder.

All that above is mostly for @nesquena and I. Here's your solution:

# index.json.rabl
collection @users
extends "users/show"

# show.json.rabl
attributes :id, :full_name

# users_controller.rb
class UsersController < ApplicationController
  def index
    @users = User.all.map {|user| UserDecorator.decorate(user) }
  end
end

# user_decorator.rb
class UserDecorator < Draper::Base
  decorates :user

  def full_name
    "#{user.first_name} #{user.last_name}"
  end
end

To go over the changes. The UserDecorator uses user as the object to access first_name and last_name - not model. The show.json.rabl file didn't need nodes per-se because id and full_name are just attributes on the model. The index.json.rabl does the same basic thing you had already setup.

But the biggest change is to have the controller setup the decorator before sending it to the view.

This is how I probably would've done it anyway. Think about an ERB. You probably weren't going to call UserDecorator in the form_for helper or anything - it makes sense to have the object already setup before you get into the view.

Now it still should've worked from within rabl when passing the object a modified object via partial. So I'll call that a bug that we'll address with the refactor in v1. But the solution above is a proper presentation / decorator setup. Controllers interface with models and setup everything for the dumb views.

I like to say that I want my views to control what's seen - not how it's seen.

@cmer
Copy link
Author

cmer commented Jul 17, 2012

@databyte As I said in my previous comment, I actually do instantiate the decorators directly in the view to benefit from caching. When going through many objects, doing so makes the execution much faster at the expense of having a tiny bit of logic in the view. If I can avoid instantiating a 1000 objects, that's a fair price to pay IMO.

For now I'll just move the method I need directly in the model but it would be great if there was a hook in RABL to manipulate the data before displaying it.

Thanks!

@databyte
Copy link
Collaborator

The biggest call in your controller is going to be to User.all, not really the thin decorators but I see what you mean.

You'll still have a problem with the call because of the way caching works in views. If you decorate an object as a parameter to rendering the partial, the object will still be created even if the contents are cached because you need the object first and then grab the cache_key from that object.

So what you need is caching layers such that:

1. #index called >
2. caching against the collection of User.all, may or may not require db access (aka action caching) >
3. User.all hits database >
4. render view call >
5. iterate over each user, decorated or not >
6. caching against user.cache_key >
7. parse and process template

So if you look at the rendering process, you can't prevent user being decorated unless you can cache against User.all to know that you don't have to render at all.

The trick with view caching at that last level is saving time with any associated models (N+1 queries) and any template fu (concat strings, complex helpers, 100 partials, etc).

@cmer
Copy link
Author

cmer commented Jul 18, 2012

Of course my sample project is just a rough example. In real life, I'd select just the id and updated_at to build the cache_key which is pretty fast.

I assume that the user partial is not rendered at all if there's a match for the cache_key? That's how I do it with ERB. I assume Rabl wouldn't execute any code in my partial unless it needs to render that partial?

@databyte
Copy link
Collaborator

Of course it's a sample - I didn't mean to imply otherwise. It's just that if you query the database to grab all users even just the ID column, it results in a lot of GC work. Compared to somehow storing a cache_key elsewhere.

For instance, in one system I had a scope that would grab the last_updated value for the entire table as the cache_key (in your case, user sorted desc by last_updated with limit 1). It works well if your table doesn't have a lot of writes obviously and this one was written to a few times a week.

The template is initially parsed to read in the object/cache values. We build up the DSL but don't evaluate it if a cache entry exists.

@databyte
Copy link
Collaborator

Closing given the solution in my comment above. We'll be sure to address it though in the API work that needs to be done.

@MaicolBen
Copy link

So I'll call that a bug that we'll address with the refactor in v1.

Any update on this? It is bad to map twice (in the controller decorating and in the rabl)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants