feat: new rule to detect enum value that do not respect specified type #913

nulltoken · 2020-01-14T08:04:29Z

Fixes #570

Checklist

Tests added / updated
Docs added / updated

Does this PR introduce a breaking change?

Yes
No

nulltoken · 2020-01-15T08:17:42Z

@rossmcdonald Could you please take a look at this, please? It's not done yet, but this should be usable for a first round of tests.

If you could also run some local tests by checking this branch out and running yarn cli lint... against a couple files that would be amazing!

@P0lip I'd also be a taker for a review pass. FWIW, I foresee to enhance the testing of the function by throwing at it more combinations of formats and values.

src/functions/formatedEnum.ts

src/rulesets/oas/__tests__/formated-enums.ts

P0lip

I like it!
If you fix typos and add tests, we should be all set.

src/functions/formatedEnum.ts

src/functions/index.ts

src/functions/formatedEnum.ts

nulltoken · 2020-01-16T23:30:24Z

Just pushed an update to ease the coverage of detection of invalid format usage

src/functions/__tests__/formattedEnum.test.ts

P0lip

LMK once you are done

src/functions/formattedEnum.ts

philsturgeon · 2020-01-17T18:27:13Z

This might actually go above and beyond. The feature request was for type, but this is type and format. Format is notoriously vague and I worry we're opening ourselves up to a maintainability nightmare, because JSON Schema 2019-09 has several new formats, and we're moving away from format being a validation keyword.

Could we stick to type, and if people really want this in the future we can find a way to make it work with JSON Schema 2019-09 (and OAS 3.1)?

nulltoken · 2020-01-17T20:25:05Z

This might actually go above and beyond. The feature request was for type, but this is type and format. Format is notoriously vague and I worry we're opening ourselves up to a maintainability nightmare, because JSON Schema 2019-09 has several new formats, and we're moving away from format being a validation keyword.

So my attempt at properly narrowing the scope in #570 (comment) was eventually ... completely useless 😜

Could we stick to type, and if people really want this in the future we can find a way to make it work with JSON Schema 2019-09 (and OAS 3.1)?

No problem. I'll dumb it down and should have something ready by tomorrow evening.

rossmcdonald · 2020-01-17T21:10:05Z

Sorry @nulltoken, I read 'format' as 'type' in your comment, so I'm most likely to blame for the extra effort here.

After speaking with @philsturgeon, I also wasn't aware of how much 'format' was a can of worms when it comes to validation, so I agree limiting scope now will probably save us from a 'death by a thousand cuts' later as everyone has their own specific definition of each format.

nulltoken · 2020-01-17T21:33:35Z

@rossmcdonald No problem! I'm actually happy to reschedule all the life-lessons I could learn from 'formats' 😄

@philsturgeon I've pushed a simpler version. Does it fit you better?

BTW, although I've tried to be cautious, it's possible some "format" related things may still hide in the corners. If you spot some, please add a comment so that I can clean that up.

nulltoken · 2020-01-17T21:36:35Z

src/functions/__tests__/typedEnum.test.ts

+      ['integer', [-2147483648, 17], 12.3],
+      ['boolean', [true, false], 1],
+      // array ?
+      // object ?


@philsturgeon I now realize I've actually never met this use case.

It looks like (from my reading of both oas2 and oas3 specs) that an enum could also typed as an array or an object.

Do you confirm this point?

OAS 2 and 3 don't have any opinions of their own about what is or is not a valid instance for an enum, but, JSON Schema does: https://json-schema.org/draft/2019-09/json-schema-validation.html#enum

It says it can be used for anything, but I have never see anyone try to doenum: [[0, 1], [2,3]] and I don't think we want to waste our time figuring out how to make that happen right now. Let's stick to the cases you've mentioned here, and make sure we test no type (no type = any, so no error)

Let's stick to the cases you've mentioned here
@phil Done.

make sure we test no type (no type = any, so no error)
Covered by an additional test case

philsturgeon · 2020-01-17T23:24:28Z

docs/reference/functions.md

+
+## typedEnum
+
+Models described by both a `type` and an `enum` must only expose enum values that respect this type.


Suggested change

Models described by both a `type` and an `enum` must only expose enum values that respect this type.

When both a `type` and `enum` are defined for a property, the enum values must respect the type.

This makes me think this would be better off as a custom function for the oas ruleset. Afterall, this is oas/json schema specific, which means it doesn't really belong in core functions, right?

philsturgeon · 2020-01-17T23:24:58Z

docs/reference/functions.md

+
+name                   | description                                                        | required?
+-----------------------|--------------------------------------------------------------------|----------
+reportingThreshold     | the maximum number of invalid values reported in the error message | yes


This seems unique to this rule, whats the idea here?

philsturgeon · 2020-01-17T23:30:25Z

src/functions/__tests__/typedEnum.test.ts

+      ['integer', [-2147483648, 17], 12.3],
+      ['boolean', [true, false], 1],
+      // array ?
+      // object ?


OAS 2 and 3 don't have any opinions of their own about what is or is not a valid instance for an enum, but, JSON Schema does: https://json-schema.org/draft/2019-09/json-schema-validation.html#enum

It says it can be used for anything, but I have never see anyone try to doenum: [[0, 1], [2,3]] and I don't think we want to waste our time figuring out how to make that happen right now. Let's stick to the cases you've mentioned here, and make sure we test no type (no type = any, so no error)

philsturgeon · 2020-01-17T23:31:47Z

src/functions/__tests__/typedEnum.test.ts

+
+        expect(runTypedEnum(schema, threshold)).toEqual([
+          {
+            message: `Some enum values do not respect the specified type "integer": ${errorMessageTrail}`,


Could the error be reported for each invalid enum value instead of all of them? this might cut out the need for reportingThreshold.

@phil Fixed. Multiple messages are now returned and reportingThreahold has been shown the door.

philsturgeon · 2020-01-17T23:32:13Z

test-harness/scenarios/typed-enum.oas3.scenario

@@ -0,0 +1,33 @@
+====test====
+Identify enum values that do not respect the specified format


Suggested change

Identify enum values that do not respect the specified format

Identify enum values that do not respect the specified type

philsturgeon · 2020-01-17T23:32:28Z

src/functions/__tests__/typedEnum.test.ts

+  });
+
+  describe('basic', () => {
+    test('should return undefined when all enum values respect the format', () => {


Suggested change

test('should return undefined when all enum values respect the format', () => {

it('is undefined when all enum values respect the type', () => {

philsturgeon · 2020-01-17T23:32:36Z

src/functions/__tests__/typedEnum.test.ts

+      expect(runTypedEnum(schema, defaultReportingThreshold)).toBeUndefined();
+    });
+
+    test('should identify enum values that do not respect the format', () => {


Suggested change

test('should identify enum values that do not respect the format', () => {

it('identifies each enum value which does not respect the type', () => {

docs/reference/functions.md

package.json

P0lip

A few minor comments, pre-approving!

src/rulesets/oas/__tests__/typed-enum.ts

src/functions/typedEnum.ts

nulltoken · 2020-01-21T22:00:04Z

@P0lip All required changes have been fixed.

nulltoken · 2020-01-21T22:22:15Z

@philsturgeon It looks like the Check Markdown Links failed because it cannot resolve https://offset.earth/stoplightinc

Sweet irony!

And.. indeed, it's down

marbemac · 2020-01-21T23:31:38Z

Ha - looks like the offset earth site is back up - just kicked off a re-run of those checks. 🤞

nulltoken changed the title ~~feat: new rule to detect enum value that do not respect specified format~~ [WIP] feat: new rule to detect enum value that do not respect specified format Jan 14, 2020

nulltoken force-pushed the ntk/formated_enum branch 2 times, most recently from effcee9 to c194d2c Compare January 15, 2020 08:06

nulltoken requested review from chris-miaskowski and P0lip January 15, 2020 08:17

nulltoken commented Jan 15, 2020

View reviewed changes

src/functions/formatedEnum.ts Outdated Show resolved Hide resolved

nulltoken commented Jan 15, 2020

View reviewed changes

src/rulesets/oas/__tests__/formated-enums.ts Outdated Show resolved Hide resolved

P0lip reviewed Jan 15, 2020

View reviewed changes

src/functions/formatedEnum.ts Outdated Show resolved Hide resolved

src/functions/index.ts Outdated Show resolved Hide resolved

src/functions/formatedEnum.ts Outdated Show resolved Hide resolved

src/functions/formatedEnum.ts Outdated Show resolved Hide resolved

nulltoken force-pushed the ntk/formated_enum branch 2 times, most recently from 78e8b1e to 59e547d Compare January 16, 2020 23:27

nulltoken requested review from rossmcdonald and removed request for chris-miaskowski January 17, 2020 07:40

nulltoken commented Jan 17, 2020

View reviewed changes

src/functions/__tests__/formattedEnum.test.ts Outdated Show resolved Hide resolved

nulltoken mentioned this pull request Jan 17, 2020

Create OpenAPI rule for enums that don't match the attribute type #570

Closed

nulltoken force-pushed the ntk/formated_enum branch from f8f05d0 to c0d2d7e Compare January 17, 2020 08:11

P0lip reviewed Jan 17, 2020

View reviewed changes

src/functions/formattedEnum.ts Outdated Show resolved Hide resolved

src/functions/formattedEnum.ts Outdated Show resolved Hide resolved

nulltoken commented Jan 17, 2020

View reviewed changes

nulltoken force-pushed the ntk/formated_enum branch from 91dc358 to f452d6e Compare January 17, 2020 21:47

philsturgeon reviewed Jan 17, 2020

View reviewed changes

philsturgeon suggested changes Jan 17, 2020

View reviewed changes

philsturgeon changed the title ~~[WIP] feat: new rule to detect enum value that do not respect specified format~~ [WIP] feat: new rule to detect enum value that do not respect specified type Jan 17, 2020

nulltoken commented Jan 18, 2020

View reviewed changes

docs/reference/functions.md Outdated Show resolved Hide resolved

nulltoken commented Jan 18, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

nulltoken force-pushed the ntk/formated_enum branch from c9d55e2 to 1844f2b Compare January 19, 2020 12:35

nulltoken mentioned this pull request Jan 19, 2020

New dedupe in 5.0.0 prevents multiple rule violations to be reported from the same source range #920

Closed

nulltoken requested review from philsturgeon and P0lip January 20, 2020 23:03

nulltoken force-pushed the ntk/formated_enum branch 3 times, most recently from ad9ef43 to 4c197b5 Compare January 21, 2020 12:34

philsturgeon previously approved these changes Jan 21, 2020

View reviewed changes

nulltoken changed the title ~~[WIP] feat: new rule to detect enum value that do not respect specified type~~ feat: new rule to detect enum value that do not respect specified type Jan 21, 2020

P0lip previously approved these changes Jan 21, 2020

View reviewed changes

nulltoken dismissed stale reviews from P0lip and philsturgeon via 69e0af2 January 21, 2020 21:39

nulltoken force-pushed the ntk/formated_enum branch from 69e0af2 to fe655ec Compare January 21, 2020 21:54

P0lip previously approved these changes Jan 21, 2020

View reviewed changes

P0lip assigned nulltoken Jan 21, 2020

feat: new typed-enum rule

dd02a4e

P0lip added the enhancement New feature or request label Jan 21, 2020

nulltoken dismissed P0lip’s stale review via dd02a4e January 21, 2020 22:01

nulltoken force-pushed the ntk/formated_enum branch from fe655ec to dd02a4e Compare January 21, 2020 22:01

P0lip approved these changes Jan 21, 2020

View reviewed changes

nulltoken requested a review from philsturgeon January 21, 2020 22:16

philsturgeon approved these changes Jan 22, 2020

View reviewed changes

Merge branch 'develop' into ntk/formated_enum

be73dec

philsturgeon added the automerge label Jan 22, 2020

rossmcdonald approved these changes Jan 22, 2020

View reviewed changes

nulltoken merged commit 8d16284 into develop Jan 22, 2020

nulltoken deleted the ntk/formated_enum branch January 22, 2020 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new rule to detect enum value that do not respect specified type #913

feat: new rule to detect enum value that do not respect specified type #913

nulltoken commented Jan 14, 2020 •

edited

Loading

nulltoken commented Jan 15, 2020 •

edited

Loading

P0lip left a comment

nulltoken commented Jan 16, 2020

P0lip left a comment

philsturgeon commented Jan 17, 2020

nulltoken commented Jan 17, 2020

rossmcdonald commented Jan 17, 2020

nulltoken commented Jan 17, 2020

nulltoken Jan 17, 2020

philsturgeon Jan 17, 2020

nulltoken Jan 18, 2020

philsturgeon Jan 17, 2020

philsturgeon Jan 17, 2020

philsturgeon Jan 17, 2020

philsturgeon Jan 17, 2020

nulltoken Jan 18, 2020

philsturgeon Jan 17, 2020

philsturgeon Jan 17, 2020

philsturgeon Jan 17, 2020

P0lip left a comment

nulltoken commented Jan 21, 2020

nulltoken commented Jan 21, 2020

marbemac commented Jan 21, 2020


		## typedEnum

		Models described by both a `type` and an `enum` must only expose enum values that respect this type.

	Models described by both a `type` and an `enum` must only expose enum values that respect this type.
	When both a `type` and `enum` are defined for a property, the enum values must respect the type.

		@@ -0,0 +1,33 @@
		====test====
		Identify enum values that do not respect the specified format

	test('should return undefined when all enum values respect the format', () => {
	it('is undefined when all enum values respect the type', () => {

	test('should identify enum values that do not respect the format', () => {
	it('identifies each enum value which does not respect the type', () => {

feat: new rule to detect enum value that do not respect specified type #913

feat: new rule to detect enum value that do not respect specified type #913

Conversation

nulltoken commented Jan 14, 2020 • edited Loading

nulltoken commented Jan 15, 2020 • edited Loading

P0lip left a comment

Choose a reason for hiding this comment

nulltoken commented Jan 16, 2020

P0lip left a comment

Choose a reason for hiding this comment

philsturgeon commented Jan 17, 2020

nulltoken commented Jan 17, 2020

rossmcdonald commented Jan 17, 2020

nulltoken commented Jan 17, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P0lip left a comment

Choose a reason for hiding this comment

nulltoken commented Jan 21, 2020

nulltoken commented Jan 21, 2020

marbemac commented Jan 21, 2020

nulltoken commented Jan 14, 2020 •

edited

Loading

nulltoken commented Jan 15, 2020 •

edited

Loading