Skip to content
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

Support automatic pluralization of types #376

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

amysutedja
Copy link

Resolves #301

Enables support for pluralize_type in serializer definitions and relationship definitions, which pluralizes the type as expected.

Additional Note: In theory it's possible that we could try to infer whether relationship types should be pluralized by asking the relationship record's serializer about its pluralization state. But currently the code only interacts with the relationship record very lightly, and I was concerned about potentially missing performance targets for complicated records, so I didn't research this path very extensively.

end

context 'when sets plural type name' do
let(:type_name) { :films }
Copy link

Choose a reason for hiding this comment

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

Should remove this line, or else pluralize_type true isn't doing anything. This line manually sets the type to :films.

end

context 'when sets singular type name' do
let(:type_name) { :film }
Copy link

Choose a reason for hiding this comment

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

Every example in this group should be using let(:type_name) { :film }, so this should be describe-level.

Copy link
Author

Choose a reason for hiding this comment

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

Can you explain more about why this should be the case?

Copy link

@glyoko glyoko Apr 4, 2019

Choose a reason for hiding this comment

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

Look at line 541. That's directly setting the type to be plural in the before block, before the MovieSerializer.pluralize_type true line is even hit. In fact, you could comment out line 524 and that test would still pass.

Given that, both examples should be given a singular type name for these tests to check anything useful. In fact, you could even get rid of the let altogether and just say MovieSerializer.set_type :film on line 523.

end
end

describe '#pluralize_type after #set_type' do
Copy link

@glyoko glyoko Apr 4, 2019

Choose a reason for hiding this comment

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

As it stands there's an issue where type is not correctly pluralized in relationships. The following test shows this:

    subject(:serializable_hash) { MovieSerializer.new(movie, include: [:actors]).serializable_hash }
    # ...
    context 'when pluralizing a relationship type after #set_type' do
      before do
        ActorSerializer.pluralize_type true
      end

      after do
        ActorSerializer.pluralize_type nil
      end

      it 'returns correct hash which relationship type equals transformed set_type value' do
        expect(serializable_hash[:data][:relationships][:actors][:data][0][:type]).to eq(:actors)
        expect(serializable_hash[:included][0][:type]).to eq(:actors)
      end
    end

This also means there's a mismatch between relationship and include types. Currently:

> serializable_hash[:data][:relationships][:actors][:data][0][:type]
# => :actor
> serializable_hash[:included][:type]
# => :actors

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately, as far as I understand, fast_jsonapi does not actually "know" internally what serializer is associated with any given relationship. The relationship data is built via simply reflecting on the object and constructing the abbreviated relationship structure. In theory it's possible to make it aware of the serializer, but see my original "Additional Note" -- I ended up being worried about missing performance targets, so I didn't implement a change here.

In order to make this scenario work, I implemented the ability to mark a specific relationship as plural with pluralize_type: true. (Check out object_serializer_pluralization_spec.rb.) This isn't ideal, but it solves this scenario in lieu of a higher-level decision about perf targets.

Copy link

Choose a reason for hiding this comment

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

I sort of glossed over that note when I first reviewed this PR, and after banging my head against this for a bit, I'm coming to the same conclusion... I'm going to investigate this a bit more because it is important for my use case, but I understand that the complexity involved in such a refactor may not be worth the effort.

At any rate it may not be worth including in this PR, as it already achieves the majority of what it sets out to do.

@shishirmk shishirmk changed the base branch from master to dev April 7, 2019 19:42
@sdhull
Copy link

sdhull commented Jun 10, 2019

This would help us move our serializers over to fast_jsonapi. Any update on this PR?

@amysutedja
Copy link
Author

@sdhull I haven't heard back on any suggested changes from the maintainers. And the Travis builds break, for some reason.

You might want to note your interest in the original ticket, #301.

@zdavis
Copy link

zdavis commented Nov 22, 2019

I wonder if you'd be willing to open this PR against https://github.com/fast-jsonapi/fast_jsonapi, which seems to me to be the way forward with this library, since the maintainers aren't responsive here. The travis failures seem completely unrelated to your changes, and I'd love to see this PR merged into that fork of this repo.

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.

6 participants