belongs_to causes unnecessary db hits

[ssendev]

failing test for #762

also fails with Adapter::Json but i'm not sure what the expected hash would look like (embed ids appears to have no effect with json adapter blog name still appears in the resulting hash (was this changed in 0.10?)).

[bf4]

typo: doesnt

[bf4]

👍 Nice

[beauby]

I would call the class PostSerializer instead of post_serializer (although I know you took the code from somewhere else, but I think this is more 'standard').
Also, I would write it like that:

class PostSerializer < ActiveModel::Serializer
  belongs_to :blog, embed: :ids
end

as I find it less confusing, but here people might disagree.

[ssendev]

tried changing it but it seems like this notation is not accepted in a method.

[beauby]

Very true indeed, I overlooked that part.

[bf4]

You could

def test_something
  post_serializer = Class.new(ActiveModel::Serializer)
  Object.const_set(:PostSerializer, post_serializer)
  PostSerializer.whatever
ensure
    Object.send(:remove_const, :PostSerializer)
end

e.g. see

active_model_serializers/test/serializers/adapter_for_test.rb

Lines 93 to 144 in 18e5812

	def test_get_adapter_from_environment_registers_adapter
	ActiveModel::Serializer::Adapter.const_set(:AdapterFromEnvironment, Class.new)
	klass = ::ActiveModel::Serializer::Adapter::AdapterFromEnvironment
	name = "adapter_from_environment".freeze
	assert_equal ActiveModel::Serializer::Adapter.get(name), klass
	assert ActiveModel::Serializer::Adapter.adapters.include?(name)
	ensure
	ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete(name)
	ActiveModel::Serializer::Adapter.send(:remove_const, :AdapterFromEnvironment)
	end
	def test_get_adapter_for_unknown_name
	assert_raises UnknownAdapterError do
	ActiveModel::Serializer::Adapter.get(:json_simple)
	end
	end
	def test_adapter
	assert_equal ActiveModel::Serializer.config.adapter, :flatten_json
	assert_equal ActiveModel::Serializer.adapter, ActiveModel::Serializer::Adapter::FlattenJson
	end
	def test_register_adapter
	new_adapter_name = :foo
	new_adapter_klass = Class.new
	ActiveModel::Serializer::Adapter.register(new_adapter_name, new_adapter_klass)
	assert ActiveModel::Serializer::Adapter.adapters.include?("foo".freeze)
	assert ActiveModel::Serializer::Adapter.get(:foo), new_adapter_klass
	ensure
	ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete(new_adapter_name.to_s)
	end
	def test_inherited_adapter_hooks_register_adapter
	Object.const_set(:MyAdapter, Class.new)
	my_adapter = MyAdapter
	ActiveModel::Serializer::Adapter.inherited(my_adapter)
	assert_equal ActiveModel::Serializer::Adapter.get(:my_adapter), my_adapter
	ensure
	ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete("my_adapter".freeze)
	Object.send(:remove_const, :MyAdapter)
	end
	def test_inherited_adapter_hooks_register_demodulized_adapter
	Object.const_set(:MyNamespace, Module.new)
	MyNamespace.const_set(:MyAdapter, Class.new)
	my_adapter = MyNamespace::MyAdapter
	ActiveModel::Serializer::Adapter.inherited(my_adapter)
	assert_equal ActiveModel::Serializer::Adapter.get(:my_adapter), my_adapter
	ensure
	ActiveModel::Serializer::Adapter::ADAPTER_MAP.delete("my_adapter".freeze)
	MyNamespace.send(:remove_const, :MyAdapter)
	Object.send(:remove_const, :MyNamespace)

[ssendev]

very interesting. but in the referenced test it's required since that's what's being tested. here it seems like it would just make the test more complex for "no" benefit. especially considering there already is a PostSerializer which would have to be restored.

[beauby]

Agreed

[joaomdmoura]

I'm okay with it.j

[beauby]

After giving it a thought, I am not sure we can avoid this "unnecessary db hit". What happens if we have belongs_to :author in the serializer, and belongs_to :author, class_name: "User" in the model? Then I fear AMS wouldn't know how to set the type property of the Resource Identifier.

[ssendev]

belongs_to :author, type: "users" comes to mind. i mean for attributes there's key

[beauby]

key for attributes is something different: it exposes an existing attribute under a different name (in the serialized document).
I have a seemless fix for that extra db hit, I just need #1103 to be merged first.

[joaomdmoura]

haha nice trick! 👍

[joaomdmoura]

Thank you @ssendev this is indeed something that we should address, we should not do unnecessary queries or anything that might slow down the apps performance.

As @beauby this might be a little bit tricky because of the example:
belongs_to :author, class_name: "User"

What you suggest might be a way to make it work belongs_to :author, type: "users" but I think we can try to discuss and come up with a more elegant solution. @beauby can you open a [WIP] PR with your solution, meanwhile I'll check #1103

[ssendev]

an alternative would be to specify a serializer and require json_api_type to be defined on it, or guess it from the serializers name . both solutions wouldn't be able to handle polymorphic relations but then there is probably an author_type column which could be consulted.

it would probably also be possible to to inspect the relationship on the model object.association(:author).reflection.class_name

[sandstrom]

This would be a godsend. Currently using AMS and need manual work-arounds to avoid these costly DB-lookups.

@ssendev has a great point, simply use reflection to inspect the relationships of the model. Specifying a type manually would be rare and only needed for polymorphic relationships.

I don't use ActiveRecord, so not 100% sure but I think Author.reflect_on_association(:books).klass works[1]. Mongoid, another popular ORM, also has good reflection support.

[1] https://github.com/rails/rails/blob/master/activerecord/lib/active_record/reflection.rb

[bf4]

So sorry we lost track of this!

[NullVoxPopuli]

needs rebase, I'd like to get this merged soonish

[ssendev]

this fails with

-{:data=>{:id=>"post", :type=>"posts", :relationships=>{:blog=>{:data=>{:id=>"blog", :type=>"blogs"}}}}}
+{:data=>{:id=>"post", :type=>"posts", :relationships=>{:blog=>{:data=>nil}}}}

is this expected or am i missing something?

[NullVoxPopuli]

@ssendev is that a conflict in the rebase? or is that the result of a test? (I guess I don't know what context that is in)

[ssendev]

https://travis-ci.org/rails-api/active_model_serializers/jobs/132809729#L323-L324

[NullVoxPopuli]

it is yeah.

my initial hunch is that something is off, because specifying a relationship with a data of nil seems weird. I'm going to check the json api spec, and see what is says about unspecified relationships.

It could be that we want to just omit that relationship entry.

[ssendev]

but the relationship is specified it just doesn't get serialized

[NullVoxPopuli]

ah, yes, I was looking at the wrong test.

[NullVoxPopuli]

I think I was confused earlier, and thought this was a fix for the problem. ha. I should not be on github when I'm tired.

[NullVoxPopuli]

anywho, I'm looking at fixing this

[NullVoxPopuli]

Closing in favor of #1750, as I added some notes in the code about what's happening where.