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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(repository): make the navigational property err msg more clear #4400

Merged
merged 1 commit into from Jan 13, 2020

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented Jan 9, 2020

related #4392, stackoverflow

The belongsTo generator generates the default relation name by taking the @belongsTo decorated name then removing Id from it. Idealy, the default fk name is expected to be customerId, and the ideal relation name will be customer (code).

However, lots of users would come up with their own customized name, customer_id for example. And the default relation name generated by the belongsTo will be customer_id according to the code. This is problematic because in CRUD operations, it recognize customer_id as a navigational property instead of a model property. As a result, operations like

orderRepo.create({id: 1, name: 'LoopBack', customer_id: 1})

will be rejected with error 500 Error: Navigational properties are not allowed in model data.

Currently I couldn't find a good way to improve the way it generates the default relation name. So I am proposing to make the error message more clear and also update the site before we come up a better way for generating the default relation name.

Example new error message.
Navigational properties are not allowed in model data (model "Order" property "customer_id"). Please remove it or make sure the relation name is not the same as the property name in belongsTo relation.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for improving the error message 鉂わ笍

@@ -221,7 +221,7 @@ export function hasManyRelationAcceptance(
],
}),
).to.be.rejectedWith(
'Navigational properties are not allowed in model data (model "Customer" property "orders")',
'Navigational properties are not allowed in model data (model "Customer" property "orders"). Please remove it or make sure the relation name is not the same as the property name in belongsTo relation.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is too long and difficult to review on GitHub. Please split it into multiple lines, e.g.

.to.be.rejectedWith(
  'Navigational properties are not allowed in model data (model "Customer" property "orders"). ' + 
    'Please remove it or make sure the relation name is not the same as the property name in belongsTo relation.',
);

(It's just an example, I am not sure if that's enough to keep the line length under 80 chars.)

The same comment applies to most other changed lines too.

@@ -583,7 +583,7 @@ export class DefaultCrudRepository<
const relName = def.relations[r].name;
if (relName in data) {
throw new Error(
`Navigational properties are not allowed in model data (model "${this.entityClass.modelName}" property "${relName}")`,
`Navigational properties are not allowed in model data (model "${this.entityClass.modelName}" property "${relName}"). Please remove it or make sure the relation name is not the same as the property name in belongsTo relation.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be belongsTo or hasMany?
Like Customer hasMany orders, IIUC the navigational property here is orders, which is the relation name for hasMany

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes,
For hasMany, orders would only be the navigational property. When you create a customer, you would do customerRepo.create({id: 1, name: 'customer'});

But for belongsTo, by how we defining the default relation name, the fk customer_id would have the default relation name as customer_id. When you do orderRepo.create({id: 1, des: 'order', customer_id: 1}), it recognizes the customer_id in the data as the relation name instead of the foreign key. That's why I state it clear that that the error might be caused by the nav property OR the naming issue here.

Please let me know if you have any suggestions to make it more intuitive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agnes512 馃憤 thank you got it. How about the following warning:

Navigational properties are not allowed in model data (model "${this.entityClass.modelName}" property "${relName}"), please remove it.
For relation belongsTo, please make sure the relation name is not the same as the property name.

for (const r in def.relations) {
const relName = def.relations[r].name;
if (relName in data) {
let invalidName = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: invalidName --> invalidNameMsg :-p

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憦 LGTM

@agnes512 agnes512 merged commit 2d493bc into master Jan 13, 2020
@agnes512 agnes512 deleted the update-error-msg branch January 13, 2020 20:51
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 this pull request may close these issues.

None yet

3 participants