feat(cli): import LB3 models with a custom base class #4737

bajtos · 2020-02-25T11:05:11Z

The initial implementation of lb4 import-lb3-models was able to import only models inheriting from models that have a built-in counter-part in LB4: Model, PersistedModel, KeyValueModel.

In this pull request, I am implementing import of models inheriting from non-LB4-builtins. There are two major use cases:

Import a model inheriting from an LB3 built-in model like User.
Import a model inheriting from an application-specific model.

The changes are split into multiple smaller commits to make the patch easier to review.

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 👈

bajtos · 2020-02-25T11:06:21Z

@emonddr @deepakrkris @nabdelgadir @agnes512 please review

bajtos · 2020-02-25T11:13:42Z

packages/cli/test/unit/import-lb3-model/migrate-model.test.js

@@ -33,7 +33,6 @@ describe('importLb3ModelDefinition', () => {
      expect(toJSON(modelData.settings)).to.deepEqual({
        // By default, LB3 models are not strict
        strict: false,
-        replaceOnPUT: true,


FYI: replaceOnPUT is a flag set by juggler, it's used for backwards compatibility with LoopBack 2.x.

Quoting from https://loopback.io/doc/en/lb4/Model.html:

replaceOnPUT - This entry is no longer supported as LB4 controllers scaffolded by LB4 controller, PUT is always calling replaceById. Users are free to change the generated code to call patchById if needed.

bajtos · 2020-02-25T11:14:40Z

packages/cli/generators/import-lb3-models/index.js

+          'Adding %s (base of %s) to the list of imported models.',
+          chalk.yellow(baseName),
+          chalk.yellow(name),
+        );


Here is an example console output.

$ lb4 import-lb3-models ~/src/loopback/next/packages/cli/test/fixtures/import-lb3-models/app-using-model-inheritance.js WARNING: This command is experimental and not feature-complete yet. Learn more at https://loopback.io/doc/en/lb4/Importing-LB3-models.html ? Select models to import: Customer Model Customer will be created in src/models/customer.model.ts Adding UserBase (base of Customer) to the list of imported models. Model UserBase will be created in src/models/user-base.model.ts Adding User (base of UserBase) to the list of imported models. Model User will be created in src/models/user.model.ts Import of model relations is not supported yet. Skipping the following relations: accessTokens Ignoring the following unsupported settings: acls create src/models/customer.model.ts create src/models/user-base.model.ts create src/models/user.model.ts update src/models/index.ts update src/models/index.ts update src/models/index.ts

So just making sure I understand correctly, we're importing User because UserBase inherits from it? And do we always eventually want one model to extend from a built in model (e.g. in this case User inheriting from Entity)?

We're importing User because UserBase inherits from it?

Yes, exactly 👍

And do we always eventually want one model to extend from a built in model (e.g. in this case User inheriting from Entity)?

Yes. Every LoopBack 3 model has the LB3 built-in Model at the top of the inheritance chain. Some models are inheriting from PersistedModel (which inherits from Model and is mapped to Entity in LB4), some models are inheriting from KeyValueModel (which inherits from Model and is probably mapped to KeyValueModel in LB4, I am not sure).

bajtos · 2020-02-25T11:17:29Z

packages/cli/snapshots/integration/generators/import-lb3-models.integration.snapshots.js

-    validateUpsert: true,
-    idInjection: true
-  }
+  settings: {strict: false, forceId: false, validateUpsert: true, idInjection: true}


The following settings are no longer imported because they are inherited:

replaceOnPUT

Please note that forceId, idInjection and validateUpsert are explicitly configured in coffee-shop's model JSON file, therefore they are preserved in the generated output. You can compare this line with User settings below https://github.com/strongloop/loopback-next/pull/4737/files#diff-e9be610fe11bc2c954313d9283d0eda3R128-R137, where only strict + custom settings are generated.

agnes512

LGTM in general 👏 Just have a few questions.

agnes512 · 2020-02-25T15:25:10Z

packages/cli/generators/import-lb3-models/index.js

+    if (!templateData.isModelBaseBuiltin) {
+      const baseName = templateData.modelBaseClass;
+      if (
+        !this.existingLb4ModelNames.includes(baseName) &&


Just want to make sure that I understand it correctly, the reason we check the existence of the base model is because we need to import the custom base class and also it's parent class if that doesn't exist?

Yes.

Let's say the application has:

AuditedPersistedModel which is adding auditing functionality for all CRUD operations provided by PersistedModel,

several models like Product, Category and Store, where all models are based on AuditedPersistedModel.

When we run import for the first time and choose only Store to import, we also need to import AuditedPersistedModel to allow the new LB4 model to inherit from the correct base class (model).

When we run import for the second time and choose to import Product and Category, we should not import AuditedPersistedModel again, because that could overwrite bits imported manually in the previous step. Instead, LB4 CLI will skip importing this base model.

agnes512 · 2020-02-25T15:40:50Z

packages/cli/generators/import-lb3-models/migrate-model.js

+  // Core LB3 models like PersistedModel come with an id property that's
+  // injected by juggler. We don't want to inherit that property, because
+  // in LB4, we want models to define the id property explicitly.
+  if (isCoreModel(modelCtor.base)) {


Sorry I am a bit confused. Does this mean if a custom model inherits a base model, it won't have the id property after importing to LB4 app?

When scaffolding code for an imported model, we want to skip all properties inherited from the base model. For example, if User model has email property and Customer model inherits from User, then the code for Customer SHOULD NOT re-define email property again. The property should be inherited instead, both at TypeScript and juggler-metadata level.

Now juggler's PersistedModel always comes with id property (in fact, the property is added to every model created by juggler, including the base Model class), see ModelBuilder.prototype.define().

As a result, when deciding which properties belong to the model being imported (e.g. Product) and which are inherited (e.g. from PersistedModel), the primary key property id was always skipped before I introduced this check, simply because it was considered as coming from the base model (PersistedModel).

jannyHou

LGTM 👍

packages/cli/test/fixtures/import-lb3-models/app-using-model-inheritance.js

jannyHou · 2020-02-27T02:59:58Z

packages/cli/generators/import-lb3-models/migrate-model.js

-    const def = migratePropertyDefinition(properties[prop]);
+    const propDef = properties[prop];
+
+    // Skip the property if it was inherited from the base model (the parent)


reasonable to me 👍

nabdelgadir

Just have a clarification question, but LGTM

nabdelgadir · 2020-02-27T03:42:01Z

packages/cli/generators/import-lb3-models/index.js

+          'Adding %s (base of %s) to the list of imported models.',
+          chalk.yellow(baseName),
+          chalk.yellow(name),
+        );


So just making sure I understand correctly, we're importing User because UserBase inherits from it? And do we always eventually want one model to extend from a built in model (e.g. in this case User inheriting from Entity)?

We are using LB3 only for tests. CLI commands dealing with LB3 apps are using `loopback` module installed inside the LB3 app. Signed-off-by: Miroslav Bajtoš <[email protected]>

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos added the CLI label Feb 25, 2020

bajtos requested review from agnes512, deepakrkris, emonddr and nabdelgadir as code owners February 25, 2020 11:05

bajtos force-pushed the feat/import-lb3-models-with-base branch from 8ff723b to abc4c07 Compare February 25, 2020 11:06

bajtos requested review from hacksparrow, jannyHou and raymondfeng as code owners February 25, 2020 11:06

bajtos self-assigned this Feb 25, 2020

bajtos added feature Migration labels Feb 25, 2020

bajtos added this to the Feb 2020 milestone Feb 25, 2020

bajtos linked an issue Feb 25, 2020 that may be closed by this pull request

Spike: how to import LB3 models extending a custom base model #3813

Closed

bajtos commented Feb 25, 2020

View reviewed changes

agnes512 reviewed Feb 25, 2020

View reviewed changes

emonddr approved these changes Feb 25, 2020

View reviewed changes

derdeka mentioned this pull request Feb 26, 2020

feat(lb3-compat): Loopback3 compatibilty extension/package #4747

Closed

jannyHou approved these changes Feb 27, 2020

View reviewed changes

nabdelgadir approved these changes Feb 27, 2020

View reviewed changes

bajtos added 4 commits February 27, 2020 11:56

fix(cli): move loopback to devDependencies

8420c68

We are using LB3 only for tests. CLI commands dealing with LB3 apps are using `loopback` module installed inside the LB3 app. Signed-off-by: Miroslav Bajtoš <[email protected]>

feat(cli): import LB3 models with a custom base class

15c8cc2

Signed-off-by: Miroslav Bajtoš <[email protected]>

feat(cli): skip inherited props & settings when importing a LB3 model

b9716d1

Signed-off-by: Miroslav Bajtoš <[email protected]>

feat(cli): automatically include base models when importing LB3 models

8c5f3cf

Signed-off-by: Miroslav Bajtoš <[email protected]>

bajtos force-pushed the feat/import-lb3-models-with-base branch from abc4c07 to 8c5f3cf Compare February 27, 2020 10:58

bajtos merged commit 9d5f8a7 into master Feb 27, 2020

bajtos deleted the feat/import-lb3-models-with-base branch February 27, 2020 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): import LB3 models with a custom base class #4737

feat(cli): import LB3 models with a custom base class #4737

bajtos commented Feb 25, 2020 •

edited

Loading

bajtos commented Feb 25, 2020

bajtos Feb 25, 2020

bajtos Feb 25, 2020

nabdelgadir Feb 27, 2020

bajtos Feb 27, 2020

bajtos Feb 25, 2020

agnes512 left a comment

agnes512 Feb 25, 2020

bajtos Feb 27, 2020

agnes512 Feb 25, 2020

bajtos Feb 27, 2020

jannyHou left a comment

jannyHou Feb 27, 2020

nabdelgadir left a comment

nabdelgadir Feb 27, 2020

feat(cli): import LB3 models with a custom base class #4737

feat(cli): import LB3 models with a custom base class #4737

Conversation

bajtos commented Feb 25, 2020 • edited Loading

Checklist

bajtos commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agnes512 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 25, 2020 •

edited

Loading