artsy / garner

A set of Rack middleware and cache helpers that implement various caching strategies.
MIT License
344 stars 24 forks source link

Does Garner work with Roar? #55

Open monfresh opened 11 years ago

monfresh commented 11 years ago

Hi, Me again :smile: I'm using Roar as my representer. I have 3 models: Organization, Location, and Services. Here's my LocationRepresenter, which also includes some properties from the Organization and Service models:

require 'roar/representer/json'
require 'roar/representer/feature/hypermedia'

module LocationRepresenter
  include Roar::Representer::JSON
  include Roar::Representer::Feature::Hypermedia

  property :id
  # more location properties

  property :organization, extend: OrganizationRepresenter, class: Organization

  collection :services, :extend => ServiceRepresenter, :class => Service
end

Here's my Grape API location resource without Garner, which works fine:

...
get do
  locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
  locations.extend LocationsRepresenter
end

If I add Garner like this:

get do
  garner.options(expires_in: 15.minutes) do
    locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
    locations.extend LocationsRepresenter
  end
end

On the first request, I get the full representation. On the garnered request, I get the non-represented version of locations which is missing the organization and services stuff. I also tried garner.bind(Location) and garner.bind(Location).bind(Organization).bind(Service) and I'm still not getting the full representation back from garner.

If I try the same thing on an individual location:

get ':id' do
  garner.options(expires_in: 15.minutes) do
    location = Location.includes([:organization, :services]).find(params[:id])
    location.extend LocationRepresenter
  end
end

I get can't dump anonymous class #<Module:0x007f875130b3f0>.

I get the same dump error with the bind method:

get ':id' do
  garner.bind(Location) do
    location = Location.includes([:organization, :services]).find(params[:id])
    location.extend LocationRepresenter
  end
end

Any help would be greatly appreciated! Thanks!

fancyremarker commented 11 years ago

Hi @monfresh, thanks again for the helpful and detailed issue! The short answer is that right now, Garner probably does not support caching Roar presenters. However, I'd like for it to be supported, so let me take a look at this issue today along with the outstanding issue on #53, and I'll let you know.

In the meantime, if you extend the class with your Roar presenters outside the Garner block, it should work, although perhaps not optimally. So, for example:

garner.options(expires_in: 15.minutes) do
  locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
end
locations.extend LocationsRepresenter
dblock commented 11 years ago

You probably want to cache JSON within the Garner block instead of the presenter. The presenter is some code that is going to do the presentation part, and caching it won't save you much - you want to cache the result that has been presented. So call to_json on the presenter (?).

monfresh commented 11 years ago

@fancyremarker I forgot to mention that I did try calling the representer outside of the garner block. If I do this:

garner.bind(Location) do
 @locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
end
@locations.extend LocationsRepresenter

Garner returns null when I request /api/locations.

If I use the options:

garner.options(expires_in: 15.minutes) do
  @locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
end
@locations.extend LocationsRepresenter

I get unknown class/module Location.

I also tried caching just the representation like this:

get do
  locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
  garner.options(expires_in: 15.minutes) do
    locations.extend LocationsRepresenter
  end
end

but that returns the original locations, not the representation.

monfresh commented 11 years ago

I decided to give grape entities a try and was able to get Garner to work by doing this:

require "garner/mixins/rack"

module Ohana

  module Entities
    class Location < Grape::Entity
      expose :name
      expose :description
      ... 
    end
  end

  class Locations < Grape::API
    helpers Garner::Mixins::Rack
    use Rack::ConditionalGet
    use Rack::ETag

    desc 'Returns all locations'
    params do
      optional :page, type: Integer, default: 1
      optional :per_page, type: Integer
    end
    get '/locations' do
      garner.options(expires_in: 15.minutes) do
        locations = Location.page(params[:page]).per(params[:per_page])
        present locations, with: Ohana::Entities::Location
        locations.to_s
      end
    end
  end

Without adding locations.to_s, I get no _dump_data is defined for class StringIO as reported in issue #17. Is adding locations.to_s the right solution? Are there any adverse effects?

Is there something similar that can work with Roar? Instead of to_s, I tried to_json (in my Roar branch) as @dblock suggested, but that returns a String instead of JSON.

monfresh commented 11 years ago

I take that back. It's not working as expected with grape entities. The first request gives me the expected representation, but the second request returns this string: "#<Mongoid::Criteria:0x007faaa36d4db8>"

dblock commented 11 years ago

Call as_json on both.

monfresh commented 11 years ago

Thanks, that makes sense! It works great now. There was another part missing that I can't believe I overlooked. In order for garner to send back the representation, I have to explicitly set the locations variable to the represented version, then call as_json on that:

garner.options(expires_in: 15.minutes) do
  locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
  locations = present locations, with: Ohana::Entities::Location
  locations.as_json
end
monfresh commented 11 years ago

Note that the same thing does not work with Roar. I still get the non-represented version when I do this:

garner.options(expires_in: 15.minutes) do
  locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
  locations = locations.extend LocationsRepresenter
  locations.as_json
end
ekosz commented 10 years ago

@dblock Has there been any update on this on getting Roar to work with Garner?

dblock commented 10 years ago

I've been using these two together but without grape-roar and with my own representer, like so:

module Grape
  module Roar
    module Representer
      def self.included(base)
        base.extend(ClassMethods)
      end

      module ClassMethods
        def represent(object, options = {})
          object.extend self
          # our own version of Grape::Roar::Representer
          # returns a serializable object within a garner block
          object.to_json(options)
        end

        def entity_name
          self.name.split('::').last.gsub(/Presenter$/, '')
        end

        def exposures
          {}
        end

        def documentation
          Hash[representable_attrs.map do |attribute|
            property_name = attribute[:as].evaluate nil, nil
            next if property_name == '_links'
            [ property_name, { desc: attribute[:desc], type: attribute[:type] }]
          end.compact]
        end

        def build_definition(name, options, &block)
          options[:render_nil] = true
          options[:render_empty] = true
          super
        end
      end
    end
  end
end

Furthermore we render strings directly without encoding them as JSON:

# The original Grape formatter is more strict and doesn't allow String or nil.
module Grape
  module Formatter
    module Json
      class << self
        def call(object, env)
          return object if ! object || object.is_a?(String)
          return MultiJson.dump(object) if object.respond_to?(:to_json) # to_json will use ActiveSupport::JSON.encode, which double-encodes UTF-8 strings
          raise Grape::Exceptions::InvalidFormatter.new(object.class, 'json')
        end
      end
    end
  end
end

Maybe both should be combined into a grape-roar-garner gem, but seems all too hacky to me.

dblock commented 10 years ago

The above btw is https://github.com/dblock/grape-roar/issues/3. I think you could help by addressing this issue and adding a new Grape::Roar::Representer::JSON class that can be swapped. That will fix the garner part of the issue.

hadwinzhy commented 9 years ago

@monfresh i found just add to_hash make it work with representer. but i don't known why.
@dblock can u help to explain?

garner.options(expires_in: 15.minutes) do
  locations = Location.includes([:organization, :services]).page(params[:page]).per(params[:per_page])
  locations = locations.extend LocationsRepresenter
  locations.to_hash
end
dblock commented 9 years ago

The outer garner call caches the result of the block, which ends up being an array of locations extended with LocationsRepresenter. However that extend part is not actually serialized. When this is de-serialized the next time around the cached result doesn't know how to render itself.

By calling to_hash you serialize the actual output, not the array of instances that produces the output.