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

Emit schema excluding certain model properties #3297

Merged
merged 6 commits into from Jul 10, 2019

Conversation

@nabdelgadir
Copy link
Contributor

commented Jul 2, 2019

See #2653.

  • Add exclude option to emit model schema excluding certain model properties.
  • Update POST in example applications to exclude id.
    • Update their docs for consistency.
  • Update the CLI templates to also exclude id from POST request body.

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 馃憟

@nabdelgadir nabdelgadir requested review from bajtos and raymondfeng as code owners Jul 2, 2019

@nabdelgadir nabdelgadir self-assigned this Jul 2, 2019

@nabdelgadir nabdelgadir force-pushed the exclude-model-schema branch from 94295c5 to d8befdd Jul 2, 2019

@bajtos
Copy link
Member

left a comment

Great patch! Let's improve few details please.

@nabdelgadir nabdelgadir force-pushed the exclude-model-schema branch from d8befdd to 969c5fe Jul 4, 2019

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

@nabdelgadir nabdelgadir force-pushed the exclude-model-schema branch 2 times, most recently from b1975d2 to 0ea2feb Jul 5, 2019

@bajtos

bajtos approved these changes Jul 8, 2019

Copy link
Member

left a comment

LGTM with a small comment.

@nabdelgadir nabdelgadir force-pushed the exclude-model-schema branch from 0ea2feb to db19455 Jul 8, 2019

@jannyHou
Copy link
Contributor

left a comment

@nabdelgadir The fix looks reasonable in general 馃憤 Please note a model's id name is not always id, therefore we cannot hardcode the property name. You can get the id name by calling method like

const idName = this.definition.idName();

@@ -55,7 +55,13 @@ export class <%= controllerClassName %> {
})
async create(
@param.path.<%= sourceModelPrimaryKeyType %>('id') id: typeof <%= sourceModelClassName %>.prototype.<%= sourceModelPrimaryKey %>,
@requestBody() <%= targetModelRequestBody %>: <%= targetModelClassName %>,
@requestBody({

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 8, 2019

Contributor

The id field is not always called id, I think we need to get a model's id name in the template.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

Good point. AFAICT, lb4 model does allow users to create a primary key property with a different name than id.

Example prompts & answers to use pk instead of id:

Model Product will be created in src/models/product.model.ts

Let's add a property to Product
Enter an empty property name when done

? Enter the property name: pk
? Property type: string
? Is pk the ID property? Yes
? Is it required?: No
? Default value [leave blank for none]:

I agree with @jannyHou that we should honor custom PK name, there should be a new test to verify. If this is too much work then I can live with adding support for custom PK names in a follow-up pull request.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 9, 2019

Contributor

If this is too much work then I can live with adding support for custom PK names in a follow-up pull request.

Yeah forgot to say, feel free to address it in another PR :)

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jul 10, 2019

Author Contributor

Will do in a separate PR 馃憤

},
},
})
todoList: Omit<TodoList, 'id'>,

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 8, 2019

Member

Should we support include for todoList: Pick<TodoList, 'id'>?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 8, 2019

Member

I'm fine to do it in a separate PR.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

I am not sure what would you use include for? A kind of a white-list? That would make sense 馃憤

Let's discuss include and Pick in a separate PR. I think it's best to wait until there is (user) demand for that feature.

nabdelgadir added some commits Jul 2, 2019

feat(repository-json-schema): add an option to exclude properties fro鈥
鈥 schema

Add a new option `exclude: []` so that `getJsonSchema` and related helpers can request a model schema that excludes specified properties

@nabdelgadir nabdelgadir force-pushed the exclude-model-schema branch from db19455 to ffd4d40 Jul 10, 2019

@nabdelgadir nabdelgadir merged commit 4c1ce67 into master Jul 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.008%) to 91.493%
Details

@delete-merged-branch delete-merged-branch bot deleted the exclude-model-schema branch Jul 10, 2019

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