Fixes virtual value not being used

[GriffinHeart]

First PR here so let me know if I should change something,

Noticed that virtual_value was never being used and this PR fixes it

[joaomdmoura]

hey @GriffinHeart I appreciate what you did here 😄, but Virtual value is used behind the scenes, on every association that doesn't have a serializer itself as you check here

I'm not sure if virtual_value was created to be used as an option. Because, as you can see on the the serializer.rb (link above), it wouldn't work on associations that doesn't have a serializer (or arrays where one elements doesn't have a serializer)

Also, I can't see a use case, because you can only work with static values. But if you use the a method to override it instead, you have access to object and scope.

I'll not close it right now because I may have not seen some use case here, did you had something in mind when you decided to work on it? 😄

Btw your virtual value representation doesn't match the JSON API format. Check the resource linakge and the resource identifier object

... A "resource identifier object" MUST contain type and id members.

[bf4]

On the other hand @GriffinHeart has pointed out that val is passed in but not used

-          resource[:relationships][name] = { data: nil }
+          resource[:relationships][name] = { data: val }

[joaomdmoura]

Indeed. But val should be parsed to follow JSON API conventions, even without a serializer.

[bf4]

Fortunately, it appears the only place this code is used is here.

50          def add_relationship(resource, name, serializer, val=nil)
… 
137         def add_resource_relationships(attrs, serializer, options = {})
138           if association.respond_to?(:each)
139             add_relationships(attrs, name, association)
140           else
141             if opts[:virtual_value]
142               add_relationship(attrs, name, nil, opts[:virtual_value])
143             else
144               add_relationship(attrs, name, association)

Which means this code is equivalent to the below diff, unless I'm missing something

+50         def add_relationship(resource, name, serializer)
-50         def add_relationship(resource, name, serializer, val=nil)
… 
137         def add_resource_relationships(attrs, serializer, options = {})
138           if association.respond_to?(:each)
139             add_relationships(attrs, name, association)
140           else
141             if opts[:virtual_value]
+142              add_relationship(attrs, name, nil)
-142              add_relationship(attrs, name, nil, opts[:virtual_value])
143             else
144               add_relationship(attrs, name, association)

[GriffinHeart]

@joaomdmoura Sorry for a late reply.

My biggest issue is that if you add relationships that don't have a serialiser using the jsonapi adapter they will be rendered as nil. When the relationship doesn't have a serialiser it will be using the virtual_value to carry the relationship.

In either case I just want the option to be able to render something instead of a nil, which I'm not sure is compatible with jsonapi 1.0

[joaomdmoura]

@GriffinHeart you are right! Just tested it. Great catch!

So yeah, I'm willing to merge it. But I don't think it should be an option. But this might be done in another PR.

Another optmization would be formatting the object to fit with the JSON API conventions as I mentioned earlier, guaranteeing it would have the usual type and idkeys.

I'm merging this one because it's a great fix! But would be awesome to see both optimizations in further PRs 😄

[GriffinHeart]

@joaomdmoura Agreed, virtual_value doesn't make as much sense being a public option since you can always overwrite attributes and relationships

I think the real use case is not virtual_value just the ability to define the relationship like

has_many :rewards

def rewards
  { ...somestub }
end

and have the adapter use that for serialisation.