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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Feb 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 0 additions & 18 deletions docs/site/Importing-LB3-models.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,24 +60,6 @@ _The tracking GitHub issue:

Workaround: define relational metadata & navigational properties manually.

### Models inheriting from custom base class

_The tracking GitHub issue:
[loopback-next#3813](https://github.com/strongloop/loopback-next/issues/3813)_

Models inheriting from application-specific models (including LB3 built-in
models like `User`) cannot be imported yet.

Workaround:

1. Modify your LB3 model to inherit from `Model`, `PersistedModel` or
`KeyValueModel`.

2. Import the model to LB4

3. Update the imported model to inherit for the desired application-specific
model.

### MongoDB's `ObjectID` type

The tracking GitHub issue:
Expand Down
26 changes: 26 additions & 0 deletions packages/cli/generators/import-lb3-models/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ module.exports = class Lb3ModelImporter extends BaseGenerator {
this.destinationPath(),
this.artifactInfo.outDir,
);
this.artifactInfo.modelDir = path.resolve(
this.artifactInfo.rootDir,
utils.modelsDir,
);
return super.setOptions();
}

Expand Down Expand Up @@ -99,6 +103,14 @@ Learn more at https://loopback.io/doc/en/lb4/Importing-LB3-models.html
this.modelNames = answers.modelNames;
}

async loadExistingLb4Models() {
debug(`model list dir ${this.artifactInfo.modelDir}`);
this.existingLb4ModelNames = await utils.getArtifactList(
this.artifactInfo.modelDir,
'model',
);
}

async migrateSelectedModels() {
if (this.shouldExit()) return;
this.modelFiles = [];
Expand Down Expand Up @@ -136,6 +148,20 @@ Learn more at https://loopback.io/doc/en/lb4/Importing-LB3-models.html
);
debug('LB4 model data', templateData);

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

!this.modelNames.includes(baseName)
) {
this.log(
'Adding %s (base of %s) to the list of imported models.',
chalk.yellow(baseName),
chalk.yellow(name),
);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

this.modelNames.push(baseName);
}
}
const fileName = utils.getModelFileName(name);
const fullTargetPath = path.resolve(this.artifactInfo.relPath, fileName);
debug('Model %s output file', name, fullTargetPath);
Expand Down
63 changes: 53 additions & 10 deletions packages/cli/generators/import-lb3-models/migrate-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const {
findBuiltinType,
} = require('../model/property-definition');
const chalk = require('chalk');
const {isDeepStrictEqual} = require('util');

module.exports = {
importLb3ModelDefinition,
Expand All @@ -43,12 +44,29 @@ function importLb3ModelDefinition(modelCtor, log) {

logNamingIssues(modelName, log);

const baseDefinition = modelCtor.base.definition;
const baseProps = {...baseDefinition.properties};

// 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

delete baseProps.id;
}

const templateData = {
name: modelName,
className: pascalCase(modelName),
...migrateBaseClass(modelCtor.settings.base),
properties: migrateModelProperties(modelCtor.definition.properties),
settings: migrateModelSettings(modelCtor.definition.settings, log),
properties: migrateModelProperties(
modelCtor.definition.properties,
baseProps,
),
settings: migrateModelSettings(
modelCtor.definition.settings,
baseDefinition.settings,
log,
),
};

const settings = templateData.settings;
Expand All @@ -59,7 +77,7 @@ function importLb3ModelDefinition(modelCtor, log) {
return templateData;
}

function migrateModelProperties(properties) {
function migrateModelProperties(properties = {}, inherited = {}) {
const templateData = {};

// In LB 3.x, primary keys are typically contributed by connectors later in
Expand All @@ -72,7 +90,16 @@ function migrateModelProperties(properties) {
});

for (const prop in properties) {
const def = migratePropertyDefinition(properties[prop]);
const propDef = properties[prop];

// Skip the property if it was inherited from the base model (the parent)
Copy link
Contributor

Choose a reason for hiding this comment

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

reasonable to me 👍

const baseProp = inherited[prop];
if (baseProp && isDeepStrictEqual(propDef, baseProp)) {
delete templateData[prop];
continue;
}

const def = migratePropertyDefinition(propDef);
templateData[prop] = createPropertyTemplateData(def);
}

Expand Down Expand Up @@ -132,21 +159,30 @@ function migrateBaseClass(base) {
};
}

// TODO: handle inheritance from application models
throw new Error(
'Models inheriting from app-specific models cannot be migrated yet. ' +
`Base model configured: ${baseModelName}`,
);
return {
modelBaseClass: baseModelName,
isModelBaseBuiltin: false,
};
}

function migrateModelSettings(settings = {}, log) {
function migrateModelSettings(settings = {}, inherited = {}, log) {
// Shallow-clone the object to prevent modification of external data
settings = {...settings};

// "strict" mode is enabled only when explicitly requested
// LB3 models allow additional properties by default
settings.strict = settings.strict === true;

// Remove settings inherited from the base model
for (const key in inherited) {
// Always emit the value of 'strict' setting, make it explicit
if (key === 'strict') continue;

if (isDeepStrictEqual(settings[key], inherited[key])) {
delete settings[key];
}
}

if (settings.forceId === 'auto') {
// The value 'auto' is used when a parent model wants to let the child
// model make the decision automatically, depending on whether the child
Expand Down Expand Up @@ -204,3 +240,10 @@ function migrateModelSettings(settings = {}, log) {

return settings;
}

function isCoreModel(modelCtor) {
const name = modelCtor.modelName;
return (
name === 'Model' || name === 'PersistedModel' || name === 'KeyValueModel'
);
}
2 changes: 1 addition & 1 deletion packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@types/ejs": "^3.0.1",
"@types/node": "^10.17.16",
"glob": "^7.1.6",
"loopback": "^3.26.0",
"loopback-datasource-juggler": "^4.18.1",
"mem-fs": "^1.1.3",
"mem-fs-editor": "^6.0.0",
Expand All @@ -50,7 +51,6 @@
"json5": "^2.1.1",
"latest-version": "^5.1.0",
"lodash": "^4.17.15",
"loopback": "^3.26.0",
"minimist": "^1.2.0",
"mkdirp": "^1.0.3",
"natural-compare": "^1.4.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,7 @@ exports[`lb4 import-lb3-models imports CoffeeShop model from lb3-example app 1`]
import {Entity, model, property} from '@loopback/repository';

@model({
settings: {
strict: false,
forceId: false,
replaceOnPUT: true,
validateUpsert: true,
idInjection: true
}
settings: {strict: false, forceId: false, validateUpsert: true, idInjection: true}
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

})
export class CoffeeShop extends Entity {
@property({
Expand Down Expand Up @@ -62,3 +56,140 @@ exports[`lb4 import-lb3-models imports CoffeeShop model from lb3-example app 2`]
export * from './coffee-shop.model';

`;


exports[`lb4 import-lb3-models imports a model inheriting from a custom base class 1`] = `
import {model, property} from '@loopback/repository';
import {UserBase} from '.';

@model({settings: {strict: false, customCustomerSetting: true}})
export class Customer extends UserBase {
@property({
type: 'boolean',
})
isPreferred?: boolean;

// Define well-known properties here

// Indexer property to allow additional data
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[prop: string]: any;

constructor(data?: Partial<Customer>) {
super(data);
}
}

export interface CustomerRelations {
// describe navigational properties here
}

export type CustomerWithRelations = Customer & CustomerRelations;

`;


exports[`lb4 import-lb3-models imports a model inheriting from a custom base class 2`] = `
import {model, property} from '@loopback/repository';
import {User} from '.';

@model({settings: {strict: false, customUserBaseSetting: true}})
export class UserBase extends User {
@property({
type: 'boolean',
required: true,
default: false,
})
isAccountVerified: boolean;

// Define well-known properties here

// Indexer property to allow additional data
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[prop: string]: any;

constructor(data?: Partial<UserBase>) {
super(data);
}
}

export interface UserBaseRelations {
// describe navigational properties here
}

export type UserBaseWithRelations = UserBase & UserBaseRelations;

`;


exports[`lb4 import-lb3-models imports a model inheriting from a custom base class 3`] = `
import {Entity, model, property} from '@loopback/repository';

@model({
settings: {
strict: false,
caseSensitiveEmail: true,
hidden: ['password', 'verificationToken'],
maxTTL: 31556926,
ttl: 1209600
}
})
export class User extends Entity {
@property({
type: 'number',
id: 1,
generated: true,
updateOnly: true,
})
id?: number;

@property({
type: 'string',
})
realm?: string;

@property({
type: 'string',
})
username?: string;

@property({
type: 'string',
required: true,
})
password: string;

@property({
type: 'string',
required: true,
})
email: string;

@property({
type: 'boolean',
})
emailVerified?: boolean;

@property({
type: 'string',
})
verificationToken?: string;

// Define well-known properties here

// Indexer property to allow additional data
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[prop: string]: any;

constructor(data?: Partial<User>) {
super(data);
}
}

export interface UserRelations {
// describe navigational properties here
}

export type UserWithRelations = User & UserRelations;

`;
Loading