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): add an option to make properties optional #3309

Merged
merged 2 commits into from Jul 15, 2019

Conversation

@nabdelgadir
Copy link
Contributor

commented Jul 5, 2019

Add a new option optional: [] so that getJsonSchema and related helpers can request a model schema that marks specified properties as optional

Connect to #2653

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 5, 2019

@nabdelgadir nabdelgadir self-assigned this Jul 5, 2019

@nabdelgadir nabdelgadir force-pushed the optional-model-schema branch from 48efa13 to e11e73a Jul 5, 2019

}

it('makes one property optional when the option "optional" includes one property', () => {
const originalSchema = getJsonSchema(Product);

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 8, 2019

Contributor

@nabdelgadir A question for the generic type T in this PR: if you don't specify the type when calling getJsonSchema(ModelCtor), then where is it provided?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 8, 2019

Member

I think TS 3.5.x defaults to unknown.

let suffix = '';
if (options.partial) {
suffix += 'Partial';
}
if (options.optional && options.optional.length) {
suffix += 'Optional[' + options.optional + ']';
}
if (options.includeRelations) {
suffix += 'WithRelations';
}

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 8, 2019

Member

Would it be simpler to have the title as:

<originalModelName>:<stringified-json-for-options>

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

Please note the suffix is used in JSON/OpenAPI Schema titles. These titles are later used by code generators like swagger-codegen to emit interface or class names in TypeScript/Java/C#/etc.

While it may be easier for us to build the prefix as stringified json, I believe it will provide suboptimal experience for our users and anybody consuming OpenAPI spec generated by LB4 applications.

@@ -232,13 +232,27 @@ describe('build-schema', () => {
expect(key).to.equal('modelPartial');
});

it('returns concatenated option names otherwise', () => {
it('returns "optional[id,_rev]" when a single option "optional" is set', () => {

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

The test name is misleading, you are setting two properties as optional.

Suggested change
it('returns "optional[id,_rev]" when a single option "optional" is set', () => {
it('returns "optional[id,_rev]" when "optional" is set with two items', () => {

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jul 9, 2019

Author Contributor

So this is supposed to mean only the optional option is set (similar to the partial test above it: returns "partial" when a single option "partial" is set'), but I'll reword this one.

const key = buildModelCacheKey({
// important: object keys are defined in reverse order

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

Please preserve this comment in the test it('returns concatenated option names otherwise', () => {

it('returns concatenated option names otherwise', () => {
const key = buildModelCacheKey({
partial: true,
optional: ['id', '_rev'],

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

When partial is set to true, the optional config is effectively ignored, because all properties are optional.

Should we tell the user that they are may be doing something wrong? Personally, I would throw an error if both partial and optional are set.

Regardless of the solution we choose, please add a test to verify what happens when both partial and optional are set.


On Slack, we discussed an alternative solution: instead of adding optional?: (keyof T)[];, we can modify partial to accept boolean | keyof T)[]. This removes the edge case where both optional and partial are set. My concern was that this solution may be more difficult to understand for our users.

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jul 9, 2019

Author Contributor

The reason I opted to use optional is because I felt the name partial might be confusing. If you set partial to [prop1, prop2] then it might make it seem like these are the only two properties required and the rest are optional. What do you think?

I'll modify the test based on the approach.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 9, 2019

Member

I agree with @nabdelgadir that partial: ['a', 'b'] can be misleading.

IMO, partial: true becomes a shortcut of optional to mark all properties optional. My intuitive interpretation is the more specific one wins, i.e., optional: ['a', 'b'] should override partial: true. WDYT?

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jul 9, 2019

Author Contributor

I agree. I think optional should override partial as well.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 16, 2019

Member

I agree. I think optional should override partial as well.

Makes sense 馃憤

I feel it's important to describe this rule in (API) docs and have a test to verify the behavior.

@nabdelgadir nabdelgadir force-pushed the optional-model-schema branch 2 times, most recently from d83d9e6 to 4cbaafd Jul 11, 2019

@bajtos bajtos referenced this pull request Jul 12, 2019
9 of 9 tasks complete
"@types/json-schema": "^7.0.3"
"@types/json-schema": "^7.0.3",
"debug": "^4.1.1",
"@types/debug": "^4.1.4"

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 12, 2019

Member

Nitpick: it's good enough to be a dev dep.

);
});

it('doesn\'t make properties optional when the option "optional" includes no properties', () => {

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 12, 2019

Member

Nitpick: we can use ` to quote the string to avoid escaping, such as:

`doesn't make properties optional when the option "optional" includes no properties`

@nabdelgadir nabdelgadir force-pushed the optional-model-schema branch from 4cbaafd to d28bb56 Jul 15, 2019

nabdelgadir added some commits Jul 5, 2019

feat(repository-json-schema): add an option to make properties optional
Add a new option `optional: []` so that `getJsonSchema` and related helpers can request a model schema that marks specified properties as optional

@nabdelgadir nabdelgadir force-pushed the optional-model-schema branch from d28bb56 to 0e383fc Jul 15, 2019

@nabdelgadir nabdelgadir merged commit 363a4b5 into master Jul 15, 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.02%) to 91.521%
Details

@delete-merged-branch delete-merged-branch bot deleted the optional-model-schema branch Jul 15, 2019

@bajtos
Copy link
Member

left a comment

I agree. I think optional should override partial as well.

Makes sense 馃憤

I feel it's important to describe this rule in (API) docs and have a test to verify the behavior.

@nabdelgadir I see that you have included the test, that's great!

Can you please update the API docs to describe the precedence rule for partial and optional too? I think it would be best to mention it in API docs for both options.

expect(optionalNothingSchema.title).to.equal('Product');
});

it('overrides "partial" option when "optional" options is set', () => {

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 16, 2019

Member

馃憤

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.