fix(cli): discard changes if lb4 `relation` fails. Allow customized relation name for belongsTo #4373

agnes512 · 2020-01-06T19:33:45Z

Fixes #3287 and #4209

1st commit: code changes
2nd commit: tests

Before

Example:

hasMany
? SourceModel: Customer
? DestModel: Order,
? Foreign key to define on the target model: customerId
? Source property name for the relation getter: `orders
? Allow Order queries to include data from related Customer instances? Yes
==> issues:
- if generation gets aborted, part of code still gets generated in Model and Repository
- the default value doesn't match with the code in /repository/relation
belongTo
? SourceModel: Order
? DestModel: Customer,
? Source property name for the relation getter: customerId
? Allow ..inclusion? Yes
==> issues:
- if generation gets aborted, part of code still gets generated in Model and Repository
- the default value doesn't match with the code in /repository/relation
- generated code failed if the foreign key name is not the default value
- generator doesn't respect custom relation name, the only relation it generates is targetModelName (e.g customer)

New changes

checks if corresponding repository exists
checks if pk exists and validates the name
checks if fk exists and validates the name
asks fk for both hasMany and belongsTo
asks relation name for both hasMany and belongsTo
checks if the relation name exists
allow custom relation name for belongsTo
- This fixes bug(cli): cli does not respect custom relation getter name #4209
- customized relation name will be specified in the relation metadata

Examples of new prompt:

hasMany
? Please select the relation type hasMany
? Please select source model Customer
? Please select target model Order
? Foreign key name to define on the target model c_id // not default value
? Source property name for the relation getter (will be the relation name) orders
? Allow Customer queries to include data from related Review instances? Yes

=>

// Customer property:
  @hasMany(() => Order, {keyTo: 'c_id'})  // custom fk
  orders: Order[];

// Order fk:
  @property({
    type: 'number',
  })
  c_id?: number;  // custom fk

belongsTo
? Please select the relation type belongsTo
? Please select source model Order
? Please select target model Customer
? Foreign key name to define on the source model c_id
? Relation name my_customer // not default name
? Allow Customer queries to include data from related Review instances? Yes

=>

//Order fk:
  @belongsTo(() => Customer, {name: 'my_customer'}) . // allow custom relation name
  c_id: number;    // allow custom fk name

For belongsTo, if the fk name is the same as the relation name, the prompt would ask the user to re-enter a new name to avoid a known issue: Relations with custom names not working

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 👈

agnes512

Add a new prompt to ask the source key of belongsTo (e.g customerId). By comparing the source key name and the relation name, the generator would respect customized relation name, see issue #4209

agnes512 · 2020-01-08T20:41:30Z

packages/cli/generators/relation/belongs-to-relation.generator.js

+      },
+    ];
+    // except the customized relation name, also need to check if the property name is the same as relation name
+    // or it will get 'navigational property' error when creating instances


related stackoverflow question.

packages/cli/generators/relation/index.js

agnes512 · 2020-01-08T20:47:46Z

packages/cli/generators/relation/index.js

+    this.artifactInfo.defaultRelationName = this._getDefaultRelationName();
+    // for hasMany && hasOne, the source key is the same as the relation name
+    const msg =
+      this.artifactInfo.relationType === 'belongsTo'


This property name will be the source key and the relation name for hasMany and hasOne. So I separate it from belongsTo here.

agnes512 · 2020-01-08T20:53:47Z

@derdeka @achrinza could you take a look? part of code fixes #4209. Thanks.

bajtos

I am afraid I don't have bandwidth to review the changes in details, I'll leave the review to somebody else who is more familiar with this part of our code base.

packages/cli/generators/relation/base-relation.generator.js

packages/cli/snapshots/integration/generators/relation.belongs-to.integration.snapshots.js

jannyHou · 2020-01-09T15:01:38Z

packages/cli/snapshots/integration/generators/relation.belongs-to.integration.snapshots.js

 > {

-  public readonly customerClass: BelongsToAccessor<CustomerClass, typeof OrderClass.prototype.orderNumber>;
+  public readonly customerClassCustNumber: BelongsToAccessor<CustomerClass, typeof OrderClass.prototype.orderNumber>;


hmm what does Cust mean 🤔 ?

CustNumber is the name of the id property of customerClass model.

I change the generator to generate customerClassId instead to match with our code in /repository/relation

jannyHou

Great effort on improving the complicated prompts in relation cli 👍 👍
I left some minor comments and questions.

packages/cli/test/integration/generators/relation.has-many.integration.js

jannyHou · 2020-01-09T15:46:46Z

packages/cli/generators/relation/index.js

+      );
+      // checks if the relation name already exists in repo
+      if (
+        relationUtils.doesPropertyExist(


Just curious: I think the relation definition is in the model file, why do we have to use more steps to get the corresponding repository file and check the existence?

Good question! From my understanding the model has the property (e.g orders or customerId). And the repository has the relation name (e.g orders or customer). They might have the same name but most of time they don't. For example, for belongsTo relation, the relation name is customer.

The generator throws errors if the repository already has the relation name defined. And the generator didn't have such checks before .

And the repository has the relation name

Oh that's smart! 👍 makes perfect sense then

packages/cli/generators/relation/index.js

emonddr · 2020-01-09T15:32:01Z

packages/cli/generators/relation/belongs-to-relation.generator.js

+      `this.${relationName} = ` +
      `this.createBelongsToAccessorFor('` +
-      `${this.artifactInfo.relationName.replace(/Id$/, '')}',` +
+      `${relationName}',` +


Here Id is no longer going to be replaced with empty string. Will this cause a regression?

I have relationName defined above so we don't need it here anymore.

emonddr · 2020-01-09T15:46:00Z

packages/cli/generators/relation/has-many-relation.generator.js


  _getRepositoryRelationPropertyName() {
-    return utils.pluralize(utils.camelCase(this.artifactInfo.dstModelClass));
+    return this.artifactInfo.relationName;


We're removing calls to the util.camelCase in several places. Will this cause a problem?

I have relationName defined above so we can just use the value here.

packages/cli/generators/relation/index.js

packages/cli/test/integration/generators/relation.has-many.integration.js

emonddr · 2020-01-09T18:17:12Z

The relation-based test cases pass (i ran them on my laptop), so that's great. Doesn't appear to be any regressions.
Some coverage fell though :S

agnes512 · 2020-01-09T20:39:31Z

The relation-based test cases pass (i ran them on my laptop), so that's great. Doesn't appear to be any regressions.
Some coverage fell though :S

If you check the tests It is caused by the generator every time it does exit(err), not the actual code itself. I don't any way to skip it. If anyone knows please let me know 😄

achrinza

Just curious; do we also need to update the docs that reference the CLI prompt procedure?

See:

agnes512 · 2020-01-10T12:45:18Z

Just curious; do we also need to update the docs that reference the CLI prompt procedure?

See:

https://loopback.io/doc/en/lb4/Relation-generator.html

https://loopback.io/doc/en/lb4/todo-list-tutorial-relations.html

Yes, I will add them once the proposal looks good to most of you 😄

jannyHou · 2020-01-10T15:31:28Z

packages/cli/generators/relation/base-relation.generator.js

-    } else {
-      relationUtils.addProperty(classDeclaration, property);
-    }
+    // already chcked the existence of property before


(sorry I am not very familiar with the relation cli which results in many questions ❔ ❔ ) Hmm, where is it checked? did a quick search of code before it but didn't find any clue 🙈

Here the property is the relation name, which has been checked in index file. Sorry the PR is big, I should've separated them.

jannyHou · 2020-01-10T15:33:35Z

packages/cli/generators/relation/base-relation.generator.js

-      throw new Error(
-        'Parameter ' + parameterName + ' already exist in the constructor.',
-      );
+      // no need to check if the getter already exists


Doesn't parameter exist imply the relation already exists? And therefore an error should be thrown like the deleted code?

The param is getter, such as @repository.getter('OrderRepository') protected orderRepositoryGetter: Getter<OrderRepository>. And I don't think this implies a certain relation name already exists. That's why I removed it.

And I was also thinking about some edge cases. For example, since we allow customized keyFrom, user could have two same relations with 2 models with two different names. In this case, they will share the same getter.

since we allow customized keyFrom, user could have two same relations with 2 models with two different names. In this case, they will share the same getter.

👍 Thank you I agree with you.
The property should be checked instead of parameter.

jannyHou · 2020-01-10T15:48:16Z

packages/cli/generators/relation/belongs-to-relation.generator.js

+      relationDecorator = [
+        {
+          name: 'belongsTo',
+          arguments: [`() =>  ${className}, {name: '${relationName}'}`],


I am a bit confused about defaultRelationName and sourceKeyName, wouldn't they be the same?
I find it hard to think of a user scenario that defaultRelationName, sourceKeyName and relationName are all different...

Good point. I will list out the differences:

Before, the generator asked foreign key, relationName for hasMany.
and it only asked relationName for belongTo if defaultRelationName doesn't exist. It used relationName as the foreign key name (e.g customerId). Then it use the target model as the real relation name (e.g `customer).

This is why how it dealt with belongsTo is problematic:

if the default foreign key already exists, it didn't prompt

if the default foreign key doesn't exists, it takes whatever it is, but the real relation name is still the target model, and this is different from how the belongsToAccessor generates the default name. For example: if you type in customer_id as your foreign key, the generator would have customer as the default relation name. But for the belongsToAccessor, the default relation name is customer_id instead of customer. That's why users are getting Cannot find type of undefined errors.

In order to allow custom fk and relation name for belongsTo, I added a new field sourceKeyName to represent the foreign key of belongsTo. And relationName to represent the real relation name.

The line defaultRelationName !== relationName here is to check if the relationName is customized. If so, it should be specified in the name field, just like how we documented in our docs Relation Metadata.

Therefore, it can have correct property:

@belongsTo(() => Customer, {name: 'my_customer'}) // customized relation name customer_id: number; // customized fk

packages/cli/generators/relation/belongs-to-relation.generator.js

packages/cli/generators/relation/index.js

packages/cli/generators/relation/belongs-to-relation.generator.js

packages/cli/generators/relation/index.js

packages/cli/lib/utils.js

packages/cli/test/fixtures/relation/index.js

docs/site/BelongsTo-relation.md

docs/site/Relation-generator.md

nabdelgadir

Great job 👍

agnes512 force-pushed the fix-lb4-relation branch 2 times, most recently from 479d9e0 to d492728 Compare January 8, 2020 20:38

agnes512 changed the title ~~fix(cli): discard changes if lb4 relation fails~~ fix(cli): discard changes if lb4 relation fails. Allow customized relation name for belongsTo Jan 8, 2020

agnes512 commented Jan 8, 2020

View reviewed changes

agnes512 marked this pull request as ready for review January 8, 2020 20:51

agnes512 requested review from bajtos and raymondfeng as code owners January 8, 2020 20:51

agnes512 requested review from achrinza and derdeka January 8, 2020 20:53

bajtos requested review from elv1s and nabdelgadir January 9, 2020 07:59

bajtos reviewed Jan 9, 2020

View reviewed changes

packages/cli/generators/relation/base-relation.generator.js Outdated Show resolved Hide resolved

packages/cli/snapshots/integration/generators/relation.belongs-to.integration.snapshots.js Show resolved Hide resolved

jannyHou reviewed Jan 9, 2020

View reviewed changes

emonddr suggested changes Jan 9, 2020

View reviewed changes

achrinza requested changes Jan 10, 2020

View reviewed changes

jannyHou reviewed Jan 10, 2020

View reviewed changes

agnes512 force-pushed the fix-lb4-relation branch 3 times, most recently from 6d8e6a5 to 3a8932a Compare January 12, 2020 20:29

nabdelgadir reviewed Jan 13, 2020

View reviewed changes

agnes512 force-pushed the fix-lb4-relation branch 4 times, most recently from 5d33f0d to 6c552fb Compare January 13, 2020 17:27

nabdelgadir reviewed Jan 14, 2020

View reviewed changes

docs/site/BelongsTo-relation.md Show resolved Hide resolved

docs/site/Relation-generator.md Outdated Show resolved Hide resolved

agnes512 force-pushed the fix-lb4-relation branch 2 times, most recently from 5fe3a6f to fb85d50 Compare January 14, 2020 13:42

nabdelgadir approved these changes Jan 14, 2020

View reviewed changes

agnes512 force-pushed the fix-lb4-relation branch from fb85d50 to ad21b69 Compare January 14, 2020 16:12

achrinza approved these changes Jan 15, 2020

View reviewed changes

emonddr approved these changes Jan 16, 2020

View reviewed changes

agnes512 force-pushed the fix-lb4-relation branch from ad21b69 to c6df843 Compare January 17, 2020 16:44

Agnes Lin added 3 commits January 17, 2020 11:45

fix(cli): disgard any changes if lb4 relation fails

209dc11

test(cli): modify tests for relation generator changes

e939aaf

docs(cli): update docs with new changes

6f0f474

agnes512 force-pushed the fix-lb4-relation branch from c6df843 to 6f0f474 Compare January 17, 2020 16:46

agnes512 merged commit 0ca8cd1 into master Jan 17, 2020

agnes512 deleted the fix-lb4-relation branch January 17, 2020 18:19

agnes512 mentioned this pull request Jan 17, 2020

feat(cli): add hasOne relation type to lb4 relation #4171

Merged

nabdelgadir mentioned this pull request Jan 22, 2020

docs: update todo-list example #4412

Merged

7 tasks

bajtos mentioned this pull request May 22, 2020

@loopback/cli: Generate proper relation belongsTo fix #5430 #5492

Merged

7 tasks

fix(cli): discard changes if lb4 relation fails. Allow customized relation name for belongsTo #4373

fix(cli): discard changes if lb4 relation fails. Allow customized relation name for belongsTo #4373

Uh oh!

Conversation

agnes512 commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

Example:

New changes

Examples of new prompt:

Checklist

Uh oh!

agnes512 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agnes512 commented Jan 8, 2020

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emonddr commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agnes512 commented Jan 9, 2020

Uh oh!

achrinza left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agnes512 commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fix(cli): discard changes if lb4 `relation` fails. Allow customized relation name for belongsTo #4373

fix(cli): discard changes if lb4 `relation` fails. Allow customized relation name for belongsTo #4373

agnes512 commented Jan 6, 2020 •

edited

Loading

emonddr commented Jan 9, 2020 •

edited

Loading

achrinza left a comment •

edited

Loading