Follow up to #1454 #1504

groyoh · 2016-02-10T00:34:53Z

PR #1454 was merged with some missing fixes. All these fixes are addressed by this PR:

Rename ActiveModel::Serializer::Adapter::JsonApi::Association to ActiveModel::Serializer::Adapter::JsonApi::Relationship
Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module
Add unit test for ActiveModel::Serializer::Adapter::JsonApi::Relationship
Add unit test for ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier

@bf4 here you go ;)

groyoh · 2016-02-10T00:38:07Z

test/adapter/json_api/api_objects/relationship_test.rb

+            end
+
+            def test_relationship_data_not_included
+              test_relationship({}, options: { include_data: false })


This case should probably raise an error since JSONAPI does not allow the relationship to be empty (http://jsonapi.org/format/#document-resource-object-relationships). I'll let you guys decide how to procede with this one.

Depends if the conditions are programmer error. Generated nil values aren't included... So? In this case, is it always programmer error? Should it create an error response?

In this case, it would most likely be a programmer mistake who would do something like:

has_one :author do include_data false end

which is kind of useless but that may happen. This should be the concern of AMS to make sure that the serializer either does not serializer the whole relationship in such case (not usefull in my opinion) or raise an error to let the programmer know that this is a misuse of the gem.

FOLLOWUP

bf4 · 2016-02-10T01:56:33Z

Awesome, thanks! Let's keep up the momentum

bf4 · 2016-02-10T03:03:10Z

@groyoh Can you add a Changelog entry at the top of whichever section makes sense to you?

groyoh · 2016-02-10T10:10:09Z

@bf4 I'll add it to the changelog 😉

PR rails-api#1454 was merged with some missing fixes. All these fixes are addressed by this commit: - Rename ActiveModel::Serializer::Adapter::JsonApi::Association to ActiveModel::Serializer::Adapter::JsonApi::Relationship - Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module - Add unit test for ActiveModel::Serializer::Adapter::JsonApi::Relationship - Add unit test for ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier

groyoh · 2016-02-10T12:00:40Z

@bf4 I added the changelog. Should I also update the documentation or create another PR for this?

bf4 · 2016-02-10T14:28:38Z

If you're offering docs now is good :)

B mobile phone

On Feb 10, 2016, at 6:00 AM, Yohan Robert [email protected] wrote:

@bf4 I added the changelog. Should I also update the documentation or create another PR for this?

—
Reply to this email directly or view it on GitHub.

groyoh · 2016-02-10T14:33:42Z

@bf4 since this change mainly concerns JSONAPI, should it go into docs/serializers.md next to the associations documentation or somewhere else?

bf4 · 2016-02-10T15:11:49Z

@groyoh right now the serializers include some jsonapi concerns. As long as it's part of the serializer, I think it makes sense to put in that file, but include more usage info on it in the jsonapi or adapter section

When using the relationships DSL with a block e.g. has_one :bio do link :self, "some_link" end the "data" field would be rendered with a nil value even though the bio is not nil. This happened because the block return value was set to nil but used as a value for the "data" field.

groyoh · 2016-02-10T20:09:42Z

@bf4 @beauby I updated the PR because I figured out that #1454 actually had a bug. When using a block for a relationship like:

has_one :bio do
  link :self, "whatever"
end
# => { data: nil, links: { self: "whatever" } }

would always render data as nil because we would take the block return value as the relationship value. I added some specs for it too.

I am unsure about the solution though, let me know what you think.

groyoh · 2016-02-10T20:10:44Z

lib/active_model/serializer/reflection.rb

@@ -42,17 +42,17 @@ def initialize(*)

      def link(name, value = nil, &block)
        @_links[name] = block || value
-        nil
+        :nil


Not sure if this should be called :nil or something else. I actually wanted to have your input on this as I was not too creative here.

this is to signify something like :block_returned_nil_value?

Yes. For more details, there are two use cases to consider for the relationship block:

Where the block return the value we want to use for the relationship:

has_one :author do meta some: :thing object.own_author end

Where the block return some value to define that we want to use the default relationship:

has_one :author do meta some: :thing end

For the latest, I defined the value to be :nil but it could be change to anything else if a better suggestion pops up since this is the internal API.

Maybe create an issue or start a pr to discuss any improvements

B mobile phone

On Feb 12, 2016, at 6:11 PM, Yohan Robert [email protected] wrote:

In lib/active_model/serializer/reflection.rb:

@@ -42,17 +42,17 @@ def initialize(*)

def link(name, value = nil, &block) @_links[name] = block || value

nil

:nil
Yes. For more details, there are two use cases to consider for the relationship block:

Where the block return the value we want to use for the relationship:
has_one :author do
meta some: :thing
object.own_author
end
Where the block return some value to define that we want to use the default relationship:
has_one :author do
meta some: :thing
end
For the latest, I defined the value to be :nil but it could be change to anything else if a better suggestion pops up since this is the internal API.

—
Reply to this email directly or view it on GitHub.

groyoh · 2016-02-10T20:20:11Z

Also please let me know if I should squash the commits.

bf4 · 2016-02-11T04:39:01Z

lib/active_model/serializer/adapter/json_api.rb

        autoload :Meta
        autoload :Deserialization
+        require 'active_model/serializer/adapter/json_api/api_objects'


So, in https://github.com/rails-api/active_model_serializers/pull/1004/files#r52563837 I noticed that we've already started a pattern of sticking the json_api objects in the json_api folder. e.g. above we have Meta and Link which are also json_api object.

In code I removed I move that object into json_api/jsonapi.rb. What do you think about just sticking also the api objects in the json_api folder as we've essentially already been doing?

It's fine by me. Should I still keep the ApiObjects namespace?

NEEDS FOLLOWUP

groyoh · 2016-02-12T16:47:06Z

@scharfie have you tried with master or with my branch? #1454 indeed contained a bug and this PR should fix it.

scharfie · 2016-02-12T17:04:04Z

@groyoh Sorry, I just realized this - your branch does resolve the issue. Nice work 👍

bf4 · 2016-02-12T20:52:38Z

Merged!

groyoh force-pushed the association-blocks branch from 72b5699 to 8856736 Compare February 10, 2016 00:35

groyoh reviewed Feb 10, 2016
View reviewed changes

bf4 added V: 0.10.x C: JSON API S: Changelog Missing labels Feb 10, 2016

groyoh force-pushed the association-blocks branch from 8856736 to 2c41938 Compare February 10, 2016 11:59

groyoh reviewed Feb 10, 2016
View reviewed changes

bf4 reviewed Feb 11, 2016
View reviewed changes

bf4 added S: Needs followup and removed S: Changelog Missing labels Feb 12, 2016

bf4 merged commit b1fd433 into rails-api:master Feb 12, 2016

This was referenced Feb 13, 2016

[RFC] How to handle improper usage error #1514

Closed

Fix bug displaying nil for relationship link href #1516

Merged

groyoh deleted the association-blocks branch February 20, 2016 01:19

groyoh removed the S: Needs followup label Apr 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up to #1454 #1504

Follow up to #1454 #1504

groyoh commented Feb 10, 2016

groyoh Feb 10, 2016

bf4 Feb 10, 2016

groyoh Feb 10, 2016

bf4 Feb 12, 2016

bf4 commented Feb 10, 2016

bf4 commented Feb 10, 2016

groyoh commented Feb 10, 2016

groyoh commented Feb 10, 2016

bf4 commented Feb 10, 2016

groyoh commented Feb 10, 2016

bf4 commented Feb 10, 2016

groyoh commented Feb 10, 2016

groyoh Feb 10, 2016

bf4 Feb 12, 2016

groyoh Feb 13, 2016

bf4 Feb 14, 2016

groyoh commented Feb 10, 2016

bf4 Feb 11, 2016

groyoh Feb 11, 2016

bf4 Feb 12, 2016

groyoh commented Feb 12, 2016

scharfie commented Feb 12, 2016

bf4 commented Feb 12, 2016

Follow up to #1454 #1504

Follow up to #1454 #1504

Conversation

groyoh commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bf4 commented Feb 10, 2016

bf4 commented Feb 10, 2016

groyoh commented Feb 10, 2016

groyoh commented Feb 10, 2016

bf4 commented Feb 10, 2016

groyoh commented Feb 10, 2016

bf4 commented Feb 10, 2016

groyoh commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groyoh commented Feb 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groyoh commented Feb 12, 2016

scharfie commented Feb 12, 2016

bf4 commented Feb 12, 2016