bug: `id` is set to exclude attribute in deserialization and still included as `undefined`

[diosney]

I've setup:

export const defaultSerializerOptions = {
  id                    : 'id',
  blacklist             : [
    'type'
  ],
  blacklistOnDeserialize: [
    'id',
    'type',
    'createdAt',
    'updatedAt'
  ]
};

But still on deserialization is giving me id = undefined since I'm not sending it, though I want to exclude it completely from deserialization.

[stefanvanherwijnen]

Correct me if I'm wrong, but I think id = undefined and not setting id is the same (although explicitly and implicitly). Setting the id to undefined makes more sense than not setting it at all, because the id is explicitly undefined in this case.

[diosney]

@stefanvanherwijnen I can understand your reasoning behind letting the id value as undefined, but to me, in this case it is a deviation of the blacklist meaning and its effect on the fields at the attributes key, in which case are filtered, not set to undefined, as expected.

If an attribute is blacklisted, doesn't it mean it should completely be erased from output? Because if it is not the case, it will force users to add an extra util function or a replacement to correctly filter the intended blacklisted attributes, as I had to do.

For example, in my case, setting id=undefined does not prevent id to appear later in an Object.keys output, needed for me to further validate the serialization/deserialization output.

If a full exclusion wasn't the intended outcome with the blacklist feature, maybe can you add an extra excluded parameter similar to blacklist but that will completely filter the attributes from output? That will for sure solve the misinterpretation and my use case.

Thanks.

[stefanvanherwijnen]

The problem is that id is not an attribute'. I understand your problem, but this should be easily fixed with result.filter(item => item !== undefined)?

The ID and attributes are handled differently in the source code, so applying a attribute blacklist to the ID will be more complicated than just filtering the result.

[diosney]

Please, bear with be for pushing on this, I just think that this has to be baked in the module instead of being in user ground.

All this time since I added the issue I've been using a simple filtering solution like the one you stated, I'm only discussing here that the blacklist feature should be global to all properties of the final object, including type and id, since they are use cases for excluding them.

Maybe this can be a separate boolean flag to include or not undefined attributes? or a separate boolean flag to control top-level properties behavior? or even exclude them later on the final object.

I do understand that type and id are required properties in the JSONAPI standard, but IMHO that doesn't mean that a de/serializer has to comply with that since we are not dealing here with pure JSONAPI objects, but user defined objects to be serialized and deserialized into.

Hope this makes sense and you understand me, again, sorry for pushing on this, but makes perfect sense to me to have it baked in.

Nevertheless, if you still think that this has to be on user land, ok, we can close the issue and continue just like we were now :)

[stefanvanherwijnen]

Actually, you are right and this has nothing to do with id: undefined (which I should have realized earlier). For the id, the blacklist is not considered at all. So if you would provide an id, you would end up with the id in the deserialized data instead of undefined.

This is how the id is handled:

json-api-serializer/lib/JSONAPISerializer.js

Line 380 in e346ab5

deserializedData[options.id] = data.id || undefined;

As you can see a few lines beneath it, the attributes are neatly picked and omitted according to the blacklist. To blacklist/whitelist the id, you could do something like this:

    if (options.whitelistOnDeserialize.length) {
      if (options.whitelistOnDeserialize.includes('id')) {
        deserializedData[options.id] = data.id || undefined;
      }
    } else if (options.blacklistOnDeserialize) {
      if (!options.blacklistOnDeserialize.includes('id')) {
        deserializedData[options.id] = data.id || undefined;
      }
    }

This works, but it's not exactly a neat solution 🙄 .

So, unless @danivek intentionally designed it such that blacklist only works on attributes (as the documentation says: An array of blacklisted attributes), I agree this is a bug. But the question is how to solve it neatly.

[diosney]

Awesome! Well, we just need to wait until @danivek states his opinion on the approach for its implementation (if he agrees).

In the meantime, I will continue filtering after deserialization logic.

Thanks @stefanvanherwijnen!

[danivek]

@diosney what about having just a check on id like this:

if (data.id !== undefined) {
deserializedData[options.id] = data.id;
}

instead of:

json-api-serializer/lib/JSONAPISerializer.js

Line 380 in e346ab5

deserializedData[options.id] = data.id || undefined;

So if you would not provide an id, you would not end up with the id in the deserialized data instead of undefined.

[diosney]

@danivek What I was really asking for is for a way to enforce a blacklist/filtering of id and type on deserialization, just like it is done for attributes, so no matter if they were specified or not, I just wanted the library to take care of that for me, just like it does for the attributes, that doesn't matter if they are present or not, in the output they won't be there if I blacklisted them.

Thanks

[medfreeman]

@diosney what about having just a check on id like this:

if (data.id !== undefined) {
deserializedData[options.id] = data.id;
}

instead of:

json-api-serializer/lib/JSONAPISerializer.js

Line 380 in e346ab5

deserializedData[options.id] = data.id || undefined;

So if you would not provide an id, you would not end up with the id in the deserialized data instead of undefined.

@danivek Would you be ok w/ this specific fix? I have it as a patch in my codebase for a long time.

[danivek]

@medfreeman I'm ok with this fix

[diosney]

@medfreeman It is something, but is not what I ultimately expect, sorry. If I blacklist it, it mustn't be in the final output, IMHO.

[medfreeman]

Do we all agree that the id property should be absent from the output anyway?

@diosney i agree with you about the feature you're requesting, but i think these can be considered as two separate issues.

I'll then submit a PR that won't close your issue though.