Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty relationships returned as data: null. #41

Closed
wesselvdv opened this issue Jan 11, 2018 · 11 comments
Closed

Empty relationships returned as data: null. #41

wesselvdv opened this issue Jan 11, 2018 · 11 comments
Assignees
Labels

Comments

@wesselvdv
Copy link

wesselvdv commented Jan 11, 2018

The 'solution' for issue #38 seems to have introduced a breaking change in json-api-serializer, and in my opinion is making sure that json-api-serializer does not follow the json:api spec. It also makes sure that the problem faced in #25 is now caused by json-api-serializer, and no longer by improper serialisation to JSON.

The issue:

const JSONAPISerializer = require('json-api-serializer');
const Serializer = new JSONAPISerializer();

// Schools type
Serializer.register('schools', {
  relationships: {
    students: {
      type: 'students',
      links: {
        self: '/students'
      }
    }
  }
});

// Students type
Serializer.register('students', {
  relationships: {
    grades: {
      type: 'grades',
      links: {
        self: '/grades'
      }
    }
  }
});

Serializer.register('grades');

// Input data for schools
const input = {
  "id": "1",
  "name": "School Name",
  "students": {
    id: 1,
  },
};

const output = Serializer.serialize('schools', input);
console.log(JSON.stringify(output));

output:

{
  "jsonapi": {
    "version": "1.0"
  },
  "data": {
    "type": "schools",
    "id": "1",
    "attributes": {
      "name": "School Name"
    },
    "relationships": {
      "students": {
        "links": {
          "self": "/students"
        },
        "data": {
          "type": "students",
          "id": "1"
        }
      }
    }
  },
  "included": [
    {
      "type": "students",
      "id": "1",
      "attributes": {},
      "relationships": {
        "grades": {
          "links": {
            "self": "/grades"
          },
          "data": null
        }
      }
    }
  ]
}

Explanation:
I know you think you're following the spec, but look at what it says:
screen shot 2018-01-11 at 15 08 40

It clearly states that ONE of those three has to be present. Not ALL three with a value of null. This literally broke my API.

My suggestion is to following the common convention regarding missing relationships. Keep data on undefined, so it'll be filtered out on stringify. Make sure that links are included for lazy loading, and if there's metadata present output that as well. If the relationship IS present, but has a value of null then it should be included with data is null.

I am downgrading to v1.9.1 where this change hasn't been included.

@danivek
Copy link
Owner

danivek commented Jan 12, 2018

@wesselvdv, sorry for having breaking something... I agree with you that json-api-serializer must also follow the rule you mention.

To clarify all cases:

1. If relationship is present and is null it should output with data: null

const JSONAPISerializer = require('json-api-serializer');
const Serializer = new JSONAPISerializer();

// Schools type
Serializer.register('schools', {
  relationships: {
    students: {
      type: 'students',
      links: {
        self: '/students'
      }
    }
  }
});

// Students type
Serializer.register('students', {
  relationships: {
    grades: {
      type: 'grades',
      links: {
        self: '/grades'
      }
    }
  }
});

Serializer.register('grades');

// Input data for schools
const input = {
  "id": "1",
  "name": "School Name",
  "students": {
    id: 1,
  },
  "grades": null
};

const output = Serializer.serialize('schools', input);
console.log(JSON.stringify(output));

Output should have data: null for grades relationship:

{
  "jsonapi": {
    "version": "1.0"
  },
  "data": {
    "type": "schools",
    "id": "1",
    "attributes": {
      "name": "School Name"
    },
    "relationships": {
      "students": {
        "links": {
          "self": "/students"
        },
        "data": {
          "type": "students",
          "id": "1"
        }
      }
    }
  },
  "included": [
    {
      "type": "students",
      "id": "1",
      "attributes": {},
      "relationships": {
        "grades": {
          "links": {
            "self": "/grades"
          },
          "data": null
        }
      }
    }
  ]
}

2. Considering I revert to data:undefined if relationship is missing and it have at least one of the following links or meta property, it should output with data: undefined

const JSONAPISerializer = require('json-api-serializer');
const Serializer = new JSONAPISerializer();

// Schools type
Serializer.register('schools', {
  relationships: {
    students: {
      type: 'students',
      links: {
        self: '/students'
      }
    }
  }
});

// Students type
Serializer.register('students', {
  relationships: {
    grades: {
      type: 'grades',
      links: {
        self: '/grades'
      }
    }
  }
});

Serializer.register('grades');

// Input data for schools
const input = {
  "id": "1",
  "name": "School Name",
  "students": {
    id: 1,
  }
};

const output = Serializer.serialize('schools', input);
console.log(JSON.stringify(output));

Output should not have data for grades relationship:

{
  "jsonapi": {
    "version": "1.0"
  },
  "data": {
    "type": "schools",
    "id": "1",
    "attributes": {
      "name": "School Name"
    },
    "relationships": {
      "students": {
        "links": {
          "self": "/students"
        },
        "data": {
          "type": "students",
          "id": "1"
        }
      }
    }
  },
  "included": [
    {
      "type": "students",
      "id": "1",
      "attributes": {},
      "relationships": {
        "grades": {
          "links": {
            "self": "/grades"
          }
        }
      }
    }
  ]
}

3. Considering I revert to data: undefined If relationship is missing and NOT have at least one of the following links or meta property

const JSONAPISerializer = require('json-api-serializer');
const Serializer = new JSONAPISerializer();

// Schools type
Serializer.register('schools', {
  relationships: {
    students: {
      type: 'students',
      links: {
        self: '/students'
      }
    }
  }
});

// Students type
Serializer.register('students', {
  relationships: {
    grades: {
      type: 'grades'
    }
  }
});

Serializer.register('grades');

// Input data for schools
const input = {
  "id": "1",
  "name": "School Name",
  "students": {
    id: 1,
  }
};

Output will have empty object for grades relationship

{
  "jsonapi": {
    "version": "1.0"
  },
  "data": {
    "type": "schools",
    "id": "1",
    "attributes": {
      "name": "School Name"
    },
    "relationships": {
      "students": {
        "links": {
          "self": "/students"
        },
        "data": {
          "type": "students",
          "id": "1"
        }
      }
    }
  },
  "included": [
    {
      "type": "students",
      "id": "1",
      "attributes": {},
      "relationships": {
        "grades": {}
      }
    }
  ]
}

This case is not clear regarding the spec. I don't think "relationships": { "grades": {} }" is a valid output, because it break the following rule:

relationship object” MUST contain at least one of the following:

  • links: ...
  • data: ...
  • meta: ...

Is grades relationship object should be filtered out from relationships object in such case ?
Is grades relationship object should have at least data with null ?

@Mattii any ideas ?

@wesselvdv wesselvdv reopened this Jan 12, 2018
@wesselvdv
Copy link
Author

@danivek I agree with the first two cases. Those appear to correspond to the json:api spec, and will make sure that the behaviour introduced by #38 is reverted.

Is grades relationship object should be filtered out from relationships object in such case ?

I wouldn't introduce this behaviour, this will cause a lot of confusion if objects just magically start disappearing.

Is grades relationship object should have at least data with null ?

I wouldn't do this either, this will cause the same situation as we have now, if I understand you correctly.

I suggest to introduce the following behaviour if a defined relationship is missing in an object that's serialised:

  • If json-api-serializer can deduce the links object, or if it has been defined by the user in the type definition, and the relationships object is missing.
    • Include the links object, make sure data is undefined, and if applicable include meta.
  • If json-api-serializer can NOT deduce the links object, the relationships object is missing and no meta has been defined.
    • Throw an exception stating that no spec compliant json:api object can be created.
  • If json-api-serializer can NOT deduce the links object, the relationships object is missing, but meta data has been defined.
    • Include the meta object and make sure data & links are undefined.

Unless I am missing something, the above should make sure that json-api-serializer always creates objects that are compliant.

@danivek danivek self-assigned this Jan 15, 2018
@danivek danivek added the bug label Jan 15, 2018
@wesselvdv
Copy link
Author

@danivek thanks for the quick fix!!

@alechirsch
Copy link
Contributor

@danivek I am seeing results that contradicts the second case in your previous comment.

I registered a type with about 15 relationships.

I fetched data without including any relationships, the resulting json includes all 15 relationships as

{
   data: null
}

I would expect that if my data does not have any relationships defined, it should not attempt to serialize them even if they are registered with my type.

@alechirsch
Copy link
Contributor

@danivek, thoughts?

@danivek
Copy link
Owner

danivek commented Aug 20, 2019

@alechirsch, I think it breaks JSON:API specification https://jsonapi.org/format/#document-resource-object-linkage

Resource linkage MUST be represented as one of the following:
null for empty to-one relationships.
an empty array ([]) for empty to-many relationships.

What your input data looks like ?

@alechirsch
Copy link
Contributor

According to this, it should NOT include related resources that were not requested

https://jsonapi.org/format/#fetching-includes

If an endpoint supports the include parameter and a client supplies it, the server MUST NOT include unrequested resource objects in the included section of the compound document.

I think #35 might be related to this.

In my case, I am requesting 1 relationship in the include part of the my query. The data fetches one relationship and then it serializes ALL relationships that are registered with that type, even though the user does not request them.

@danivek
Copy link
Owner

danivek commented Aug 20, 2019

What your input data looks like ?

json-api-serializer tries to detect if a relationship has been populated or not by checking if it is a plain object. In this case, it tries to serialize the relation and set it in included, otherwise it considers that it is an array of identifier or simply an identifier and set typeand id in the relationship data of a resource.

@alechirsch
Copy link
Contributor

alechirsch commented Aug 20, 2019

Here is an example of what I am talking about:

it('should not serialize relationships that were not provided', function(done) {
      const Serializer = new JSONAPISerializer();
      Serializer.register('people', {});
      Serializer.register('article', {
        relationships: {
          author: {
            type: 'people'
          }
        }
      });
  
      const data = {
        id: '1',
        title: 'Nice article'
      };
  
      const serialized = Serializer.serialize('article', data);
      console.log(serialized);
      
      expect(serialized.data).to.have.property('id');
      expect(serialized.data.attributes).to.have.property('title');
      expect(serialized.data.relationships).to.not.have.property('author');
      done();
    });

This produces:

{
  jsonapi: { version: '1.0' },
  meta: undefined,
  links: undefined,
  data: { type: 'article',
    id: '1',
    attributes: { title: 'Nice article' },
    relationships: { author: { data: null } },
    meta: undefined,
    links: undefined
  },
  included: undefined
}

If data has an empty object called author, then I would expect the above to be the result.
I expected this as the result

{
  jsonapi: { version: '1.0' },
  meta: undefined,
  links: undefined,
  data: { type: 'article',
    id: '1',
    attributes: { title: 'Nice article' },
    relationships: {},
    meta: undefined,
    links: undefined
  },
  included: undefined
}

Also this statement mentions empty relationships, not necessarily undefined relationships:

Resource linkage MUST be represented as one of the following:
null for empty to-one relationships.
an empty array ([]) for empty to-many relationships.

@alechirsch
Copy link
Contributor

I would expect the code to be like this, it still produces null or empty array relationships IF the relationships are defined as empty in the data object provided.

// Line 627
if (
        serializeRelationship.data !== undefined ||
        serializeRelationship.links !== undefined ||
        serializeRelationship.meta !== undefined
      ) {
        // Convert case
        relationship = options.convertCase
          ? this._convertCase(relationship, options.convertCase)
          : relationship;
  
        serializedRelationships[relationship] = serializeRelationship;
      }

As opposed to this:

      if (
        serializeRelationship.data === undefined &&
        serializeRelationship.links === undefined &&
        serializeRelationship.meta === undefined
      ) {
        serializeRelationship = {
          data: null
        };
      }

      // Convert case
      relationship = options.convertCase
        ? this._convertCase(relationship, options.convertCase)
        : relationship;

      serializedRelationships[relationship] = serializeRelationship;

@danivek danivek reopened this Aug 21, 2019
@danivek
Copy link
Owner

danivek commented Aug 21, 2019

@alechirsch You're right, regarding the spec there's no reason for a missing relationship to be serialized as null

alechirsch pushed a commit to alechirsch/json-api-serializer that referenced this issue Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants