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

move model_name into serializer, so it can get overwritten by serializer, similar to 'root' #1169

Closed
wants to merge 1 commit into from

Conversation

mreinsch
Copy link

I'm actually no quite sure why json_api uses serializer.object.class.model_name directly, instead of serializer.json_key. Then resource_identifier_type_for could be implemented like this:

def resource_identifier_type_for(serializer)
  if ActiveModel::Serializer.config.jsonapi_resource_type == :singular
    serializer.json_key
  else
    ActiveSupport::Inflector.pluralize(serializer.json_key)
  end
end

The json_key is further used in json api adapter, line 11: https://github.com/rails-api/active_model_serializers/blob/master/lib/active_model/serializer/adapter/json_api.rb#L11 - shouldn't this be unified?

@beauby
Copy link
Contributor

beauby commented Sep 17, 2015

There has been a lot of refactoring going on with the JsonApi adapter, and your concern about serializer.object.class.model_name is legitimate. The general situation about handling the type attribute is not satisfying for the moment because we should have a way to

  1. change the format globally (the first step was the plural/singular config, but we may want to support stuff like dasherized keys instead of underscored, and currently the namespaces don't play very well with the type attribute),
  2. override the type in a serializer (which your PR offers, on top of fixing an inconsistency). However, I think an overridden type attribute should not be subject to inflection. This is arguable, but I believe users who actively decide to modify the type should know what they are doing.

Note: there's a PR (#1122) that somehow solves 1. and 2., but in an inconsistent way (the override works by simply defining a type method in the serializer).

@mreinsch
Copy link
Author

@beauby , regarding point 2) that an overridden type attribute should not be subject of inflection: I think the adapter design makes this a bit more difficult, and I can imagine situations where you'd want plural for JSON API, but singular for some legacy API.

def object_model_name
object.class.model_name
end

Copy link
Member

Choose a reason for hiding this comment

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

I like the encapsulation.. would like to think a bit about adding another method, and also the name of this method.. or should we just use serializer.json_key and let you overwrite that?

Copy link
Author

Choose a reason for hiding this comment

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

using the 'serializer.json_key' is what I suggested in the description of the pull request as an alternative (I had this code already done before I realised this). Using 'serializer.json_key' would probably make more sense, even though the json api adapter then needs to pluralise it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bf4 IMO I don't think the serializer should have any methods on it relating to a specific adapter.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jfelchner
Copy link
Contributor

I worked on #1029 a bit last night and should have a new draft out today which will work better without diving into sub-hashes.

It'll also allow the user to define arbitrary (and separate) transformers for the root, the attributes, the types and the relationships.

I agree with @beauby that if the user is overriding something that it should not be subject to inflection.

I think the correct call here is to have two more methods on the serializer which return singular_model_name and plural_model_name. These would do the inflection by default. They could be overridden by the user to do whatever they wanted and then the adapter would just call singular_model_name/plural_model_name.

That would give the user the most flexibility. For the majority case, they'd just override model_name and let the default inflectors do their job. For the custom case, they could override one or both of the *_model_name methods.

@jfelchner
Copy link
Contributor

As it currently stands the serializer is too coupled to the adapter and needs to be broken apart, but I think the solution above will work for now.

@mreinsch
Copy link
Author

@jfelchner sounds good, looking forward to seeing this in master :)

@mreinsch mreinsch closed this Sep 18, 2015
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.

5 participants