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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend discriminator to support number and boolean values #1729

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Extend discriminator to support number and boolean values #1729

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ce7a34b
Extend discriminator to support number and boolean values
pkuczynski Aug 5, 2021
c7938f0
Merge branch 'master' into discriminator
epoberezkin Oct 9, 2021
b442098
Merge branch 'master' into discriminator
pkuczynski Oct 26, 2022
85d65f9
apply code feedback
pkuczynski Oct 26, 2022
813f8cf
Merge branch 'master' into discriminator
epoberezkin Nov 13, 2022
364dfde
Merge branch 'master' into discriminator
pkuczynski Apr 6, 2023
65ff2d2
Merge branch 'master' into discriminator
pkuczynski Sep 18, 2023
a3600bf
Merge branch 'master' into discriminator
jasoniangreen Apr 30, 2024
0b42fcf
Merge branch 'master' into discriminator
pkuczynski Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions lib/vocabularies/discriminator/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const def: CodeKeywordDefinition = {
const valid = gen.let("valid", false)
const tag = gen.const("tag", _`${data}${getProperty(tagName)}`)
gen.if(
_`typeof ${tag} == "string"`,
_`["string", "number", "boolean"].includes(typeof ${tag})`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be improved. The compilation phase should collect all tags and in case they are all the same type, this type would be used. In general case it should use something like or(...tagTypes.map((t) => _`typeof ${tag} == ${t}`)) - include call is inefficient. This general case is likely to be ok for tagTypes with a single item, if not - or function can be improved (but I think it should just work).

() => validateMapping(),
() => cxt.error(false, {discrError: DiscrError.Tag, tag, tagName})
)
Expand All @@ -41,9 +41,9 @@ const def: CodeKeywordDefinition = {
function validateMapping(): void {
const mapping = getMapping()
gen.if(false)
for (const tagValue in mapping) {
for (const tagValue of mapping.keys()) {
gen.elseIf(_`${tag} === ${tagValue}`)
gen.assign(valid, applyTagSchema(mapping[tagValue]))
gen.assign(valid, applyTagSchema(mapping.get(tagValue)))
}
gen.else()
cxt.error(false, {discrError: DiscrError.Mapping, tag, tagName})
Expand All @@ -57,8 +57,10 @@ const def: CodeKeywordDefinition = {
return _valid
}

function getMapping(): {[T in string]?: number} {
const oneOfMapping: {[T in string]?: number} = {}
type OneOfMapping = Map<string | number | boolean, number>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add null.


function getMapping(): OneOfMapping {
const oneOfMapping: OneOfMapping = new Map()
const topRequired = hasRequired(parentSchema)
let tagRequired = true
for (let i = 0; i < oneOf.length; i++) {
Expand All @@ -78,7 +80,7 @@ const def: CodeKeywordDefinition = {
}

function addMappings(sch: AnySchemaObject, i: number): void {
if (sch.const) {
if (sch.const !== undefined) {
addMapping(sch.const, i)
} else if (sch.enum) {
for (const tagValue of sch.enum) {
Expand All @@ -89,11 +91,16 @@ const def: CodeKeywordDefinition = {
}
}

function addMapping(tagValue: unknown, i: number): void {
if (typeof tagValue != "string" || tagValue in oneOfMapping) {
throw new Error(`discriminator: "${tagName}" values must be unique strings`)
function addMapping(tagValue: any, i: number): void {
if (
!["string", "number", "boolean"].includes(typeof tagValue) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+null

oneOfMapping.has(tagValue)
) {
throw new Error(
`discriminator: "${tagName}" values must be unique strings, numbers or booleans`
)
}
oneOfMapping[tagValue] = i
oneOfMapping.set(tagValue, i)
}
}
},
Expand Down
183 changes: 164 additions & 19 deletions spec/discriminator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe("discriminator keyword", function () {
}

describe("validation", () => {
const schema1 = {
const stringSchema1 = {
type: "object",
discriminator: {propertyName: "foo"},
oneOf: [
Expand All @@ -44,7 +44,7 @@ describe("discriminator keyword", function () {
],
}

const schema2 = {
const stringSchema2 = {
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
Expand All @@ -66,18 +66,139 @@ describe("discriminator keyword", function () {
],
}

const schemas = [schema1, schema2]

it("should validate data", () => {
assertValid(schemas, {foo: "x", a: "a"})
assertValid(schemas, {foo: "y", b: "b"})
assertValid(schemas, {foo: "z", b: "b"})
assertInvalid(schemas, {})
assertInvalid(schemas, {foo: 1})
assertInvalid(schemas, {foo: "bar"})
assertInvalid(schemas, {foo: "x", b: "b"})
assertInvalid(schemas, {foo: "y", a: "a"})
assertInvalid(schemas, {foo: "z", a: "a"})
const numberSchema = {
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
oneOf: [
{
properties: {
foo: {const: 1},
a: {type: "string"},
},
required: ["a"],
},
{
properties: {
foo: {enum: [2, 3]},
b: {type: "string"},
},
required: ["b"],
},
],
}

const boolSchema = {
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
oneOf: [
{
properties: {
foo: {const: true},
a: {type: "string"},
},
required: ["a"],
},
{
properties: {
foo: {const: false},
b: {type: "string"},
},
required: ["b"],
},
],
}

const mixedSchema = {
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
oneOf: [
{
properties: {
foo: {const: "x"},
a: {type: "string"},
},
required: ["a"],
},
{
properties: {
foo: {enum: [1, 2]},
b: {type: "string"},
},
required: ["b"],
},
{
properties: {
foo: {const: true},
c: {type: "string"},
},
required: ["c"],
},
],
}

const stringSchemas = [stringSchema1, stringSchema2]
const numberSchemas = [numberSchema]
const boolSchemas = [boolSchema]
const mixedSchemas = [mixedSchema]

it("should validate data for string discriminator", () => {
assertValid(stringSchemas, {foo: "x", a: "a"})
assertValid(stringSchemas, {foo: "y", b: "b"})
assertValid(stringSchemas, {foo: "z", b: "b"})

assertInvalid(stringSchemas, {})
assertInvalid(stringSchemas, {foo: 1})
assertInvalid(stringSchemas, {foo: "bar"})
assertInvalid(stringSchemas, {foo: "x", b: "b"})
assertInvalid(stringSchemas, {foo: "y", a: "a"})
assertInvalid(stringSchemas, {foo: "z", a: "a"})
})

it("should validate data for number discriminator", () => {
assertValid(numberSchemas, {foo: 1, a: "a"})
assertValid(numberSchemas, {foo: 2, b: "b"})
assertValid(numberSchemas, {foo: 3, b: "b"})

assertInvalid(stringSchemas, {})
assertInvalid(stringSchemas, {foo: "1"})
assertInvalid(stringSchemas, {foo: "bar"})
assertInvalid(numberSchemas, {foo: 1, b: "b"})
assertInvalid(numberSchemas, {foo: 2, a: "a"})
assertInvalid(numberSchemas, {foo: 3, a: "a"})
})

it("should validate data for boolean discriminator", () => {
assertValid(boolSchemas, {foo: true, a: "a"})
assertValid(boolSchemas, {foo: false, b: "b"})

assertInvalid(boolSchemas, {})
assertInvalid(boolSchemas, {foo: "1"})
assertInvalid(boolSchemas, {foo: true, b: "b"})
assertInvalid(boolSchemas, {foo: false, a: "a"})
})

it("should validate data for mixed type discriminator", () => {
assertValid(mixedSchemas, {foo: "x", a: "a"})
assertValid(mixedSchemas, {foo: 1, b: "b"})
assertValid(mixedSchemas, {foo: 2, b: "b"})
assertValid(mixedSchemas, {foo: true, c: "c"})

assertInvalid(mixedSchemas, {})
assertInvalid(mixedSchemas, {foo: "x"})
assertInvalid(mixedSchemas, {foo: "x", b: "b"})
assertInvalid(mixedSchemas, {foo: "x", c: "c"})
assertInvalid(mixedSchemas, {foo: 1})
assertInvalid(mixedSchemas, {foo: 1, a: "a"})
assertInvalid(mixedSchemas, {foo: 1, c: "c"})
assertInvalid(mixedSchemas, {foo: 2})
assertInvalid(mixedSchemas, {foo: 2, a: "a"})
assertInvalid(mixedSchemas, {foo: 2, c: "c"})
assertInvalid(mixedSchemas, {foo: true})
assertInvalid(mixedSchemas, {foo: true, a: "a"})
assertInvalid(mixedSchemas, {foo: true, b: "b"})
})
})

Expand Down Expand Up @@ -113,15 +234,15 @@ describe("discriminator keyword", function () {
)
})

it("tag value should be string", () => {
it("tag value should be string, number or boolean", () => {
invalidSchema(
{
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
oneOf: [{properties: {foo: {const: 1}}}],
oneOf: [{properties: {foo: {const: {baz: "bar"}}}}],
},
/discriminator: "foo" values must be unique strings/
/discriminator: "foo" values must be unique strings, numbers or booleans/
)
})

Expand All @@ -133,7 +254,27 @@ describe("discriminator keyword", function () {
required: ["foo"],
oneOf: [{properties: {foo: {const: "a"}}}, {properties: {foo: {const: "a"}}}],
},
/discriminator: "foo" values must be unique strings/
/discriminator: "foo" values must be unique strings, numbers or booleans/
)

invalidSchema(
{
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
oneOf: [{properties: {foo: {const: 1}}}, {properties: {foo: {const: 1}}}],
},
/discriminator: "foo" values must be unique/
)

invalidSchema(
{
type: "object",
discriminator: {propertyName: "foo"},
required: ["foo"],
oneOf: [{properties: {foo: {const: true}}}, {properties: {foo: {const: true}}}],
},
/discriminator: "foo" values must be unique/
)
})

Expand All @@ -154,7 +295,11 @@ describe("discriminator keyword", function () {

function assertValid(schemas: SchemaObject[], data: unknown): void {
schemas.forEach((schema) =>
ajvs.forEach((ajv) => assert.strictEqual(ajv.validate(schema, data), true))
ajvs.forEach((ajv) => {
const validate = ajv.compile(schema)
const valid = validate(data)
assert.strictEqual(valid, true)
})
pkuczynski marked this conversation as resolved.
Show resolved Hide resolved
)
}

Expand Down