Support deeply nested relations when building template

[ralfschimmel]

Currently the serialiser only supports 2-deep relations. This PR adds support for unlimited nesting of relations. The data is already in the JSON since the toJSON in the util class in recursive. This PR makes the serialiser also recursive.

[jamesdixon]

@ralfschimmel when I manually merge this, I'm getting two test failures. Did you run the test suite (npm test)?

[ralfschimmel]

@jamesdixon Mhh the diff is also strange after the rebase, I'll make a new PR en fix/run the tests beforehand :)

[jamesdixon]

Thanks @ralfschimmel!

[ralfschimmel]

@jamesdixon fixed, had a little bug in the recursing call. Also cleaned up the commit list.

[jamesdixon]

Thanks, @ralfschimmel!

@chamini2 @ShadowManu any comments on this?

[chamini2]

Hey @jamesdixon, thanks for that call to review!

It seems to be OK, I just added a small comment on the function type.

Also, should we be testing this with a spec?

[jamesdixon]

@chamini2 of course! Yes, should be testing. I believe we may already have a test that covers this -- I'd have to look.

Regarding your comment, I'm not a TS expert, but coming from other languages with void, this would appear to make sense.

[chamini2]

I don't think there are tests for the particular behaviour this PR is adding, they would have been failing 😄! Since @ralfschimmel added functionality for deeply nested relations, we should test with deeply (3 or more levels of) nested relations.

[jamesdixon]

True! That said, we should just need to add another level as you recommended to the existing test. @ralfschimmel can you make this modification to the test as well the minor modification suggested by @chamini2?

@chamini2 while you're at it, would you mind reviewing the two PRs I submitted last night? Would like to get everything merged before doing a release. Appreciate it!

[jamesdixon]

@chamini2 I believe the test that you added to cover multiple levels of relationships covers both this case as well as your change to toJSON since this code precedes the call to toJSON.

@ralfschimmel thanks for the contribution!

[ralfschimmel]

I was working on a test and noticed the following output of a deeply nested structure;

let model1: Model = bookshelf.Model.forge<any>({id: '5', attr: 'value'});
      let model2: Model = bookshelf.Model.forge<any>({id: '6', attr: 'value'});
      let model3: Model = bookshelf.Model.forge<any>({id: '7', attr: 'value'});
      let model4: Model = bookshelf.Model.forge<any>({id: '8', attr: 'value'});
      let model5: Model = bookshelf.Model.forge<any>({id: '9', attr: 'value'});
      let model6: Model = bookshelf.Model.forge<any>({id: '10', attr: 'value'});

      (<any> model1).relations['related-model'] = model2;
      (<any> model2).relations['nested-related-model'] = model3;
      (<any> model3).relations['nested-related-model'] = model4;
      (<any> model4).relations['nested-related-model'] = model5;
      (<any> model5).relations['nested-related-model'] = model6;

      let result: any = mapper.map(model1, 'models');

{ links: { self: 'https://domain.com/models' },
  included:
   [ { type: 'nested-related-models',
       id: '10',
       attributes: { attr: 'value' },
       links: { self: 'https://domain.com/nested-related-models/10' } },
     { type: 'nested-related-models',
       id: '9',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '10' },
             links:
              { self: 'https://domain.com/nested-related-models/5/relationships/nested-related-model',
                related: 'https://domain.com/nested-related-models/5/nested-related-model' } } },
       links: { self: 'https://domain.com/nested-related-models/9' } },
     { type: 'nested-related-models',
       id: '8',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '9' },
             links:
              { self: 'https://domain.com/nested-related-models/5/relationships/nested-related-model',
                related: 'https://domain.com/nested-related-models/5/nested-related-model' } } },
       links: { self: 'https://domain.com/nested-related-models/8' } },
     { type: 'nested-related-models',
       id: '7',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '8' },
             links:
              { self: 'https://domain.com/nested-related-models/5/relationships/nested-related-model',
                related: 'https://domain.com/nested-related-models/5/nested-related-model' } } },
       links: { self: 'https://domain.com/nested-related-models/7' } },
     { type: 'related-models',
       id: '6',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '7' },
             links:
              { self: 'https://domain.com/related-models/6/relationships/nested-related-model',
                related: 'https://domain.com/related-models/6/nested-related-model' } } },
       links: { self: 'https://domain.com/related-models/6' } } ],
  data:
   { type: 'models',
     id: '5',
     links: { self: 'https://domain.com/models/5' },
     attributes: { attr: 'value' },
     relationships:
      { 'related-model':
         { data: { type: 'related-models', id: '6' },
           links:
            { self: 'https://domain.com/models/5/relationships/related-model',
              related: 'https://domain.com/models/5/related-model' } } } } }

Should't the self and related links contain other id's? I think we need some fix in utils.buildRelation()?

[chamini2]

@jamesdixon, then why were the tests passing? I honestly don't understand 😞.

Maybe we need to replicate a test like the one I added testing specifically this function? Or expand the one I added to expect things of this function too?

[ralfschimmel]

One-deep seems to go fine which is wat is tested, but deeper is not.

I think my code needs to be smarter about the baseUrl, it is not building up recursively. Do we want it to build up when nested deeply? Is that JSONApi spec?

Or just like this?

https://domain.com/nested-related-models/6/relationships/nested-related-model https://domain.com/nested-related-models/7/relationships/nested-related-model https://domain.com/nested-related-models/8/relationships/nested-related-model https://domain.com/nested-related-models/9/relationships/nested-related-model

[chamini2]

Should be returning this right? (marked changes with # at the beginning of line)

{ links: { self: 'https://domain.com/models' },
  included:
   [ { type: 'nested-related-models',
       id: '10',
       attributes: { attr: 'value' },
       links: { self: 'https://domain.com/nested-related-models/10' } },
     { type: 'nested-related-models',
       id: '9',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '10' },
             links:
#             { self: 'https://domain.com/nested-related-models/9/relationships/nested-related-model',
#               related: 'https://domain.com/nested-related-models/9/nested-related-model' } } },
       links: { self: 'https://domain.com/nested-related-models/9' } },
     { type: 'nested-related-models',
       id: '8',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '9' },
             links:
#             { self: 'https://domain.com/nested-related-models/8/relationships/nested-related-model',
#               related: 'https://domain.com/nested-related-models/8/nested-related-model' } } },
       links: { self: 'https://domain.com/nested-related-models/8' } },
     { type: 'nested-related-models',
       id: '7',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '8' },
             links:
#             { self: 'https://domain.com/nested-related-models/7/relationships/nested-related-model',
#               related: 'https://domain.com/nested-related-models/7/nested-related-model' } } },
       links: { self: 'https://domain.com/nested-related-models/7' } },
     { type: 'related-models',
       id: '6',
       attributes: { attr: 'value' },
       relationships:
        { 'nested-related-model':
           { data: { type: 'nested-related-models', id: '7' },
             links:
              { self: 'https://domain.com/related-models/6/relationships/nested-related-model',
                related: 'https://domain.com/related-models/6/nested-related-model' } } },
       links: { self: 'https://domain.com/related-models/6' } } ],
  data:
   { type: 'models',
     id: '5',
     links: { self: 'https://domain.com/models/5' },
     attributes: { attr: 'value' },
     relationships:
      { 'related-model':
         { data: { type: 'related-models', id: '6' },
           links:
            { self: 'https://domain.com/models/5/relationships/related-model',
              related: 'https://domain.com/models/5/related-model' } } } } }

[ralfschimmel]

Ok, that should be fixable :)

[ralfschimmel]

Mhh, my first TS code :)

I'm not really understanding what is happening in links.buildSelf, where is the current and parent coming from? It is not in the arguments? I can just add it to the arguments but don't know what is the desired implementation.

[jamesdixon]

@jamesdixon, then why were the tests passing? I honestly don't understand 😞.

Maybe we need to replicate a test like the one I added testing specifically this function? Or expand the one I added to expect things of this function too?

I believe you are right. It was late and I was thinking that the test you had already covered the scenario, which it doesn't. I merged this prematurely.

Although, I am a bit confused as to why the test would be passing. Your reimplementation of Utils.toJSON() uses recursion to correctly output the JSON representation of the Model. @ralfschimmel's implementation of recursion in the mapper itself should output the correct template for which to apply to the JSON data, resulting in the proper JSON API representation. If the output of either of those functions is incorrect, then the resulting JSON API representation should also be wrong.

That said, are we sure that the tests are in fact correct? And does it make sense to unit test those functions and their output individually rather than just testing the output of the mapper? It may not matter given the relationship of those two functions, but throwing it out there.

[jamesdixon]

@ralfschimmel yes, regardless, links should only be 1 deep. The spec doesn't define anything deeper than that and frankly, it would just get out of control.

With regards to parent and current, those are params passed in by the serializer itself. The serializer allows us to define links as functions so that they can be dynamically be created while being serialized. Take a look at the tests: https://github.com/SeyZ/jsonapi-serializer

[chamini2]

Should we open a new issue to not forget to fix this?

[jamesdixon]

@chamini2 yes please