fix incorrect belongs_to serialization when foreign_key on object and belongs_to is blank

[InteNs]

Changes

ResourceIdentifier will return nil instead of a hash when the association foreign_key is blank
and jsonapi_use_foreign_key_on_belongs_to_relationship = true

Caveats

- locks jsonapi-renderer to < 0.2.1 untill jsonapi-rb/jsonapi-renderer#38 gets solved

Related GitHub issues

related #2335

Additional helpful information

[InteNs]

Had a quick try but can't get rake test to run:

LoadError: cannot load such file -- yard
/Users/intens/Projects/Fabriquartz/active_model_serializers/Rakefile:16:in `require'

[InteNs]

is this up for discussion? (it's blocking the PR)

lib/active_model_serializers/adapter/json_api/relationship.rb:47:9: C: Metrics/PerceivedComplexity: Perceived complexity for data_for_one is too high. [10/9]
        def data_for_one(association)
        ^^^

[wasifhossain]

I think it should be enough to make this test pass according to json_api specification.

def test_for_type_with_id_given_blank_id
  id = ''
  actual = ResourceIdentifier.for_type_with_id('admin_user', id, {})
  assert_nil actual
end

In order to do that, we simply need to return nil instead of { type: type } when id is blank in ResourceIdentifier.for_type_with_id

I tested it locally and all tests ran successfully along with the rubocop linter.

[InteNs]

@wasifhossain looks good, are you going to make a PR?

[wasifhossain]

@InteNs I think you are better equipped to test my proposed changes in your codebase and if you find it working, then take your time to update this PR. After that, we will take another look and merge if none of us find any issues with it

[InteNs]

aye, will test my fork in our staging environment

[InteNs]

tested this on staging over here and it's working fine.

[wasifhossain]

great to know 🙌 thanks for your help!

unfortunately we are having this issue with jsonapi-renderer at the moment. Lets see if there is any quick fix and decide later how should we move on afterwards.

[InteNs]

we are experiencing the same issue and downgraded the gem to 0.2.0

[wasifhossain]

I also think we can restrict the jsonapi-renderer version to 0.2.0 until this PR jsonapi-rb/jsonapi-renderer#38 gets merged, which is supposed to fix the current failure.

@InteNs can you please make a small tweak in active_model_serializers.gemspec#L45 to update the upper bound to < 0.2.1

spec.add_runtime_dependency 'jsonapi-renderer', ['>= 0.1.1.beta1', '< 0.2.1']

[wasifhossain]

@bf4 do you have any other suggestions? thanks

[InteNs]

the failing travis run seems to be a problem outside of this PR?

Also, since nothing has been released since february, Can I expect there to be a release soon after this merge?

[wasifhossain]

i agree with you @InteNs on travis failed case (looks to be rvm issue). btw @bf4 might have plans to release a new version some time soon

[wasifhossain]

@InteNs can you put a changelog entry to summarize the change this PR would bring? thanks!

[dawidof]

I also think we can restrict the jsonapi-renderer version to 0.2.0 until this PR jsonapi-rb/jsonapi-renderer#38 gets merged, which is supposed to fix the current failure.

@InteNs can you please make a small tweak in active_model_serializers.gemspec#L45 to update the upper bound to < 0.2.1

spec.add_runtime_dependency 'jsonapi-renderer', ['>= 0.1.1.beta1', '< 0.2.1']

jsonapi-rb/jsonapi-renderer#38 has been merged and jsonapi-renderer updated to 0.2.2. Try new version.

[wasifhossain]

looks like jsonapi-renderer is back to normal state 👏 , with a tiny thing still need to be fixed on our side to make the test suite pass.

let's remove that single space from 'author, comments' in the include option in test/action_controller/json_api/linked_test.rb

render json: [@post], adapter: :json_api, include: 'author,comments'

Ref: Include Doc

[wasifhossain]

👍

[wasifhossain]

note for future reference:

this change belongs to jsonapi-rb/jsonapi-renderer#38 only, irrelevant of this PR. we need to make this tweak according to the include doc in order to make the test suite pass.

[wasifhossain]

all tests are green again 🙌 I think we can merge it now