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

Error thrown when relationship is not registered #129

Closed
Istanful opened this issue Aug 30, 2021 · 5 comments · Fixed by #130
Closed

Error thrown when relationship is not registered #129

Istanful opened this issue Aug 30, 2021 · 5 comments · Fixed by #130

Comments

@Istanful
Copy link
Contributor

Description

When a resource is deserialized and the included relationship is not registered an error will be thrown. The error says Cannot read property 'schema' of undefined, which does not help a whole lot to troubleshoot.

To help the user my suggestion is to throw an error saying:

No relationship named [relationshipKey] registered on [type]

This would break backwards compability though, since some tests will fail.

What is the desired behavior here? I'm happy to patch it given how you want it to work.

Here's a test to reproduce:

    it('should throw an error when relationship is not registered', (done) => {
      const Serializer = new JSONAPISerializer();
      Serializer.register('article');
      Serializer.register('people');

      const data = {
        data: {
          id: '1',
          type: 'article',
          relationships: {
            author: {
              data: {
                type: 'people',
                id: '1'
              }
            }
          }
        },
        included: [
          {
            type: 'people',
            id: '1'
          }
        ]
      };

      expect(function () {
        Serializer.deserialize('article', data);
      }).to.throw(Error, 'No relationship named author registered on article');
      done();
    });
@danivek
Copy link
Owner

danivek commented Sep 14, 2021

Should be fixed by #126

@danivek danivek closed this as completed Sep 14, 2021
@Istanful
Copy link
Contributor Author

@danivek I don't think so. In my example the author relationship is not registrered. In his example it is. I haven't looked into the code just now, but I do remember several tests failing when I created a similar fix.

Can you reopen the issue?

As I mentioned I'm happy to create a fix, given how you want it to work.

@danivek
Copy link
Owner

danivek commented Sep 16, 2021

Edit: @Istanful I tried with your unit test and now with master/2.6.4, the error returned is now No type registered for people.

You are right, in your case another error should be thrown, indicating that the relationship has not been defined

I reopen the issue.

@danivek danivek reopened this Sep 16, 2021
@danivek
Copy link
Owner

danivek commented Sep 16, 2021

@Istanful To avoid breaking things actually, what about not throwing error and trying to deserialize with the given type if it is registered ? The same behavior as if there is no included property.

@Istanful
Copy link
Contributor Author

@danivek Sure. If you think that's the more correct behaviour then I think avoiding breakage is a good idea. 👍 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants