mysidewalk / jsonapi-parse

MIT License
13 stars 6 forks source link

All `attributes.id` & `attributes.type` will be overridden by `id` & `type` #9

Closed tim-yao closed 6 years ago

tim-yao commented 6 years ago

This is a very useful module but I have a JSONAPI response have data like this:

    "type": "articles",
    "id": "sf4379f1-bd3b-414d-8a17-191e17619d11",
    "attributes": {
      "id": "a_test_article_id"
    },

Then what I get will be:

    "type": "articles",
    "id": "sf4379f1-bd3b-414d-8a17-191e17619d11"

The attributes id is gone. That is caused by flatten function https://github.com/mysidewalk/jsonapi-parse/blob/master/src/jsonapi.js#L171

        return extend(
            {},
            { links: record.links, meta: meta },
            record.attributes,
            { id: record.id, type: record.type }
        );

That extend function merged all values by removing duplicated keys. One question, do we really need to flatten the data object? What is the purpose? In my case, there is a id field inside attributes but I can't get it after flatten.

Or if we have to flatten it, what about add attributes_id & attributes_type if there are such field inside attributes? Simply can be fixed by below:

    function flatten(record, extraMeta) {
        var meta = extend({}, record.meta, extraMeta)

       // Add this
        if (record.attributes.id) {
            record.attributes.attributes_id = record.attributes.id
        }

        if (record.attributes.type) {
            record.attributes.attributes_type = record.attributes.type
        }
       // End

        return extend(
            {},
            { links: record.links, meta: meta },
            record.attributes,
            { id: record.id, type: record.type }
        );
    }

So the attributes.id & attributes.type gets preserved. Thanks.

pjfreeze commented 6 years ago

Hey @tim-yao!

The example you provided makes a lot of sense, and I appreciate you documenting it so thoroughly.

One question, do we really need to flatten the data object? What is the purpose?

My main reason behind writing this module/package was to un-flatten the response from my APIs and and connect relationships of resources to make their object graph easier to navigate. So the purpose of it was to remove the JSONAPI structure from those objects so the JSONAPI specifics were isolated to API interactions instead of in the implementation of the rest of the application.

Or if we have to flatten it, what about add attributes_id & attributes_type if there are such field inside attributes? Simply can be fixed by below:

This module/package is intended to closely follow the JSONAPI specification, so the type and id properties are treated specially. According to the specification, the type and id properties must be unique between the attributes object, and top-level properties. (http://jsonapi.org/format/#document-resource-object-fields).

For that reason, I'm hesitant to implement the solution you proposed because it conflicts from the specification and the specifications rules about those properties.

Another thought about the solution; a resource that includes a property named attribute_id/attribute_type would be overwritten by these changes in the same way your type and id properties are being overwritten. In some ways this would just be pushing the problem down the road.

Rather than create an additional list of "reserved properties names" I think it makes the most sense to stick to the properties (id and type) included in the spec.

I'm happy to talk through other ideas of solutions if you have some. Otherwise, I think in this case a custom fork of this module/package to handle your specific need might be the best option. (Or feel free to correct my interpretation of your issue if I got something wrong!)

tim-yao commented 6 years ago

Hi @pjfreeze Thanks so much for your quick response! The link(http://jsonapi.org/format/#document-resource-object-fields) you gave is make sense to me and I have passed it to our Backend team. So it's not a issue here. I am going to close this issue now.