Support nullable #1

mdmower-csnw · 2023-10-20T15:42:57Z

Add transform for nullable: true to include null as a possible type for any of the following schema types: const, enum, type.
Log a warning when const is not set to null but schema is marked nullable: true
Log a warning when enum does not contain null but schema is marked nullable: true
Add tests and update snapshot

References from upstream:

- Add transform for nullable: true to include null as a possible type for any of the following schema types: const, enum, type, anyOf, and oneOf. - Log a warning when enum does not contain null but schema is marked nullable: true. - Add tests and update snapshot

tvharris

@mdmower-csnw: Approved. This is going to be a huge help for us in multiple projects!

There are a few points to consider below. I reviewed with the mindset that the implementation should be consistent with AJV's because EOV uses AJV.

tvharris · 2023-10-20T20:49:47Z

src/normalizer.ts

+  if (schema.const !== undefined) {
+    if (schema.const !== null) {
+      schema.enum = [schema.const, null]
+      delete schema.const
+    }
+  } else if (schema.enum) {
+    if (!schema.enum.includes(null)) {
+      schema.enum.push(null)
+      log('yellow', 'normalizer', 'enum should include null when schema is nullable', schema)


@mdmower-csnw:

Should there be a warning for the const case, too? AJV docs say with nullable: true:

const would fail, unless it is "const": null

I wouldn't be opposed to throwing instead of the warnings, because it's critical that the schema is updated or the type will be at odds with how AJV validates.

What if const and enum are both defined? Is that case caught earlier by validation?

The const case is mostly moot because normalizer Transform const to singleton enum runs just before Transform nullable to null type:

rules.set('Transform const to singleton enum', schema => { if (schema.const !== undefined) { schema.enum = [schema.const] delete schema.const } })

I included const handling just in case normalizers are reordered in the future.

What if const and enum are both defined? Is that case caught earlier by validation?

schema.enum actually gets wiped out in this case due to the above normalizer. This is a great example of how this tool is not a validator. There's an expectation that the user has performed validation prior to using this tool.

Should there be a warning for the const case, too? AJV docs say with nullable: true.

That's fine. Can do.

I wouldn't be opposed to throwing instead of the warnings, because it's critical that the schema is updated or the type will be at odds with how AJV validates.

Like mentioned before, this is not a validator. It's a tool that seldom throws.

Opted to reorder transforms so that Transform nullable to null type runs before Transform const to singleton enum in a5db6da. Also created a warning() function that prints to the console even when VERBOSE is not used.

tvharris · 2023-10-20T20:50:08Z

src/normalizer.ts

+  } else if (schema.anyOf) {
+    schema.anyOf.push(link({type: 'null'}, schema.anyOf))
+  } else if (schema.oneOf) {
+    schema.oneOf.push(link({type: 'null'}, schema.oneOf))


@mdmower-csnw: Is this how AJV handles nullable: true for anyOf and oneOf? It doesn't say explicitly in the docs.

That is a rabbit hole.

The OpenAPI spec writers (i.e. not the full picture, just one example of a team defining a JSON schema) eventually decided to restrict use of nullable to only schemas that define type in 3.0.3 (see 3.0.3: Fields). In an early draft of 3.1, when nullable was planned for deprecation rather than removal, wording implied that nullable wasn't forbidden when paired with something like allOf, but that it could be overridden (see 3.1(draft): Fields). The start of the rabbit hole is here: OpenAPI-Specification: Reference objects don't combine well with “nullable”.

I approached this from the standpoint that nullable paired with anyOf and oneOf is unambiguous (but not allOf). After more reading, it looks like this was a bad assumption and there's good reason the OpenAPI team opted to limit use of nullable to schemas with type. The most helpful comments were this inqury: OAI/OpenAPI-Specification#1368 (comment), and this response: OAI/OpenAPI-Specification#1368 (comment) .

I lean towards leaving the const and enum pairings with nullable as is, i.e. allowed, but logged if null not in value(s) -- the intent is clear even if not 100% spec compliant. On the other hand, I no longer feel comfortable pairing nullable with anyOf, oneOf, and allOf. These will be removed.

Handling of anyOf and oneOf removed in a5db6da.

tvharris · 2023-10-20T20:50:22Z

test/e2e/nullable.ts

+    h: {
+      type: 'string',
+      const: null,
+      nullable: true,
+    },


@mdmower-csnw: This might not be directly related to the changes in this PR, but I was wondering if there's a warning for a schema like this where the type and const (or enum values) are inconsistent. I don't know how AJV handles such a schema.

There is not a warning output by this tool. You can test this.

tvharris · 2023-10-20T20:50:30Z

test/normalizer/nullableAddsNullToType.json

+      "f": {
+        "type": ["null", "null"]
+      },


@mdmower-csnw: The output here and cases l, n, p don't seem ideal with the redundant null. I suppose it doesn't matter since the types are generated correctly.

Right. I don't think there's need for deduplication of types at this stage since the end user doesn't see it and the TypeScript conversion handles duplicates just fine.

Deduplication was trivial so opted to handle it in a5db6da.

mdmower-csnw

I reviewed with the mindset that the implementation should be consistent with AJV's because EOV uses AJV.

I wouldn't call that the guiding principle. This project is not affiliated with express-openapi-validator. Also, AJV is a great resource, but this tool is not a validator, so it doesn't have to be as strict. When I search the module for throw , it seems to me that the maintainer opts to work with what is available to the extent possible rather than throw because of minor validation issues. It's not my intention to start diverging significantly from upstream. If we can achieve our needs with a light touch, that's how I'd like to proceed.

mdmower-csnw · 2023-10-23T14:03:06Z

src/normalizer.ts

+  if (schema.const !== undefined) {
+    if (schema.const !== null) {
+      schema.enum = [schema.const, null]
+      delete schema.const
+    }
+  } else if (schema.enum) {
+    if (!schema.enum.includes(null)) {
+      schema.enum.push(null)
+      log('yellow', 'normalizer', 'enum should include null when schema is nullable', schema)


The const case is mostly moot because normalizer Transform const to singleton enum runs just before Transform nullable to null type:

rules.set('Transform const to singleton enum', schema => { if (schema.const !== undefined) { schema.enum = [schema.const] delete schema.const } })

I included const handling just in case normalizers are reordered in the future.

What if const and enum are both defined? Is that case caught earlier by validation?

schema.enum actually gets wiped out in this case due to the above normalizer. This is a great example of how this tool is not a validator. There's an expectation that the user has performed validation prior to using this tool.

Should there be a warning for the const case, too? AJV docs say with nullable: true.

That's fine. Can do.

I wouldn't be opposed to throwing instead of the warnings, because it's critical that the schema is updated or the type will be at odds with how AJV validates.

Like mentioned before, this is not a validator. It's a tool that seldom throws.

mdmower-csnw · 2023-10-23T15:15:23Z

src/normalizer.ts

+  } else if (schema.anyOf) {
+    schema.anyOf.push(link({type: 'null'}, schema.anyOf))
+  } else if (schema.oneOf) {
+    schema.oneOf.push(link({type: 'null'}, schema.oneOf))


That is a rabbit hole.

The OpenAPI spec writers (i.e. not the full picture, just one example of a team defining a JSON schema) eventually decided to restrict use of nullable to only schemas that define type in 3.0.3 (see 3.0.3: Fields). In an early draft of 3.1, when nullable was planned for deprecation rather than removal, wording implied that nullable wasn't forbidden when paired with something like allOf, but that it could be overridden (see 3.1(draft): Fields). The start of the rabbit hole is here: OpenAPI-Specification: Reference objects don't combine well with “nullable”.

I approached this from the standpoint that nullable paired with anyOf and oneOf is unambiguous (but not allOf). After more reading, it looks like this was a bad assumption and there's good reason the OpenAPI team opted to limit use of nullable to schemas with type. The most helpful comments were this inqury: OAI/OpenAPI-Specification#1368 (comment), and this response: OAI/OpenAPI-Specification#1368 (comment) .

I lean towards leaving the const and enum pairings with nullable as is, i.e. allowed, but logged if null not in value(s) -- the intent is clear even if not 100% spec compliant. On the other hand, I no longer feel comfortable pairing nullable with anyOf, oneOf, and allOf. These will be removed.

mdmower-csnw · 2023-10-23T15:20:13Z

test/e2e/nullable.ts

+    h: {
+      type: 'string',
+      const: null,
+      nullable: true,
+    },


There is not a warning output by this tool. You can test this.

mdmower-csnw · 2023-10-23T15:22:30Z

test/normalizer/nullableAddsNullToType.json

+      "f": {
+        "type": ["null", "null"]
+      },


Right. I don't think there's need for deduplication of types at this stage since the end user doesn't see it and the TypeScript conversion handles duplicates just fine.

- Introduce warning() to output console warnings - Add warning for nullable const, where const value is not null - Move nullable transform before const->enum transform so that potential warning about nullable const can display - Remove anyOf and oneOf handling since JSON spec should only constrain possibilities, not expand (allowance made for enum and const to expand types but they show warnings when this happens). - Avoid pushing 'null' onto available types if it already exists.

Support nullable

cda84ce

- Add transform for nullable: true to include null as a possible type for any of the following schema types: const, enum, type, anyOf, and oneOf. - Log a warning when enum does not contain null but schema is marked nullable: true. - Add tests and update snapshot

tvharris approved these changes Oct 20, 2023

View reviewed changes

mdmower-csnw commented Oct 23, 2023

View reviewed changes

mdmower-csnw merged commit cc614c8 into csnw Oct 23, 2023
30 checks passed

mdmower-csnw deleted the mdm-nullable branch October 23, 2023 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support nullable #1

Support nullable #1

mdmower-csnw commented Oct 20, 2023 •

edited

Loading

tvharris left a comment

tvharris Oct 20, 2023

mdmower-csnw Oct 23, 2023

mdmower-csnw Oct 23, 2023

tvharris Oct 20, 2023

mdmower-csnw Oct 23, 2023

mdmower-csnw Oct 23, 2023

tvharris Oct 20, 2023

mdmower-csnw Oct 23, 2023

tvharris Oct 20, 2023 •

edited

Loading

mdmower-csnw Oct 23, 2023

mdmower-csnw Oct 23, 2023

mdmower-csnw left a comment

mdmower-csnw Oct 23, 2023

mdmower-csnw Oct 23, 2023

mdmower-csnw Oct 23, 2023

mdmower-csnw Oct 23, 2023

Support nullable #1

Support nullable #1

Conversation

mdmower-csnw commented Oct 20, 2023 • edited Loading

tvharris left a 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

tvharris Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdmower-csnw left a 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

mdmower-csnw commented Oct 20, 2023 •

edited

Loading

tvharris Oct 20, 2023 •

edited

Loading