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

Refactor metaToJsonProperty to accept AJV keywords #2685

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Refactor metaToJsonProperty to accept AJV keywords #2685

merged 1 commit into from
Apr 8, 2019

Conversation

iqbaldjulfri
Copy link
Contributor

@iqbaldjulfri iqbaldjulfri commented Apr 3, 2019

We need to add additional attributes (AJV keywords) from PropertyDefinition so they will be written to openapi.json and we can use it as validation. Currently, only type, description, items (for arrays), and $ref are written (and a hard-coded format: 'date-time' for Date instances).

issue #2684

See also #1624

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

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@iqbaldjulfri Thank you for creating PR to preserve the property metadata in the JSON schema!

I left a comment regarding the blacklist of properties get removed. Please take a look thanks.

packages/repository-json-schema/src/build-schema.ts Outdated Show resolved Hide resolved
@raymondfeng
Copy link
Contributor

@iqbaldjulfri Please fix the CI failures per instructions at https://loopback.io/doc/en/lb4/submitting_a_pr.html.

@iqbaldjulfri
Copy link
Contributor Author

@raymondfeng All CI errors fixed, I need to force push to change the earlier commit message.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Thank you @iqbaldjulfri for the pull request. Please read the discussion in #1624 and #1872 to learn about the wider context of property validations.

TL;DR summary: validation rules that are implemented at JSON/OpenAPI level and not enforced at ORM/database level should be nested under a new property jsonSchema. Cross-posting from #1624 (comment):

{
  type: string,
  minLength: 10,
  // the LDL extension: jsonSchema
  jsonSchema: {
    // JSON-Schema constraints follow
    format: 'email'
  }
}

@bajtos bajtos self-assigned this Apr 5, 2019
@bajtos bajtos added feature OpenAPI REST Issues related to @loopback/rest package and REST transport in general labels Apr 5, 2019
@iqbaldjulfri
Copy link
Contributor Author

iqbaldjulfri commented Apr 5, 2019

@bajtos thank you for the insight on the bigger picture of this problem. I've refactored the code to use jsonSchema as you suggested. With this we can leverage ajv validations in the OpenAPI layer.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Lovely, I find the new code much simpler and more elegant ❤️

Please address my comment below and squash all commits into a single one, using a descriptive commit message - see https://loopback.io/doc/en/contrib/code-contrib-lb4.html#commit-message-guidelines.

In case you are not familiar with git history manipulation, you can learn the git commands here: https://loopback.io/doc/en/lb4/submitting_a_pr.html#6-rebasing


it('keeps AJV keywords', () => {
expect(
metaToJsonProperty({
Copy link
Member

@bajtos bajtos Apr 5, 2019

Choose a reason for hiding this comment

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

This statement is way too long to my taste, it also combines two parts of a test (act + assert) into a single line, which can make the test more difficult to understand. Let's split this into two statements please.

const schema = metaToJsonProperty({
  // ...
});

expect(schema).to.eql({
  // ...
});

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍 The solution that adds a nested property jsonSchema LGTM

…ustom jsonSchema

currently there is no way to use AJV's validation keywords. This commit will make metaToJsonProperty reads the jsonSchema and add it as an additional schema to be used by AJV validation on OpenAPI layer.
@iqbaldjulfri
Copy link
Contributor Author

@bajtos, all done. I've implemented your suggestion for the unit test and squashed them into a single commit.

PS: Thank you guys for everything. This is my very first contribution to open source project and I'm very grateful to have you guiding a newcomer like me. Cheers!

@bajtos bajtos merged commit d0014c6 into loopbackio:master Apr 8, 2019
@bajtos
Copy link
Member

bajtos commented Apr 8, 2019

Landed. 🎉 Thank you @iqbaldjulfri for the contribution! ❤️

Thank you guys for everything. This is my very first contribution to open source project and I'm very grateful to have you guiding a newcomer like me.

You are very welcome 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature OpenAPI REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants