Skip to content

Fix resource serialization involving last_result_set #351

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

Conversation

stokarenko
Copy link
Contributor

Sometimes JsonApiClient::Resource instance evaluates self.relationships.last_result_set = last_result_set or self.relationships.last_result_set = nil. This behavior was introduced by performance optimization commit.

But we never read the last_result_set property on JsonApiClient::Relationships::Relations objects, only the writes everywhere. That means that these writes are kind of rudiments.

Even worse, there is no attr_accessor :last_result_set defined for JsonApiClient::Relationships::Relations class, this way an attempt to write last_result_set property triggers Helpers::DynamicAttributes routine - last_result_set value become to be a part of attributes hash, and will be serialized then.

Lets remove rudimental writes to relationships.last_result_set. Without this fix the newly added test fails like this:

  1. Failure:
    SerializingTest#test_as_json_involving_last_result_set [/Users/st/dev/ror/everfi/json_api_client/test/unit/serializing_test.rb:75]:
    --- expected
    +++ actual
    @@ -1 +1 @@
    -{"type"=>"articles", "id"=>"1", "attributes"=>{"title"=>"Rails is Omakase"}}
    +{"id"=>"1", "type"=>"articles", "relationships"=>{"last_result_set"=>{"data"=>[{"type"=>"articles", "id"=>"1"}]}}, "attributes"=>{"title"=>"Rails is Omakase"}}

@stokarenko
Copy link
Contributor Author

@gaorlov Hi again )

as an option we can add attr_accessor :last_result_set to JsonApiClient::Relationships::Relations class definition, but I'm pretty sure that these writes are rudimental )

@stokarenko stokarenko force-pushed the fix-resource-serialization-involving-last-result-set branch from 1ee8c05 to c2deea5 Compare June 28, 2019 15:47
@gaorlov gaorlov requested a review from senid231 June 28, 2019 18:00
Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like relationships.last_result_set was being handled by the dynamic attribute helper; I don't think we need to add an accessor.
This looks fine to me, but I'd like to understand what that code did int he first place. @senid231 can you explain your thinking here?

@stokarenko
Copy link
Contributor Author

@gaorlov @senid231

Looks like relationships.last_result_set was being handled by the dynamic attribute helper

Indeed, and that is a problem - this way such last_result_set will take a part in serialization data, raising json size for nothing..

This PR just removes all rudimental writes to relationships.last_result_set, that cures the core reason of problem. As an alternate solution we can add attr_accessor :last_result_set to relations class, and avoid the garbage in relation attributes this way - in case if I missed something important around relationships.last_result_set :)

@gaorlov
Copy link
Collaborator

gaorlov commented Jul 1, 2019

@senid231 have you had a chance to look over this? I'm fine with adding the attr_accessor, but i'd like to better understand why the changes were introduces before we expand the API surface.
Thanks!

@senid231
Copy link
Member

senid231 commented Jul 3, 2019

@gaorlov @stokarenko indeed last_result_set is useless for relationships

I've introduce it by mistake while add relationship caching

thanks for fixing it, @stokarenko

@senid231 senid231 merged commit 23c337e into JsonApiClient:master Jul 3, 2019
@gaorlov
Copy link
Collaborator

gaorlov commented Jul 3, 2019

1.14.0 is live. Thanks for the contribution!

@stokarenko
Copy link
Contributor Author

Thank you a lot! )

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

Successfully merging this pull request may close these issues.

3 participants