devour-client icon indicating copy to clipboard operation
devour-client copied to clipboard

Serialization of Document and Resource level meta and link objects

Open randym opened this issue 9 years ago • 12 comments

The Challenge

The specification states that both the top level document and any resource may contain meta and links members. However, the deserialization here will nuke any resource specified meta or links members via the top level document's data attribute when deserializing a single resource. (e.g. jsonApi.get('posts', 1))

example 1:

data: {
  attributes:{
    someAttribute: 'a'
  },
  type: 'posts',
  id: '1'
  meta: {
    postMeta: 'some meta data for this individual post'
  }
},
meta: {
  docMeta: 'document meta'
}

Note how both the document and resource have meta members.

As of this PR: https://github.com/twg/devour/pull/24 resources are deserialized with an understanding of meta and links members. This works fine when handing a collection of resources or resources in included.

However, when defining meta data on a single resource response as shown in example 1 above, the res-deserialize middleware will replace any resource level meta data with top level meta, or undefined no top level meta data is specified.

https://github.com/twg/devour/blob/master/src/middleware/json-api/res-deserialize.js#L35

The payload from example 1 above deserializes to the following object:

{
  id: '1',
  someAttribute: 'a',
  meta: {
    docMeta: 'document meta'
  }
}

Note how the resource level meta is replaced with the document level meta data.

example 2

data: {
  attributes:{
    someAttribute: 'a'
  },
  type: 'posts',
  id: '1'
  meta: {
    postMeta: 'some meta data for this individual post'
  }
}

The payload for example 2 deserialized to the following object:

{
  id: '1',
  someAttribute: 'a'
  meta: undefined
}

The examples above also apply to resource/document level links members.

Possible Solutions?

Change the deserialized data structure

This is a non-backwards compatible change. While it does provide an arguably more accurate data structure I would be cautious of forcing existing users to rewrite their applications.

{
  resource: {
    attributes: {},
    meta: {},
    links: {}
  },
  meta: {},
  links: {}
}

collections would continue to add meta and links attributes to the array of resources.

Use docMeta and docLinks attributes on the deserialized object

This allows backwards compatibility and help to highlight that there are document level meta and links members.

{
  attributes: {},
  meta: {},
  links: {}
  docMeta: {},
  docLinks: {}
}

collections would add docMeta and docLinks attributes to the array of resources.

Merge the meta/links for single resource documents.

Too risky as there could easily be meta or link members with identical names.

I'm currently working off a fork that utilizes docMeta and docLinks, but before considering a PR I'd like to get some feedback or alternative proposals from the maintainers.

randym avatar May 16 '16 07:05 randym

Sorry for the delay responding to this – really appreciate the well thought out issue. We're seeing some problems around this as well (somewhat related to #27).

I'm wondering if we could do something like:


/*
*   `meta` contains the document wide meta...
*/
jsonApi.all('posts').then((posts, meta)=> {

})

/*
*   Meta on individual resources is deserialized into a `_meta` property...
*/
{
  id: '5',
  title: 'Some Post Title',
  _meta: {
    links: {...},
    page: {...}
  } 
}

I'm not necessarily opposed to the notion of docMeta – would love to hear feedback from some other folks.

Emerson avatar May 26 '16 16:05 Emerson

I've been using docMeta in our application for the time being and it certainly solves the problem, however having named attributes on an array definitely feels weird to me as a developer.

I think keeping JSONAPI's wrapper object around responses is another avenue that could be explored:

jsonApi.findAll('posts').then((doc) => {
  // doc => {
  //   data: [ ... ],
  //   meta: { ... },
  //   links: { ... }
  // }
})

jsonApi.findOne('posts', 1).then((doc) => {
  // doc => {
  //   data: { ... },
  //   meta: { ... },
  //   links: { ... }
  // }
})

It avoids renaming JSON-API fields completely, supports resource-level and document-level meta/links etc. with the expected names.

The big downside is backward compatibility and verbosity - it's a fairly significant design change from what devour currently does.

glsignal avatar May 27 '16 05:05 glsignal

@glsignal – Love this in some ways, but also agree about the verbosity. It's so nice to just get back a simple array of flattened objects.

We're running into issues with attaching named attributes on arrays. Just simple stuff, like when you clone an object it will clear the named attributes.

Emerson avatar May 27 '16 14:05 Emerson

Hi, guys! What is the status of this issue? I have similar problems, but with relationship members. The payload that I get from the server looks something like this:

{
  "data": {
    "type": "post",
    "id": "1",
    "attributes": {
      // ... this post's attributes
    },
    "relationships": {
      "comments": {
          "links": {
            "self": "http://example.com/post/1/relationships/comments"
          }
        }
    }
  }
}

Any ideas how to include comments links in deserialized object?

isBatak avatar Jun 15 '16 21:06 isBatak

Still haven't made a decision on this yet, I'll start giving it some thought this week. Tough because we ideally don't want to break the existing API.

Emerson avatar Jun 20 '16 14:06 Emerson

I've been casually thinking about this one for a while since first seeing it.

This approach jsonApi.all('posts').then(({ posts, meta, links }) => { ... - adding additional arguments for the very top level meta, links, etc. seems like the best option in here at the moment.

Taking it further, if we're shooting for full spec coverage, in what way might it make sense to add support for other possible top level fields like errors, jsonapi? (spec)

glsignal avatar Jun 23 '16 04:06 glsignal

Are people still using devour or have you all moved onto another jsonapi library? @Emerson any update on a decision moving forward for serializing meta data using devour?

I'd like to continue using devour, so my workaround for now to leverage the serializer for something like a jsonApi.update is to put the meta data in the attributes for now and let the backend handle it from there but it feels like a hack. I'd like to have it on the same level as it is in the spec.

danwarner avatar Jul 25 '17 20:07 danwarner

I think people are for sure still using it – just had another PR merged and released today. I've moved jobs since I wrote this library and we're all in on Ember, so I haven't really needed it. I've just been keeping an eye on PR's and merging the ones that seam reasonable.

It's hard for me to make a definitive call on something when I'm not actually dog fooding it. If there was a consensus and pull request here I'd be happy to hit the merge button. As for this specific issue, I'm pretty open as long as it's reasonable.

Note: just looked at the NPM stats:

npm-stats

Emerson avatar Jul 25 '17 20:07 Emerson

Thanks @Emerson I need to implement pagination as defined in the spec:

HTTP/1.1 200 OK
Content-Type: application/vnd.api+json

{
  "meta": {
    "total-pages": 13
  },
  "data": [
    {
      "type": "articles",
      "id": "3",
      "attributes": {
        "title": "JSON API paints my bikeshed!",
        "body": "The shortest article. Ever.",
        "created": "2015-05-22T14:56:29.000Z",
        "updated": "2015-05-22T14:56:28.000Z"
      }
    }
  ],
  "links": {
    "self": "http://example.com/articles?page[number]=3&page[size]=1",
    "first": "http://example.com/articles?page[number]=1&page[size]=1",
    "prev": "http://example.com/articles?page[number]=2&page[size]=1",
    "next": "http://example.com/articles?page[number]=4&page[size]=1",
    "last": "http://example.com/articles?page[number]=13&page[size]=1"
  }
}

It's a blocking problem for me, so I'll fork the repo for now and contribute back what I come up with unless there's another PR around that already solves this problem.

danwarner avatar Aug 07 '17 16:08 danwarner

I've completed the updates to support top-level document errors, meta, and links on my fork of the repo, but I won't submit a PR since it required breaking changes to make it happen. If there's a way I can contribute back to the repo let me know.

danwarner avatar Aug 07 '17 22:08 danwarner

I'm open to a 2.0 release and this is definitely an issue that needed to be addressed. Would love if you could throw up a quick PR so I could take a look.

Emerson avatar Aug 08 '17 14:08 Emerson

Thanks @Emerson I went ahead and submitted a quick PR for you. I look forward to your feedback.

danwarner avatar Aug 08 '17 20:08 danwarner