mysidewalk / jsonapi-parse

MIT License
13 stars 6 forks source link

Missing `type` property from parsed objects #2

Closed adborden closed 7 years ago

adborden commented 7 years ago

After parsing, I'm missing the type property on my entities. I'm wondering if all the non-attributes should be attached at something like meta so they're not completely lost, but don't collide with the attributes.

e.g.

{
  "data": [
    {
      "id": 1,
      "type": "person",
      "links": {},
      "attributes": {
        "name": "fred"
      }
    }
  ]
}

becomes

{
  "data": [
    {
      "id": 1,
      "name": "fred",
      "meta": {
        "type": "person",
        "links": {}
       }
    },
  ]
}
pjfreeze commented 7 years ago

Hey -

Yeah, I can see how that would be helpful to keep the metadata around.

I have two concerns about this:

  1. It doesn't conform to the JSON API specification (as far as I understand it).
  2. Any other person/organization consuming this, who also has a model/record with a meta field would have their actual attribute overwritten by this library. I think that could create some pretty unexpected behavior as well as hard to understand bugs.

In an effort to keep this as "simple" as possible, I think keeping to the JSON API spec is the best current path.

Perhaps this idea is something that could be rolled into a suggestion for the JSON API spec itself, or a fork of this which provides that functionality for you or others who are also interested.

Another possible option is to include the type value in the attributes object as well. It should come through unless type is a property you already use for something else.

adborden commented 7 years ago

Thanks for the reply. I'm not sure what you mean about 1. Is the parsed output supposed to conform to the JSON API spec? In your README example, the output doesn't conform to the spec, does it?

For 2, I completely agree, but I'm not sure there's a way to get around it for any parse/transform solution. If you have an attribute id in your entity (in addition to the id JSON API provides), you'll have a collision as it works today. To workaround, we could make the meta property name configurable.

I'm happy to provide a PR.

adborden commented 7 years ago

This might be a separate issue, but related. The links attribute on the repsonse is also lost. This means if you're doing pagination, where you might need to grab links.next or links.prev or any related links, you'll have to grab them before parsing them (I can open a new issue for this if you'd prefer).

pjfreeze commented 7 years ago

It turns out I'm flat out wrong about the type property part. The spec, as I read it, says you shouldn't have any relationship or attribute that use the type property. It also mentions the same thing for the id property.

If the consumer is following the specification for serialization, it should be safe to always set the type and id properties on the resulting object. AKA we shouldn't need to add a meta object on the record to include type.

It would be great if you'd open another issue for the links bug to help separate changes.

This issue can handle including the type property on each record, and the new one can handle fixing links.

Hopefully I can work on fixing this in the next few days, or maybe this weekend.

(When you create the new issue could you clarify when the links property is being lost. E.g. are you seeing it lost when at the top-level of a document or within a specific record?)

adborden commented 7 years ago

Ah, I see. I didn't even realize meta was part of the spec, so my option wouldn't have worked either.

Unfortunately, the API that I'm dealing with breaks the spec in that it does include an id and type in the attributes, so they get globbered by the resource's id and type... after parsing.

pjfreeze commented 7 years ago

Oh weird; do they represent different id and type values?