feat(repository-json-schema): enhance getJsonSchema for navig props #2975

samarpanB · 2019-05-27T13:31:49Z

Enhance getJsonSchema to describe navigational properties

fix #2630

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 👈

samarpanB · 2019-05-27T13:32:56Z

@bajtos Can you please do a quick review and see if I am on right track ? Its a WIP PR. I still need to add new tests.

bajtos · 2019-05-31T14:22:28Z

@samarpanB thank you for the pull request, the changes look reasonable so far. I think you will need to add code setting targetsMany property to all relational decorators, see e.g. https://github.com/strongloop/loopback-next/pull/2592/files#diff-64c2bbb6c63151367ef935def7eb54da

examples/log-extension/src/__tests__/acceptance/log.extension.acceptance.ts

examples/log-extension/src/__tests__/unit/providers/log-action.provider.unit.ts

samarpanB · 2019-06-03T10:21:52Z

@bajtos I updated the PR with all the changes and test cases also added now.

Just a note. I kind of struggled a lot this time to run the tests. 'npm test' kept on failing. First, it was due to missing deps. So, I ran 'npm ci'. But it also failed due to access issues. I am on a linux machine. After a long search/hit-and-trial, I figured that it was not working properly when I was using my own user in the machine. When I switched to root user, it worked perfectly. Do you think this is something we should document somewhere for contributors to save some time in troubleshooting ?

samarpanB · 2019-06-03T12:12:14Z

@bajtos I am not sure why the build is failing. It is showing permission errors. I think that's unrelated.

bajtos · 2019-06-04T07:39:07Z

Just a note. I kind of struggled a lot this time to run the tests. 'npm test' kept on failing. First, it was due to missing deps. So, I ran 'npm ci'.

Yes, you need to run npm ci to install dependencies before you can run npm test, see https://loopback.io/doc/en/contrib/code-contrib-lb4.html#building-the-project

But it also failed due to access issues. I am on a linux machine. After a long search/hit-and-trial, I figured that it was not working properly when I was using my own user in the machine. When I switched to root user, it worked perfectly. Do you think this is something we should document somewhere for contributors to save some time in troubleshooting ?

I find that really weird, to be honest. Did you perhaps run git clone under root user? I am not aware of any dependencies that would require global install or root privileges. npm ci is supposed to install all dependencies locally, inside your checked-out repository.

Besides running npm install, we are also running lerna to create symlinks between monorepo packages. Is it perhaps possible that these symlinks cannot be created?

Anyhow, it's hard for me to help you without seeing the particular error logs. This is also very off topic in this pull request. Can you open a new issue please where we can take a closer look at the problems you are experiencing?

samarpanB · 2019-06-04T08:13:00Z

Just a note. I kind of struggled a lot this time to run the tests. 'npm test' kept on failing. First, it was due to missing deps. So, I ran 'npm ci'.

Yes, you need to run npm ci to install dependencies before you can run npm test, see https://loopback.io/doc/en/contrib/code-contrib-lb4.html#building-the-project

But it also failed due to access issues. I am on a linux machine. After a long search/hit-and-trial, I figured that it was not working properly when I was using my own user in the machine. When I switched to root user, it worked perfectly. Do you think this is something we should document somewhere for contributors to save some time in troubleshooting ?

I find that really weird, to be honest. Did you perhaps run git clone under root user? I am not aware of any dependencies that would require global install or root privileges. npm ci is supposed to install all dependencies locally, inside your checked-out repository.

Besides running npm install, we are also running lerna to create symlinks between monorepo packages. Is it perhaps possible that these symlinks cannot be created?

Anyhow, it's hard for me to help you without seeing the particular error logs. This is also very off topic in this pull request. Can you open a new issue please where we can take a closer look at the problems you are experiencing?

Yes I agree it's off-topic. I'll raise a separate issue if I encounter this again. I just wanted a heads up if I was doing something wrong. Thanks a lot for the direction @bajtos

bajtos

Looks mostly good, I have few smaller comments to address (or discuss).

I'd like @nabdelgadir to review this pull request too, as she has been recently involved with the work related to Inclusion of related models too.

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts

nabdelgadir

Great work! +1 to @bajtos's comments and I have a couple small ones.

Also I restarted the CI job so it's passing now. 👍

nabdelgadir · 2019-06-04T22:07:39Z

packages/repository/src/relations/relation.types.ts

+   * false for relations with a single target (e.g. BelongsTo, HasOne).
+   * This property is needed by OpenAPI/JSON Schema generator.
+   */
+  targetsMany?: boolean;


I think that this should be a required property like in the issue description. What do you think?

I kept it optional for backward compatibility. But if you think otherwise I can make this as required. Thoughts @bajtos ?

That's a good point; let's wait for @bajtos' thoughts, but can you please still add it to the relation definitions? e.g.:

export interface HasManyDefinition extends RelationDefinitionBase { type: RelationType.hasMany; targetsMany: true;

added to relation definitions.

I kept it optional for backward compatibility.

Good point!

I agree that making targetsMany a required property can break existing applications or 3rd-party extensions. On the other hand, I would be surprised if any such app or extension actually did exist.

Here is the important part: what is the expected behavior when targetsMany is not set? I don't think it's a good idea to default to false.

I am proposing the following:

Add a comment starting with TODO(semver-major) explaining that we should make targetsMany required in LB5

Modify the code reading targetsMany to throw a helpful error when targetsMany does not exist or is undefined. The most important place to change is in modelToJsonSchema, but there may be other places to update too.

Preferably add a unit test to verify that modeltoJsonSchema can handle missing targetsMany and demonstrate what happens in such case.

Modify the code reading targetsMany to throw a helpful error when targetsMany does not exist or is undefined

Don't you think throwing error in this case would also mean we are making it backward incompatible ?

As I understand it, the error will be thrown only when modeltoJsonSchema is called with the new option includeRelations. Since this option did not exist before, there shouldn't be any existing code setting it to true and thus there shouldn't be any code affected by the new error being thrown.

Hmmm. Got it. Let add some error handling and a test case to verify it as well. This makes sense.

Done. Please check now.

packages/repository-json-schema/src/keys.ts

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts

nabdelgadir

LGTM 👍

bajtos

I like you have refactored & extracted the logic determining schema of the navigational property into a new function 👍

Let's find a better name for it please.

The rest LGTM.

bajtos · 2019-06-07T14:38:20Z

packages/repository-json-schema/src/build-schema.ts

+ * @param relMeta Relation metadata object
+ * @param targetRef Schema definition for the target model
+ */
+export function getPropDefForRelatedModel(


We prefer to use full names instead of abbreviations.

How about the following?

export function getNavigationalPropertyForRelation( relationMeta: RelationMetadata, targetSchema: JSONSchema ): JSONSchema { // ... }

Yeah. That's better. Changing it. Will rebase and squash as well.

…ational properties Enhance `getJsonSchema` to describe navigational properties fix loopbackio#2630

bajtos

👏

nabdelgadir · 2019-06-10T15:07:32Z

Thanks for the contribution @samarpanB! It's now landed 👏

samarpanB requested review from bajtos and raymondfeng as code owners May 27, 2019 13:31

dhmlau added the community-contribution label May 27, 2019

bajtos self-assigned this May 31, 2019

bajtos added OpenAPI Relations Model relations (has many, etc.) labels May 31, 2019

samarpanB force-pushed the getjsonschema_navig_prop branch from 4a1bb79 to 08d6a7a Compare June 3, 2019 10:11

samarpanB commented Jun 3, 2019

View reviewed changes

examples/log-extension/src/__tests__/acceptance/log.extension.acceptance.ts Outdated Show resolved Hide resolved

samarpanB commented Jun 3, 2019

View reviewed changes

examples/log-extension/src/__tests__/unit/providers/log-action.provider.unit.ts Outdated Show resolved Hide resolved

samarpanB force-pushed the getjsonschema_navig_prop branch from 08d6a7a to ed92393 Compare June 3, 2019 11:47

samarpanB force-pushed the getjsonschema_navig_prop branch from ed92393 to a5c95db Compare June 4, 2019 07:21

samarpanB force-pushed the getjsonschema_navig_prop branch from a5c95db to c1ebc12 Compare June 4, 2019 08:42

bajtos reviewed Jun 4, 2019

View reviewed changes

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts Show resolved Hide resolved

packages/repository-json-schema/src/__tests__/integration/build-schema.integration.ts Outdated Show resolved Hide resolved

bajtos assigned nabdelgadir Jun 4, 2019

bajtos requested a review from nabdelgadir June 4, 2019 11:38

nabdelgadir reviewed Jun 4, 2019

View reviewed changes

samarpanB force-pushed the getjsonschema_navig_prop branch 3 times, most recently from 9f0f042 to 82a1a9e Compare June 6, 2019 19:31

nabdelgadir approved these changes Jun 7, 2019

View reviewed changes

bajtos reviewed Jun 7, 2019

View reviewed changes

samarpanB force-pushed the getjsonschema_navig_prop branch 3 times, most recently from 8f5ed7d to 9baf86a Compare June 9, 2019 17:44

samarpanB force-pushed the getjsonschema_navig_prop branch from 9baf86a to bf0caaf Compare June 10, 2019 13:55

feat(repository-json-schema): enhance getJsonSchema to describe navig…

ef23aaa

…ational properties Enhance `getJsonSchema` to describe navigational properties fix loopbackio#2630

samarpanB force-pushed the getjsonschema_navig_prop branch from bf0caaf to ef23aaa Compare June 10, 2019 13:57

bajtos approved these changes Jun 10, 2019

View reviewed changes

nabdelgadir merged commit 7801f59 into loopbackio:master Jun 10, 2019

feat(repository-json-schema): enhance getJsonSchema for navig props #2975

feat(repository-json-schema): enhance getJsonSchema for navig props #2975

Uh oh!

Conversation

samarpanB commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

samarpanB commented May 27, 2019

Uh oh!

bajtos commented May 31, 2019

Uh oh!

Uh oh!

Uh oh!

samarpanB commented Jun 3, 2019

Uh oh!

samarpanB commented Jun 3, 2019

Uh oh!

bajtos commented Jun 4, 2019

Uh oh!

samarpanB commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nabdelgadir 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabdelgadir Jun 5, 2019 • 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!

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

Uh oh!

Uh oh!

Uh oh!

nabdelgadir left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

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!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

nabdelgadir commented Jun 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samarpanB commented May 27, 2019 •

edited

Loading

samarpanB commented Jun 4, 2019 •

edited

Loading

nabdelgadir left a comment •

edited

Loading

nabdelgadir Jun 5, 2019 •

edited

Loading