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

Relationships not respecting jsonapi standard outside controller. ( It's actually a serializer lookup problem ) #2095

Open
fazelmk opened this issue Apr 2, 2017 · 6 comments

Comments

@fazelmk
Copy link

fazelmk commented Apr 2, 2017

Tried debugging but didn't get anywhere. Maybe a problem caused by namespace.

Using

   active_model_serializers 
   ruby 2.2.5p319 (2016-04-26 revision 54774) [x86_64-linux]
   Rails 5.0.2
   Ubuntu 14.04 64bits

Serializers

class V1::Admin::CharacteristicSerializer < V1::ApiSerializer
  attributes :id, :name, :characteristic_type, :min, :max, :active

  has_many :admin_characteristic_values, each_serializer: ::V1::Admin::CharacteristicValueSerializer
end
class V1::Admin::CharacteristicValueSerializer < V1::ApiSerializer
  attributes :id, :value, :active

  belongs_to :admin_characteristic, serializer: ::V1::Admin::CharacteristicSerializer
end

Expected behavior vs actual behavior

Behavior outside controller

object = ::Admin::Characteristic.includes(:admin_characteristic_values)
ActiveModelSerializers::SerializableResource.new(object, each_serializer: V1::Admin::CharacteristicSerializer).as_json
:relationships=>
      :admin_characteristic_values=>
       {:data=>
         [{"id"=>21, "value"=>"Value 0", "active"=>true, "admin_characteristic_id"=>5, "created_at"=>Sun, 02 Apr 2017 09:25:18 BRT -03:00, "updated_at"=>Sun, 02 Apr 2017 09:25:18 BRT -03:00},
          {"id"=>23, "value"=>"Value 2", "active"=>true, "admin_characteristic_id"=>5, "created_at"=>Sun, 02 Apr 2017 09:25:18 BRT -03:00, "updated_at"=>Sun, 02 Apr 2017 09:25:18 BRT -03:00},
          {"id"=>25, "value"=>"Value 4", "active"=>true, "admin_characteristic_id"=>5, "created_at"=>Sun, 02 Apr 2017 09:25:18 BRT -03:00, "updated_at"=>Sun, 02 Apr 2017 09:25:18 BRT -03:00}]}

Behavior inside controller

"relationships"=>
   "admin_characteristic_values"=>
     {"data"=>
       [{"id"=>"1", "type"=>"admin-characteristic_value"},
        {"id"=>"3", "type"=>"admin-characteristic_value"},
        {"id"=>"5", "type"=>"admin-characteristic_value"}]}

@fazelmk
Copy link
Author

fazelmk commented Apr 3, 2017

I did some debugging. And this is as far as got til now.

active_model_serializers/adapter/json_api.rb

437: def relationships_for(serializer, requested_associations, include_slice)
438:   include_directive = JSONAPI::IncludeDirective.new(
439:     requested_associations,
440:     allow_wildcard: true
441:   )
442:   serializer.associations(include_directive, include_slice).each_with_object({}) do |association, hash|
443:     hash[association.key] = Relationship.new(serializer, instance_options, association).as_json
444:     binding.pry
445:   end
446: end

From controller using render json: @records, each_serializer: V1::Admin::CharacteristicSerializer

hash[assotiation.key] has

{:data=>[{:id=>"1", :type=>"admin-characteristic_value"}, {:id=>"3", :type=>"admin-characteristic_value"}]}

and assotiation variable has

#<struct ActiveModel::Serializer::Association
name=:admin_characteristic_values,
options=
 {:each_serializer=>V1::Admin::CharacteristicValueSerializer,
  :namespace=>V1::Admin,
  :include_data=>true,
  :links=>{},
  :meta=>nil,
  :serializer=>
   #<ActiveModel::Serializer::CollectionSerializer:0x007efe28731298
    @object=
     [#<Admin::CharacteristicValue:0x007efe28ec9938
       id: 1,
       value: "Value 0",
       active: true,
       admin_characteristic_id: 1,
       created_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00,
       updated_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00>,
      #<Admin::CharacteristicValue:0x007efe28ec95f0
       id: 3,
       value: "Value 2",
       active: true,
       admin_characteristic_id: 1,
       created_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00,
       updated_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00>],
    @options=
     {:namespace=>V1::Admin,
      :scope=>#<User id: 1, email: "teste@teste.teste", created_at: "2017-04-03 22:05:52", updated_at: "2017-04-03 22:05:52">,
      :scope_name=>:current_user,
      :serializer_context_class=>V1::Admin::CharacteristicSerializer},
    @root=nil,
    @serializers=
     [#<V1::Admin::CharacteristicValueSerializer:0x007efe26f0f318
       @instance_options=
        {:namespace=>V1::Admin,
         :scope=>#<User id: 1, email: "teste@teste.teste", created_at: "2017-04-03 22:05:52", updated_at: "2017-04-03 22:05:52">,
         :scope_name=>:current_user,
         :serializer_context_class=>V1::Admin::CharacteristicSerializer},
       @object=
        #<Admin::CharacteristicValue:0x007efe28ec9938
         id: 1,
         value: "Value 0",
         active: true,
         admin_characteristic_id: 1,
         created_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00,
         updated_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00>,
       @root=nil,
       @scope=#<User id: 1, email: "teste@teste.teste", created_at: "2017-04-03 22:05:52", updated_at: "2017-04-03 22:05:52">>,
      #<V1::Admin::CharacteristicValueSerializer:0x007efe26f0bce0
       @instance_options=
        {:namespace=>V1::Admin,
         :scope=>#<User id: 1, email: "teste@teste.teste", created_at: "2017-04-03 22:05:52", updated_at: "2017-04-03 22:05:52">,
         :scope_name=>:current_user,
         :serializer_context_class=>V1::Admin::CharacteristicSerializer},
       @object=
        #<Admin::CharacteristicValue:0x007efe28ec95f0
         id: 3,
         value: "Value 2",
         active: true,
         admin_characteristic_id: 1,
         created_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00,
         updated_at: Mon, 03 Apr 2017 19:05:52 BRT -03:00>,
       @root=nil,
       @scope=#<User id: 1, email: "teste@teste.teste", created_at: "2017-04-03 22:05:52", updated_at: "2017-04-03 22:05:52">>]>},
block=nil>

From rspec using ActiveModelSerializers::SerializableResource.new( @records, each_serializer: V1::Admin::CharacteristicSerializer )

hash[assotiation.key] has

{:data=>
  [{"id"=>1, "value"=>"Value 0", "active"=>true, "admin_characteristic_id"=>1, "created_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00, "updated_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00},
   {"id"=>3, "value"=>"Value 2", "active"=>true, "admin_characteristic_id"=>1, "created_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00, "updated_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00}]}

and assotiation variable has

#<struct ActiveModel::Serializer::Association
 name=:admin_characteristic_values,
 options=
  {:each_serializer=>V1::Admin::CharacteristicValueSerializer,
   :namespace=>nil,
   :include_data=>true,
   :links=>{},
   :meta=>nil,
   :virtual_value=>
    [{"id"=>1, "value"=>"Value 0", "active"=>true, "admin_characteristic_id"=>1, "created_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00, "updated_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00},
     {"id"=>3, "value"=>"Value 2", "active"=>true, "admin_characteristic_id"=>1, "created_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00, "updated_at"=>Mon, 03 Apr 2017 19:05:52 BRT -03:00}]},
 block=nil>

someone has any clue why the difference?

@fazelmk
Copy link
Author

fazelmk commented Apr 3, 2017

Ok the serializer lookup is causing the problem due to namespacing.

ActiveModelSerializers::SerializableResource.new( @records, { each_serializer: V1::Admin::CharacteristicSerializer namespace: "V1::Admin" }) solved the issue.

I think it would be better to look first in the serializer to see if the serializer was defined (I did define it), then look in the same namespace and finally look in the root namespace.

@fazelmk fazelmk changed the title Relationships not respecting jsonapi standard outside controller. Relationships not respecting jsonapi standard outside controller. ( It's actually a serializer lookup problem ) Apr 3, 2017
@bf4
Copy link
Member

bf4 commented Apr 4, 2017

@fazelmk Thanks so much for this!

I'm sorry you had to read through that json api relationships code. It's a beast!

What do you think we should do to help future us?

@fazelmk
Copy link
Author

fazelmk commented Apr 4, 2017

@bf4 this is what I would do.

If the serializer for the relationship was defined (as the code bellow shows) it should use it.

class V1::Admin::CharacteristicSerializer < V1::ApiSerializer
  attributes :id, :name, :characteristic_type, :min, :max, :active

  has_many :admin_characteristic_values, each_serializer: ::V1::Admin::CharacteristicValueSerializer
end

Basic lookup (leave some common use cases not treated)
first with same namespace of the serializer containing the association
if did'n find use root namespace

Accurate lookup
It may be wise to do a lookup in every namespace going from the namespace of the serializer to the root namespace.

As the example I got namespace V1::Admin, if I didn't declare the serializer it could be V1::Admin or just V1 if it's a serializer outside the admin namespace. This would cover almost all the use cases.

Leaving cross-version (which I think shouldn't be treated) and models in sub-domains of the serilizer domain (which we wouldn't have at hand to do lookup).

This exceptions can be treated by declare the serializer for the relationship in the current serializer, so it won't be a problem for the user.

I didn't dig up enough to find the lookup code. If you point me in the right direction I can make a PR for this.

@bf4
Copy link
Member

bf4 commented Apr 12, 2017

@fazelmk I just want to acknowledge I've read this. I'm spread pretty thinly and haven't yet read it carefully enough to make a suggestion.

@ysyyork
Copy link

ysyyork commented Feb 28, 2018

Jesus, this saved me a day lol. Thanks @fazelmk

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

No branches or pull requests

3 participants