Model Discovery #2245
Model Discovery #2245
Conversation
There was a problem hiding this comment.
Thank you @marvinirwin for the pull request!
I am not very familiar with this part of our codebase, I'd like @raymondfeng and @jannyHou to review the changes.
@marioestradarosa IIRC, you were interested in generating LB4 models from database schema too, what is your opinion on the implementation proposed in this pull request?
|
@marvinirwin Great effort! Please see my comments. |
|
@raymondfeng @bajtos Is there anything else I can do to help this get merged? I think I've addressed all the reviewers' concerns. |
There was a problem hiding this comment.
@marvinirwin, I've added some comments mostly on the docs.
I'm thinking:
- to add the
lb4 discovercommand to the main cli docs page: https://loopback.io/doc/en/lb4/Command-line-interface.html, so that this page contains a complete list of cli commands that are available. - to add more test cases, e.g. the error case.
It can be in a follow-up PR.
|
@raymondfeng @dhmlau @bajtos Got anything else? |
|
@marvinirwin, thanks for updating the PR. I've added one minor comment. |
We don't merge branches and use rebase instead. That way the git history remains neatly linear. Could you please run |
There was a problem hiding this comment.
Great patch, @marvinirwin! Please see my comments below for things to improve.
@jannyHou @raymondfeng PTAL too
| `--schema`: Specify the schema which the datasource will find the models to | ||
| discover | ||
|
|
||
| `--schemaDefs`: The path to a `.json` file containing a model definition, like |
There was a problem hiding this comment.
Can we find a better name for this option, what that will convey the fact that we are loading models from JSON files instead of the database?
Few ideas to consider:
--import--import-definition--from-definition--from-file--from-lb3-models
Also please make mention that the JSON file can contain either a single model definition object or an array with multiple definitions.
There was a problem hiding this comment.
On the second thought, I am not sure if it's a good idea to mix lb4 discover with importing models from JSON definitions. I find it little bit confusing as a user, plus it makes the implementation more complex because of all those if (importing from file) return statements at the top of many generator steps.
Have you considered moving this functionality into lb4 model generator? I think it will give us CLI that feels pretty natural too.
lb4 model --import definition.json
# or perhaps
lb4 model --from-definition definition.jsonThere was a problem hiding this comment.
Yes it's dramatically more intuitive, I'll move it to the model generator.
Slightly related: The .json loading was used to give me a way of testing the model discovery, which I still have a bit of trouble doing thoroughly. A couple of solutions:
The memory datasource could be given discoverModelDefinitions and discoverSchema, but that might be out of the scope of this PR.
Docker could also be used in the unit tests, but that definitely out of scope of this PR
There was a problem hiding this comment.
@bajtos After looking around the code a bit I'm proposing keeping the --schemaDefs option. And that I submit a seperate PR for adding the --import option to the model generator.
The current discover --schemaDefs path accepts an array of objects, which mimics how the discover command discovers multiple models. I also need a way to mock data for the integration test.
There was a problem hiding this comment.
Let's discuss testing and user experience independently.
For testing, I think you should mock discoverModelDefinitions and/or discoverSchema in such way that your tests sees the model schema you are interested in. I'll try to find time to create a code snippet showing how to mock those two methods.
For user experience, I still prefer importing via lb4 model over --schemaDefs. I am fine with leaving this option out of this initial pull request, as long as --schemaDefs is removed too.
There was a problem hiding this comment.
I am thinking about the following solution for testing:
- Modify the datasource class used by your tests (
mem.datasource.js.txt) to override discovery methods provided byjuggler.DataSource. - In these overridden methods, load schema from
test-schema-defs.jsonand return the relevant parts depending on which discovery method was called.
A mock-up to illustrate what I mean:
export class MemDataSource extends juggler.DataSource {
async discoverSchema(name, options) {
await ensureSchemaDefsLoaded();
const schema = this.schemas.find(s => s.name === name);
if (schema) return schema;
throw new Error(`Schema not found: ${name}`);
}
private schemas;
private async ensureSchemaDefsLoaded() {
if (schemas) return;
const schemaFile = path.resolve(__dirname, 'test-schema-defs.json');
_schemas = JSON.parse(await fs.readFile(schemaFile, 'utf-8'));
}
} There was a problem hiding this comment.
I'm pretty embarrassed I didn't think of implementing my own DataSource. The testing is now much easier.
packages/cli/package-lock.json
Outdated
| { | ||
| "name": "@loopback/cli", | ||
| "version": "1.2.2", | ||
| "lockfileVersion": 1, |
There was a problem hiding this comment.
I believe we are intentionally not using package-lock files. Please remove it from the pull request.
There was a problem hiding this comment.
I still see the package-lock file included in the PR. PTAL again.
| @@ -0,0 +1,29 @@ | |||
| "use strict"; | |||
| var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) { | |||
There was a problem hiding this comment.
Hmm, keeping versioning JS files is not pretty. It makes me wonder how are tests for other generators (e.g. lb4 repository) dealing with datasource discovery? Could you PTAL and see if there is a way how to use an existing approach? Do we actually need this .js file? Isn't it enough to have datasource .json file?
There was a problem hiding this comment.
You're right, I'm going to try and do this with the legacy.Datasource constructor and a JSON file. The only downside I can see is that if any config is defined inserver/datasources/datasource.ts it won't be loaded properly
There was a problem hiding this comment.
I've tried this, but if I only use a .json file it will crash because the lb4 command doesn't have the connector. Is it worth it to try and load the connectors found in the discovering node_modules?
There was a problem hiding this comment.
Could you please create a new feature branch in your repo where you will commit the code showing what have you tried, so that I can run that version myself and better understand what's going on under the hood? I am afraid I am not able to build a good picture in my head just from our discussion.
There was a problem hiding this comment.
Compare loading from name.datasource.json, to loading name.datasource.js
(requires lb4 project with non-memory datasource in dist, it doesn't have to connect successfully)
git clone https://github.com/marvinirwin/loopback-next --single-branch --branch discovering-from-json .;
cd packages/cli;
npm run build;
npm link;
cd SOME_LB4_PROJECT;
# new DataSource(JSON.parse(fs.readFileSync(pathToJson).toString()))
# This should tell you to npm install the connector
LOAD_JSON_DATASOURCES=true lb4 discover;
# new require(pathToJavascript);
# This will not tell you to npm install the connector
LOAD_JSON_DATASOURCES= lb4 discover;
|
@marvinirwin I see that you pushed new commits. Is the patch ready for another review? Most of us are not receiving notifications when a new commit is pushed into a pull request, please post a comment when it's time to review the changes again. |
|
@bajtos New fixes
|
|
@bajtos New commit!
|
|
@bajtos I think I understand what you mean about the duplication of logic in loading datasources now.
Once these changes are implemented both the I'll begin that PR once this one has been merged. |
Sounds good. I noticed that our code coverage dropped by 0.4% with your patch in place, which is more than we usually accept. I reviewed the code coverage data, it looks like you pull request is missing tests for datasource prompt when discovering a single model (see e.g. https://coveralls.io/builds/22183916/source?filename=packages/cli/generators/discover/index.js)) and for the code mapping discovered types to TypeScript snippets (see https://coveralls.io/builds/22183916/source?filename=packages/cli/generators/model/index.js#L457). How easy or difficult it is to write tests covering these two areas? |
|
@bajtos Thanks for reminding me about the discovering single models. I added tests for that and some tests ensuring nicer error messages. I'm not sure about the loopback->typescript mapping parts. I think I can hit them all in a more exhaustive test, but I think that's out of the scope of this PR. |
There was a problem hiding this comment.
Thank you @marvinirwin for the improvements, the patch LGTM now.
@raymondfeng @jannyHou could you PTAL too?
If there are no objections then I'd like to proceed with landing this great feature early next week.
packages/cli/test/integration/generators/discover.integration.js
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,106 @@ | |||
| const DataSource = require('loopback-datasource-juggler').DataSource; | |||
There was a problem hiding this comment.
I would prefer to avoid direct dependency on loopback-datasource-juggler and import DataSource class constructor from @loopback/repository.
Not a big deal, we can improve this in a follow-up pull request.
There was a problem hiding this comment.
Just have a few comments but LGTM otherwise. Can you also add this command to the CLI README?
|
@marvinirwin could you please address the comments raised by @nabdelgadir above, and also add this new command to the CLI README? I'd like to get them fixed before we land this patch. |
|
@bajtos done |
You can run |
|
The first line of my output is not correct, but I'm not sure how to change it. (discover does not have a name parameter) |
@raymondfeng could you please help here? I think you now the internal CLI architecture best. Personally, I can live with incorrect help message for the time being, I see much more value in having the command |
|
@bajtos Alright I've put the mildly incorrect output from |
See https://travis-ci.org/strongloop/loopback-next/jobs/516258539. Your feature branch seems to contain a bunch of commits that are violating our Commit Message Guidelines. Could you please try to rebase your feature branch on top of the latest master? See https://loopback.io/doc/en/lb4/submitting_a_pr.html#6-rebasing We can help you with this last step if you like, just let us know. |
|
@bajtos Done, I think |
|
@marvinirwin one last thing, can you squash your commits into one commit? If you need any help, let us know (see here for reference, if needed). |
|
@slnode test please |
|
@marvinirwin I've rebased your branch and resolved some conflicts, PTAL and if you are satisfied, we can land your patch. |
|
LGTM |
|
Awwwwwesome, Finally 🎉 |
|
There're some changes we want to go in for the same release. Once that's approved, we'll release. Hopefully very soon 🤞 |
There was a problem hiding this comment.
@bajtos Is './dist/datasources' the default datasource directory and not './dist/src/datasources'?
Adds the command
lb4 discovercommand and its documentation.The command has a
schemaDefsoption (the path to a file which contains a schema definition, or array of lb3 schema definitions) which overrides everything and simply creates the corresponding typescript file. It's used for the integration test, and could also solve #2224It's a piece of #2090
Implements #1921
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated