feat(repository-json-schema): add an option to make properties optional #3309

nabdelgadir · 2019-07-05T18:18:57Z

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 👈

jannyHou · 2019-07-08T16:06:47Z

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

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


@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?

I think TS 3.5.x defaults to unknown.

raymondfeng · 2019-07-08T19:16:59Z

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

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


Would it be simpler to have the title as:

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

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.

bajtos · 2019-07-09T11:40:05Z

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

@@ -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', () => {


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', () => {

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.

bajtos · 2019-07-09T11:40:33Z

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

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


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

bajtos · 2019-07-09T11:42:19Z

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

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


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.

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.

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?

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

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.

raymondfeng · 2019-07-12T21:37:38Z

packages/repository-json-schema/package.json

-    "@types/json-schema": "^7.0.3"
+    "@types/json-schema": "^7.0.3",
+    "debug": "^4.1.1",
+    "@types/debug": "^4.1.4"


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

raymondfeng · 2019-07-12T21:39:21Z

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

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


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`

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

bajtos

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.

bajtos · 2019-07-16T08:17:55Z

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

+        expect(optionalNothingSchema.title).to.equal('Product');
+      });
+
+      it('overrides "partial" option when "optional" options is set', () => {


nabdelgadir requested review from bajtos and raymondfeng as code owners July 5, 2019 18:18

nabdelgadir self-assigned this Jul 5, 2019

nabdelgadir force-pushed the optional-model-schema branch from 48efa13 to e11e73a Compare July 5, 2019 19:42

nabdelgadir added feature OpenAPI labels Jul 8, 2019

jannyHou reviewed Jul 8, 2019

View reviewed changes

raymondfeng reviewed Jul 8, 2019

View reviewed changes

bajtos reviewed Jul 9, 2019

View reviewed changes

nabdelgadir force-pushed the optional-model-schema branch 2 times, most recently from d83d9e6 to 4cbaafd Compare July 11, 2019 17:44

bajtos mentioned this pull request Jul 12, 2019

Emit schema excluding certain model properties #2653

Closed

9 tasks

raymondfeng reviewed Jul 12, 2019

View reviewed changes

raymondfeng approved these changes Jul 12, 2019

View reviewed changes

nabdelgadir force-pushed the optional-model-schema branch from 4cbaafd to d28bb56 Compare July 15, 2019 15:22

nabdelgadir added 2 commits July 15, 2019 11:32

feat(repository-json-schema): add an option to make properties optional

83f645b

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

fix(example-todo-list): make foreign key optional in request body

0e383fc

nabdelgadir force-pushed the optional-model-schema branch from d28bb56 to 0e383fc Compare July 15, 2019 15:33

nabdelgadir merged commit 363a4b5 into master Jul 15, 2019

delete-merged-branch bot deleted the optional-model-schema branch July 15, 2019 15:54

bajtos reviewed Jul 16, 2019

View reviewed changes

nabdelgadir mentioned this pull request Jul 16, 2019

Update API Docs and test to show that "optional" takes precedence over "partial" #3370

Merged

7 tasks

raymondfeng mentioned this pull request Aug 5, 2019

fix(repository-json-schema): update to pass openapi validation in azure #3504

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(repository-json-schema): add an option to make properties optional #3309

feat(repository-json-schema): add an option to make properties optional #3309

nabdelgadir commented Jul 5, 2019 •

edited

Loading

jannyHou Jul 8, 2019

raymondfeng Jul 8, 2019

raymondfeng Jul 8, 2019

bajtos Jul 9, 2019

bajtos Jul 9, 2019

nabdelgadir Jul 9, 2019

bajtos Jul 9, 2019

bajtos Jul 9, 2019

nabdelgadir Jul 9, 2019 •

edited

Loading

raymondfeng Jul 9, 2019

nabdelgadir Jul 9, 2019

bajtos Jul 16, 2019

raymondfeng Jul 12, 2019

raymondfeng Jul 12, 2019

bajtos left a comment

bajtos Jul 16, 2019

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

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

feat(repository-json-schema): add an option to make properties optional #3309

feat(repository-json-schema): add an option to make properties optional #3309

Conversation

nabdelgadir commented Jul 5, 2019 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabdelgadir Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabdelgadir commented Jul 5, 2019 •

edited

Loading

nabdelgadir Jul 9, 2019 •

edited

Loading