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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 10, 2019

Conversation

Projects
None yet
4 participants
@samarpanB
Copy link
Contributor

commented May 27, 2019

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 samarpanB requested review from bajtos and raymondfeng as code owners May 27, 2019

@samarpanB

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 31, 2019

@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

@bajtos bajtos self-assigned this May 31, 2019

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch from 4a1bb79 to 08d6a7a Jun 3, 2019

@samarpanB

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@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 samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch from 08d6a7a to ed92393 Jun 3, 2019

@samarpanB

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

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

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch from ed92393 to a5c95db Jun 4, 2019

@bajtos

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

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

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch from a5c95db to c1ebc12 Jun 4, 2019

@bajtos
Copy link
Member

left a comment

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.

@bajtos bajtos requested a review from nabdelgadir Jun 4, 2019

@nabdelgadir
Copy link
Contributor

left a comment

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. 馃憤

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

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 4, 2019

Contributor

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

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 5, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 5, 2019

Contributor

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;

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 5, 2019

Author Contributor

added to relation definitions.

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 6, 2019

Member

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.

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 6, 2019

Member

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

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 6, 2019

Author Contributor

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 ?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 6, 2019

Member

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.

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 6, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 6, 2019

Author Contributor

Done. Please check now.

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

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch 3 times, most recently from 9f0f042 to 82a1a9e Jun 5, 2019

@nabdelgadir
Copy link
Contributor

left a comment

LGTM 馃憤

@bajtos
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 7, 2019

Member

We prefer to use full names instead of abbreviations.

How about the following?

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

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 7, 2019

Author Contributor

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

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch 3 times, most recently from 8f5ed7d to 9baf86a Jun 7, 2019

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch from 9baf86a to bf0caaf Jun 10, 2019

feat(repository-json-schema): enhance getJsonSchema to describe navig鈥
鈥tional properties

Enhance `getJsonSchema` to describe navigational properties

fix #2630

@samarpanB samarpanB force-pushed the samarpanB:getjsonschema_navig_prop branch from bf0caaf to ef23aaa Jun 10, 2019

@bajtos

bajtos approved these changes Jun 10, 2019

Copy link
Member

left a comment

馃憦

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

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 91.765%
Details
@nabdelgadir

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.