Loopback-datasource-juggler: Fix use of `as` param in polymorphic relations in json model definition

[ebarault]

Linked

https://github.com/strongloop/loopback-datasource-juggler/issues/1297

Description/Steps to reproduce

While reviewing the doc for polymorphic relations, i found quite a strange peace of code in DataSource.prototype.defineRelations()
as you can see in lib/datasource.js#L486

what it does:
if the JSON relation settings has something like

"someRelation": {
  "model": "someModel",
  "polymorphic": {
    "as": "someRelationName"
  }
}

which is currently advised in the doc here

then in lib/datasource.js#L507
params.model will be replaced by "someRelationName" instead of using the provided model name

and obviously the lookUpModelTo() method in relation-definition.js will throw because it looks to resolve a modelTo name "someRelationName".

The keyword as is meant to name the relations when defining them by code.

Expected

In my quest to review the polymorphic relations doc, i found that the keyword as inside the relation.polymorphic config object has currently multiple different conflicting applications :

• serving as a shortcut to define the prefix for foreignKey and discriminator,
• defining the name of the relation (same role as the top level "as")
• defining the name of the relation's modelTo

As a start to improve things, I propose to discuss removing the logic defined in lib/datasource.js#L485

Note: i also noted that in a general manner it has not much meaning using the as keyword in a model JSON definition as the relation key defines the relation's name, and asshould be reserved when defining relations by code. Use of the as keyword in json model definitions can lead to non-user-friendly errors if not unpredictable results.

@ssh24 : again i made you the reviewer of this, but do not hesitate to appoint another reviewer if required

[ssh24]

@ebarault It looks like the issue is with assigning params.model to the polymorphic relation name instead of the target model name? The code in lib/datasource.js#L507, should then just assign params.model = targetModel; and not care about the _polymorphicName_.

Also, I am wondering if this should be back-ported.

[ebarault]

@ssh24: thanks for joining!

I think it's more subtle than that, have you checked this other issue?
https://github.com/strongloop/loopback-datasource-juggler/issues/1297

I believe there is one valid case where we should assign polymorphicName rather than targetModel : if relation is hasMany, params.model is omitted, and polymorphic=targetModelName. The code looks like such a shortcut definition was intended.

"someRelation" : {
  "type": "hasMany",
  "polymorphic": "someModelName"
}

Thoughts?

I believe this should be back ported. I'm currently reviewing the polymorphic docs, it's very confusing and lots of examples don't work, lb2 and lb3. We should fix and simplify things

[ssh24]

I looked at the other issue and am bit confused.

If the relation is hasMany, according to the docs polymorphic relation can be defined in both ways:

{
  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "model": "Picture",
      "polymorphic": "imageable"
    }
  }
...

And:

{
  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "model": "Picture",
      "polymorphic": {
        "as": "imageable",
        ...
       } 
    }
  }
...

This issue is regarding the second code snippet where it assigns the polymorphicName to imageable and params.model also gets that value, am I correct?

Now, I understand with both #1297 and this issue the relation object will look something like in the first code snippet, where polymorphic could be either polymorphicName or the targetModelName.

However, that brings up a question in my mind. How would we support other fields inside the polymorphic objects? Fields like foreignKey, discriminator:

  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "model": "Picture",
      "polymorphic": {
        "as": "imageable",
        "foreignKey": "imageableId",
        "discriminator": "imageableType"
       } 
    }
  }
...

[ebarault]

as said i am currently reviewing all the polymorphic relations doc and find lots of examples that just don't work.

take this first snippet you copy-pasted:

{
  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "model": "Picture",
      "polymorphic": "imageable"
    }
  }
...

it's incorrect when using model JSON definition although it works when defined by code (see this test, let's have a look :

• the relation name is already defined by the relation key: pictures
• the modelTo is defined explicitly as "model": "Picture"
• now there's this last param "polymorphic" of type string :
  
  
  
  • according to lib/datasource.js#L507, it should replace the relation modelTo, ... which is already defined here as Picture
  
  
  • according to lib/relation-definition.js#L561, it's should be assigned to the polymorphic.as property, which stands for the name of the relation,... which is already defined as pictures
  
  

what a mess ! 🤕

_NOTE: surprisingly I found this test which seems to work with this model JSON definition syntax, but it's only because the Picture model is not attached and it fallbacks in a createListener which totally lacks the polymorphic logic (see here then here)
This test should be reworked along with the createListener function in DataSource.defineRelation();_

---

To simplify, the keyword as should never be used for polymorphic relations in the JSON model definitions.

the correct JSON definitions are either:

{
  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "polymorphic": "Picture"
    }
  }
...

or

{
  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "model": "Picture",
      "polymorphic": {
        "foreignKey": "imageableId",
        "discriminator": "imageableType"
      }
    }
  }
...

I'm using them intensively. The only case where using the as keyword works is if assigned a model name, which deceives the concept behind as, which elsewhere in LB represents a way to customize the relation name. This can only mislead users.

---

I'm also doing a similar assessment for relations defined by code using the methods like .hasMany(), .belongsTo(), etc. and i believe it should not be used either inside the relation.polymorphicobject, but should be only used internally. The only askeyword which is still valid when defining relations by code is the top level one.

for e.g.
correct
Author.hasMany(Picture, {as: 'photos', polymorphic: {foreignKey: 'imageableId', discriminator: 'imageableType'}}

not correct
Author.hasMany(Picture, {polymorphic: {as: 'photos', foreignKey: 'imageableId', discriminator: 'imageableType'}}

in this latter case, the current code would look for a model named photos and throw an error.

---

However, that brings up a question in my mind. How would we support other fields inside the polymorphic objects? Fields like foreignKey, discriminator

defining foreignKey, discriminator is fully supported. See in lib/relation-definition.js#L563 : only if the values are not provided they are replaced by default values.

[ssh24]

@ebarault This makes sense. Thank you for the clear explanation. I am reviewing the related PR #1301

[jannyHou]

@ebarault Thank you for fixing the gap between Doc and code of polymorphic relation, I am trying to review your pr https://github.com/strongloop/loopback-datasource-juggler/pull/1301, and after reading through all related issues I feel I'd better write down my understanding here and make sure we are on the same page, in case I overlook some details and give you useless& confusing reviews :p

Maybe let's start from reaching an agreement on what is our expected doc, then implement&review code according to the correct doc.

There are three entries that we need to clarify their meaning and usage:
Given a polymorphic relation defined in modelFrom:

rName: {
  type: 'rType',
  as: 'rAs',
  model: 'rModelTo'
  polymorphic: {
    as: 'rAs',
    foreignKey: 'rAsId',
    discriminator: "rAsType"
  }
}

• as:
  
  
  
  • Represents the relationName.
  
  
  • Will be overridden if polymorphic(string) or polymorphic(object).as provided
  
  
• model:
  
  
  
  • Represents the modelTo in a relation.
  
  
  • On the other side, the related model(modelTo) will have a belongsTo relation defined as
    
    js relatedRName: { type: 'belongsTo', model: 'not_sure_is_it_defaultRelationModelFrom_or_rAs', // I am confused with the `model` property here. // My understanding is it could serve for either purpose but **NOT BOTH**: // 1. Represents the default modelFrom, // but the actual modelFrom should be looked up dynamically by polymorphic relation // 2. Represents the `rAs` defined in modelFrom's relation, // otherwise we have no idea which polymorphic relation it belongsTo. polymorphic: { as: 'rAs' // **`as` is NOT mentioned in doc** // Whether we need it depends on what's the usage of the `model` property above // If `model` serves the first purpose(Represents the default modelFrom), // this will be a **MUST PROVIDED** property, // otherwise we have no idea which polymorphic relation it belongsTo // If `model` serves the second purpose(Represents the `rAs` defined in modelFrom's relation) // this one will be optional, and will override `model` if provided foreignKey: 'rAsId', discriminator: 'rAsType' // these two properties should be the same as the ones defined in modelFrom's relationDef } }
  
  
• polymorphic:
  
  
  
  • typeOf polymorphic === String
  
  
  • Represents the relationName
  
  
  • Will override as
  
  
  • foreignKey will be generated by default as as + 'Id',
    
    discriminator will be generated by default as as + 'Type'
  
  
  • typeOf polymorphic === Object
  
  
  • as: Represents the relationName
  
  
  • foreignKey:
    
    
    
    
    
    • A property added to modelTo.
    
    
    
    • Represents the fk to modelFrom's id.
    
    
    
    • Will be generated by default as as + 'Id'
    
    
    
  
  
  • discriminator:
    
    
    
    
    
    • A property added to modelTo.
    
    
    
    • Represents the actual modelFrom and be looked up and defined dynamically .
    
    
    
    • Will be generated by default as as + 'Type'
    
    
    
  
  

[ebarault]

@jannyHou
IMO the starting point is the following test: it('sets up polymorphic relations')

it expects one of the configurations described in the docs so let's take it as a basis:

hasMany relation

{
  "name": "Author",
  ...
  "relations": {
    "pictures": {
      "type": "hasMany",
      "model": "Picture",
      "polymorphic": "imageable"
    }
  }
...
}

belongsTo relation

{
  "name": "Picture",
  ...
  "relations": {
    "imageable": {
      "type": "belongsTo",
      "polymorphic": true
    }
  }
...
} 
// note that in the example from the doc and in the test "imageable" is a very badly badly chosen name IMO as it's meant to reflect the concept of --belonging to Author--

It instructs that

• if type == 'hasMany'
• and typeOf polymorphic == 'string'
• 👉 polymorphic expects the name of the belongsTo relation name.

from the belongsTo relation name will be deduced the relation's foreignKey and discriminators.

---

(considering we represent here the spec for the hasMany polymorphic relation)

Based on that your following statement is incorrect:

• polymorphic:
  
  
  
  • typeOf polymorphic === String
  
  
  • Represents the relationName belongsTo relation name
  
  
  • Will override as
    
    
    
    
    
    • foreignKey will be generated by default as as + 'Id' polymorphic + 'Id',
    
    
    
    • discriminator will be generated by default as as + 'Type' polymorphic + 'Type'
    
    
    
  
  

it does not represent the hasMany relation name, but the belongsTo relation name
in such conditions, the rest of the statement regarding is correct if replacing as by polymorphic

---

Now regarding the case where polymorphic is an object:

• polymorphic:
  
  
  
  • typeOf polymorphic === Object
  
  
  • as: Represents the relationName belongsTo relation name
  
  
  • foreignKey: A property from modelTo.
    
    
    
    
    
    • Represents the fk to modelFrom's id.
    
    
    
    • Will be generated by default as as + 'Id'
    
    
    
  
  
  • discriminator: A property from modelTo.
    
    
    
    
    
    • Represents the actual modelFrom and be looked up and defined dynamically .
    
    
    
    • Will be generated by default as as + 'Type'
    
    
    
  
  

the use of as inside the polymorphic object is very misleading. as will be used to generate the foreignKey and discriminator properties but the only case where it's relevant to do that is if as represents the belongsTo relation name.

so the sense of as in the polymorphic object is the opposite of the sense of as at the root of the relation object:

• as at root of modelFrom relation object: used to override the name of the modelFrom relation
• as in modelFrom hasMany relation polymorphic object: represents the modelsTo's belongsTo relation name, used to generate foreignKey and discriminator when not provided

so your statement here is also incorrect:

rName: {
 type: 'rType',
 as: 'rAs',
 model: 'rModelTo'
 polymorphic: {
   as: 'rAs',
   foreignKey: 'rAsId',
   discriminator: "rAsType"
 }
}

• as:
  Represents the relationName.
  Will be overridden if polymorphic(string) or polymorphic(object).as provided

it should not as the outer as and the inner as do not represent the same relation name (modelFrom vs. modelTo)

---

TBH: i don't know how exactly to handle such a naming conflict issue without changing the name of the polymorphic as. Even very precisely described in the docs it will keeps confusing people.

[ebarault]

putting aside the polymorphic as naming problem, a proper spec for the hasMany/belongsTo polymorphic relations would be:

hasMany relation

• type: hasMany
• as: redefines this relation's name (optional)
• model: modelTo
• polymorphic:
  
  
  
  • typeOf polymorphic === String
  
  
  • matching belongsTo relation name
    
    
    
    
    
    • foreignKey is generated as polymorphic + 'Id',
    
    
    
    • discriminator is generated as polymorphic + 'Type'
    
    
    
  
  
  • typeOf polymorphic === Object
  
  
  • as: matching belongsTo relation name
    
    
    
    
    
    • (required) if both foreignKey and discriminator are NOT provided
    
    
    
    • (extraneous) if both foreignKey and discriminator are provided
    
    
    
  
  
  • foreignKey: A property of modelTo, representing the fk to modelFrom's id.
    
    
    
    
    
    • generated by default as as + 'Id'
    
    
    
  
  
  • discriminator: A property of modelTo, representing the actual modelFrom to be looked up and defined dynamically
    
    
    
    
    
    • generated by default as as + 'Type'
    
    
    
  
  

---

belongsTo relation

• type: belongsTo
• as: redefines this relation's name (optional)
• model: NOT EXPECTED: should throw an error as relation validation
• polymorphic:
  
  
  
  • typeOf polymorphic === Boolean
    
    
    
    
    
    • foreignKey is generated as relationName + 'Id',
    
    
    
    • discriminator is generated as relationName + 'Type'
    
    
    
  
  
  • typeOf polymorphic === Object
  
  
  • as: NOT EXPECTED: should throw an error at relation validation
  
  
  • foreignKey: A property of modelTo, representing the fk to modelFrom's id.
  
  
  • discriminator: A property of modelTo, representing the actual modelFrom to be looked up and defined dynamically .
  
  

👉 this is mostly compatible with the current doc, once cleaned up a few errors
what do you think?

[jannyHou]

Thank you @ebarault your definition post in https://github.com/strongloop/loopback-datasource-juggler/issues/1298#issuecomment-298596768 looks perfect to me 👍

[ebarault]

@jannyHou excellent! i'll review the relation definition code accordingly then.

---

Regarding the naming of polymorphic as:

Considering that when typeOf polymorphic === Object, as is extraneous when foreignKey and discriminator are provided, and that

polymorphic: {
  as: 'belongsToRelName`
}

is fully equivalent to:

polymorphic: 'belongsToRelName`

I suggest we stop referencing the polymorphic as keyword in the doc and favor the latter shorthand declaration. The code will keep handling the as keyword though for backward compatibility.

What do you think?

[jannyHou]

I suggest we stop referencing the polymorphic as keyword in the doc

@ebarault Agree, I had same thought yesterday, I would prefer to define the relation either like
polymorphic: 'belongsToReName'
OR
polymorphic: {fk: 'fkName', discriminator: 'disName'},
just DO NOT combine them with as

[jannyHou]

@kjdelisle TBR

• repo: loopback-datasource-juggler
• version: minor release

[jannyHou]

Document PR: https://github.com/strongloop/loopback.io/pull/381