Issue when using enumerable exhibit, the enumerable exhibit is passed to subsequent exhibits #27

Closed
leehambley opened this Issue Dec 10, 2012 · 11 comments

2 participants

@leehambley

Given the following:

irb(main):009:0> DisplayCase::Exhibit.exhibits
=> [DisplayCase::Exhibit::Exhibited, DisplayCase::EnumerableExhibit, Api::V2::AccountExhibit, Api::V2::ApplicationExhibit, Api::V2::EventExhibit, Api::V2::LocationExhibit, Api::V2::RoleExhibit, Api::V2::ScheduleExhibit, Api::V2::ShiftCategoryExhibit, Api::V2::ShiftExhibit, Api::V2::UserExhibit]

Of which only for this instance Api::V2::LocationExhibit is important, implementation here:

module Api
  module V2
    class LocationExhibit < DisplayCase::Exhibit
      def self.applicable_to?(object)
        # >>>>>>>>>> The ugliest debugging way in the world <<<<<<<<<<<<<
        warn object.class # This is the debug warning!
        warn object.inspect
        object_is_any_of?(object, 'Location')
      end
      def to_hash
        # ... snip ...
      end
      def to_json(options={})
        # ... snip ...
      end
    end
  end
end

The problem is when calling something like:

irb(main):008:0> exhibit(current_user.locations).to_json

The warning above has received object as an instance of Array, which when #inspect'ed reports as (snipped) DisplayCase::EnumerableExhibit((....locations here....)), more info:

irb(main):002:0> exhibit(current_user.locations).to_json
  Location Load (0.5ms)  SELECT DISTINCT `locations`.* FROM `locations` INNER JOIN `shift_categories` ON `locations`.`id` = `shift_categories`.`location_id` INNER JOIN `shift_categories_users` ON `shift_categories`.`id` = `shift_categories_users`.`shift_category_id` WHERE `shift_categories_users`.`user_id` = 493
Array
DisplayCase::EnumerableExhibit(([#<Location id: 64, name: "Test Location", account_id: 159, #....snip....# shift_categories_count: 6>]))
=> "[{\"applications_#......snip.....#_sort_by\":\"working_hours\"}]"

Problem: The LocationExhibit applicable_to?() is only entered once, and receives the DisplayCase::EnumerableExhibit( wrapped list of locations, I believe the LocationExhibit should also be queried once, for each of the locations in the array?

Question: Should it not be the case that the Locations exhibit is passed each location in the list, first? Am I mis-using the EnumerableExhibit?

Here's a little more about the structure of locations, but it's a simple ActiveRecord relation...

irb(main):007:0> current_user.locations.class
=> Array

irb(main):008:0> current_user.locations.size
=> 1

irb(main):009:0> current_user.locations.map(&:class)
=> [Location(id: integer, name: string, account_id: integer, created_at: datetime, updated_at: datetime, slug: string, logo_file_name: string, logo_content_type: string, logo_file_size: integer, logo_updated_at: datetime, first_day: integer, slot_minutes: integer, min_time: integer, max_time: integer, default_view: string, default_event_minutes: integer, show_event_header: boolean, applications_visible: string, assignments_visible: string, swap_shifts: boolean, manners_address: string, users_sort_by: string, notes_visible: boolean, shift_categories_count: integer)]

@leehambley

More investigation has yielded that to_json on the EnumerableExhibit is not implemented, neither is to_hash, so the use-case:

  render json: exhibit(current_user.things_and_stuff)

We considered re-opening the class in our application to implement them both

module DisplayCase
  class EnumerableExhibit
    def to_json
      __getobj__.map { |i| exhibit(i).to_hash }.to_json
    end
  end
end
@codeodor
Objects on Rails member

Lee,

Thanks for the detailed info. Let me play around with it a bit so I can figure out the proper solution.

I don't remember off the top of my head why it should be doing that, and my inclination is that it should remain a relation instead of running through it all (or at least continue to behave like one).

Clearly there's a reason it's not doing that, so I need to figure out why, and if that's on purpose or not.

@codeodor
Objects on Rails member

@leehambley,

I've looked this over a bit more. I think the problem is your applicable_to? test is:

object_is_any_of?(object, 'Location')

But that would mean it's looking for a String, and since the contents are Locations it's not applying the LocationExhibit.

What happens if you change the test to:

object_is_any_of?(object, Location)

?

Does it then apply the LocationExhibit and call its to_json method?

Let me know if I've overlooked something, or misunderstood.

@leehambley

object_is_any_of? is supposed to take a string, it operates on class names. The problem was worked-around on our side by implementing the DisplayCase::EnumerableExhibit as such:

require 'base64'
require 'display_case/enumerable_exhibit'

module DisplayCase
  class EnumerableExhibit
    def to_hash_array(options={})
      __getobj__.map do |i|
        exhibit(i).to_hash
      end
    end
    def to_json(options={})
      to_hash_array.to_json
    end
  end
end
@leehambley leehambley closed this Dec 18, 2012
@leehambley

See also: #28

@codeodor
Objects on Rails member

Lee,

Sorry, I was looking at it backward. I see it just calls #to_s on each of the classes, but I thought it was calling name.

Sam

@codeodor codeodor reopened this Dec 18, 2012
@codeodor
Objects on Rails member

Re: #28, in #29 @avdi mentioned he's working on a PR, so I'll see what he does there first. Thanks!

@codeodor
Objects on Rails member

Lee, I reopened this because I'd like to look into it some more.

I have a question though: why did you convert to hash instead of call to_json on each element?

@leehambley

We use the hash states of various exhibits when nesting exhibits in other exhibited objects; using JSON directly would be like adding a string full of special characters, which would be double-encoded on the way up to the browser.

@codeodor
Objects on Rails member

Thanks again for your help on this @leehambley.

I decided to sit down for a while this morning and see if this was still an issue, given there have been a few changes in the last few months.

I'm proud to report that I think I've finally figured out the disconnect (though I'm still not sure of the solution).

You mentioned "Should it not be the case that the Locations exhibit is passed each location in the list, first? Am I mis-using the EnumerableExhibit?"

But in my view at the time, it clearly did do that: https://github.com/objects-on-rails/display-case/blob/master/spec/exhibits/enumerable_exhibit_spec.rb#L36

However, I figured out the problem when testing this morning. Suppose I have an ActiveRecord model for District. Then DisplayCase will have 2 different behaviors depending on how I call a method (in your case, to_json).

The first, we access an element directly:

districts = District.limit(3)
districts = DisplayCase::Exhibit.exhibit(districts)
districts[0].to_json #=> call the DistrictExhibit#to_json, if defined, or delegates to District#to_json if not

In the secon, we call it on the collection:

districts = District.limit(3)
districts = DisplayCase::Exhibit.exhibit(districts)
districts.to_json #=> calls #to_json on the ActiveRecord::Relation, since you do not have an exhibit for ActiveRecord::Relation which implements #to_json

As you mentioned, the EnumerableExhibit does not implement it, so what happens is it delegates to ActiveRecord::Relation, which uses #to_json on all the ActiveRecord objects, instead of what you were expecting,that it would do it on the exhibited objects.

At first, I was not sure if we should assume we can override #to_json for an Enumerable (and I'm still of that opinion, as you'll see below).

However, one thing that had me swaying is that the way Rails does it, it doesn't seem to matter if it's an ActiveRecord::Relation nor an Array. Even if I call #to_a on the relation, it has this behavior where it's not using the method on the exhibited object, even though we're overriding #to_json.

Anyway, I did some digging and found out why that is the case: Rails ends up calling as_json on the objects when you call to_json.

That reminded me of If you're using to_json, you're doing it wrong, and of course you can see if you browse the encoding support in Rails, we should totally be overriding as_json, not to_json.

Even then, however, it's somehow managing to call as_json on our ActiveRecord objects using the ActiveModel serializer, not on our exhibited object. I haven't managed to figure out what's going on there, but I want to before I implement it in DisplayCase, because I'd like to understand what is really going on before I patch it -- it feels rather like checking if defined?(something) to get rid of an error you're seeing, rather than figuring out the root cause of the error. 😄

In the mean time, you don't really need the monkey patch: you could implement as_json and then call it on the collection you're passing to respond_with, and it will work.

I will continue to try to figure out why it ends up going to the ActiveModel serializer as_json method rather than using the one you defined when you call to_json though!

@codeodor
Objects on Rails member

Ok, I think I've figured it out.

EnumerableExhibit derives from Exhibit, which derives from SimpleDelegate, which descends from BasicObject. It also includes the Enumerable module.

Rails defines #to_json on Object, Array, and Hash, but not Enumerable, so our BasicObject does not get it from any of the places it'd be likely to get it.

Because of that, the call gets forwarded to the ActiveRecord::Relation, which converts itself to an array, and then calls #to_json.

However, Rails does define #as_json on Enumerable.

That's why our call to #as_json works, but #to_json does not.

For now, I've implemented #to_json on our EnumerableExhibit, and it fixes the problem (it first calls #as_json, then #to_json on that). I'm also going to open a pull request to Rails to see if they think Enumerable has a place in having #to_json defined.

@codeodor codeodor closed this Apr 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment