RC3 Updates for JSON API #853
Conversation
| end | ||
|
|
||
| def type | ||
| object.class.to_s.demodulize.underscore.pluralize |
There was a problem hiding this comment.
- I believe this should go into the adapter because it's not really used by anyone else and it's a JSONAPI need.
- Having this in the serializer, we're forced to keep it public, which I think doesn't make much sense.
There was a problem hiding this comment.
Oh, I see what you're doing. You are:
- relying on the serialized object's type, not the serializer name.
- leaving it here so people can override the inferred type.
Is that it? I'm onboard with that.
There was a problem hiding this comment.
Yes, easily overriding the type is one of the main reasons I put this here. One can create an ApplicationSerializer and use different inflection rules.
I put the id here as well because it made thing more symmetrical, but I can remove it.
There was a problem hiding this comment.
Wouldn't be a good idea to add comments or GOTCHA's to those methods with explanation why the was placed here and what the original intention was behind that decision?
There was a problem hiding this comment.
Good idea, I was already planning on adding it to the documentation.
However, we need to figure out what to do about the id here. The main issue is that the jsonapi adapter requires an id attribute, so how do we handle the cases where the user doesn't add the id attribute to the configuration (or any other required attribute)? Raise an error, or add the id transparently? If the latter, I can implement a generic solution, something like
def attributes(options = {})
attributes =
if options[:fields]
self.class._attributes & options[:fields]
else
self.class._attributes.dup
end
attributes += options[:required_fields] if options[:required_fields]
attributes.each_with_object({}) do |name, hash|
hash[name] = read_attribute(name)
end
end
def read_attribute(attr)
if respond_to?(attr)
send(attr)
else
object.read_attribute_for_serialization(attr)
end
end
There was a problem hiding this comment.
I'd suggest raising an error when the id attribute is missing since in some cases the id used to build the URL isn't the resource's original id but a slug maybe.
|
Superb job here. I'm really glad this was done. Could you:
|
| @@ -172,6 +180,8 @@ def attributes(options = {}) | |||
| self.class._attributes.dup | |||
| end | |||
|
|
|||
| attributes += options[:required_fields] if options[:required_fields] | |||
There was a problem hiding this comment.
Just noticed that this could lead to duplicates, I'll fix it.
|
@kurko ping. Looks like changelog and readme were updated. |
|
@mateomurphy can you rebase? |
Better reflect generated output
Although spec is agnostic about inflection rules, examples given are plural
Indicate support for RC3 of JSON API
mateomurphy
force-pushed
the
jsonapi-format-updates
branch
from
b04efe3 to
b695180
Compare
|
@joshsmith Yep, done |
| @@ -16,7 +16,7 @@ def initialize(serializer, options = {}) | |||
| end | |||
|
|
|||
| def serializable_hash(options = {}) | |||
| @root = (@options[:root] || serializer.json_key.to_s.pluralize).to_sym | |||
| @root = :data | |||
There was a problem hiding this comment.
As this fixed by standard whould be better to move that into initialize method?
There was a problem hiding this comment.
Actually, there's no reason to store the root, as it's not used elsewhere
Use the same base class we use for other test models
| @@ -164,6 +164,14 @@ def json_key | |||
| end | |||
| end | |||
|
|
|||
| def id | |||
| object.id if object | |||
There was a problem hiding this comment.
The JSON API spec requires resource id to be a string.
There was a problem hiding this comment.
It's converted to a string in the adapter (https://github.com/rails-api/active_model_serializers/pull/853/files#diff-ae1e38e96a8bf28ae37918a65c448c73R89)
|
LGTM. HERE WE GO! |
|
Reference #829 |
|
I found some bugs right after you merged this. I fixed them here: #858 |
dataroot,included, and linkages)