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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ajv-spec benchmarks #256

Merged
merged 1 commit into from
Oct 22, 2022
Merged

Add ajv-spec benchmarks #256

merged 1 commit into from
Oct 22, 2022

Conversation

sinclairzx81
Copy link
Contributor

@sinclairzx81 sinclairzx81 commented Oct 22, 2022

This PR includes an additional ajv-spec benchmark. It is offered to provide a structural comparison between the schemas that are generated via TSON.application<[...], 'ajv'> (which are not working for several existing tests).

This benchmark uses the same schematics as the TypeBox benchmark but instead measures the Ajv compiler. The ajv-spec benchmark also uses the same spoiler data that was implemented for the TypeBox benchmark to ensure parity. All tests are currently passing (however these have only been implemented for the is benchmark)

This PR also updates the TypeBoxObjectUnionImplicit to use Type.Optional() over Type.Union([Type.Undefined(), ...]) to permit missing properties as per spoiler data.

Results below.

Components typescript-json typebox ajv ajv-spec io-ts class-validator zod
object (hierarchical) 81097.70324462268 118578.5412456366 54221.45328719723 37174.72527472527 6593.477858069297 49.69391429600288 334.80095395340305
object (recursive) 66755.83549462764 73629.89972652688 Failed 32892.74447949527 3687.965722801788 32.056145675265554 56.66605538235833
object (union, explicit) 10990.792561834267 10205.264119904954 992.4228423581594 5681.200353045013 2349.778270509978 13.155490590169926 28.506271379703538
object (union, implicit) 10943.84410983171 60568.386863948035 3529.8507462686566 29959.765910753475 Failed 13.325930038867297 15.891032917139615
array (recursive) 5851.241166878058 5332.346410066617 Failed 1825.7132406730066 352.48901903367494 3.0349013657056143 7.322568531731131
array (union, explicit) 2977.364740522635 1786.0594795539034 Failed 579.9350415012631 273.7137511693171 6.3753984624039 2.4390243902439024
array (union, implicit) 3117.9919384389887 1787.4279337920775 Failed 696.6764061358656 312.61699737925875 7.293809612867028 3.0297292179511457
ultimate union 428.3369803063457 210.28799130592284 Failed 15.630746598013976 Failed Failed 0.18109380659181457

Submitting for comparative benchmarking and review.

@sinclairzx81
Copy link
Contributor Author

Linking draft implementation of TS > JSON Schema representation mapping on repl.it using the ObjectUnionImplicit test for reference.

Could be good to compare the TSON.application<...> output vs the output generated on this link, and just check what's different. You should be able to fork the repl and modify the type and interfaces for different generated schema outputs.

Wondering if specifically TSON.application<T, 'json-schema-draft-6'> could be possible (where the output is a limited subset of the draft-6 specification) (noting the differences between the swagger specification like nullable). Would be good to get good Ajv alignment in any case.

Hope this helps!

Copy link
Owner

@samchon samchon left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. I will refactor it sometimes later and try to enhance json schema generator following your guide.

@samchon samchon merged commit e036315 into samchon:master Oct 22, 2022
@sinclairzx81
Copy link
Contributor Author

@samchon All good :)

@samchon
Copy link
Owner

samchon commented Oct 22, 2022

Anyway, is it really okay that defining optional only instead of using undefined type?

As you know, TypeScript can assigned undefined value on the question marked property type like below.

With additionalProperties: false option, failure case could be happened.

interface ISomething {
    name: string;
    etc?: string;
}

const name: string = "unknown";
const etc: string = "another";
const something: ISomething = {
    name,
    etc: name === "unknown" ? undefined : etc,
};

@sinclairzx81
Copy link
Contributor Author

is it really okay that defining optional only instead of using undefined type?

Yes, it's fine. TypeScript will treat prop?: string as prop: string | undefined. But the key difference is that Optional allows the property key to be missing. As noted per #255 (comment), see expanded description below

// -------------------------------------------------------------------
// { prop: undefined | null | number } means:
// - prop key MUST exist
// - prop value must be undefined | null | number

prop: Type.Union([Type.Undefined(), Type.Null(), Type.Number()])

{ prop: undefined } // ok
{ prop: null }      // ok
{ prop: 1 }         // ok
{ }                 // fail !!

// -------------------------------------------------------------------
// { prop?: null | number } means:
// - prop key can be OPTIONAL
// - prop value must be null | number (or undefined)

prop: Type.Optional(Type.Union([Type.Null(), Type.Number()]))

{ prop: undefined } // ok - see below for details.
{ prop: null }      // ok
{ prop: 1 }         // ok
{ }                 // ok

Note: The reason { prop: undefined } is ok in the second example is due to how JavaScript would test for undefined on that value. Consider.

const value = {}
if(value.prop === undefined) console.log('ok')

So to support missing properties, the correct way to express this is via Type.Optional(T) (which removes the key from the schemas required array). Also keep in mind that JSON Schema does not have a specification for undefined (only null). So if you're testing Ajv specifically, you will want to remove all Type.Undefined() and use Type.Optional() only.

Hope that helps.

@sinclairzx81
Copy link
Contributor Author

Just on this...

With additionalProperties: false option, failure case could be happened.

The { additionalProperties: false } will be fine too as the schema still encodes for "known" properties. The additionalProperties check is usually dependent on the keys returned from Object.keys(schema.properties).

So for example.

// ------------------------------------------------------
// TypeBox
// ------------------------------------------------------

const T = Type.Object({
   prop: Type.Optional(Type.Union([Type.String(), Type.Null()])
   // ^  string | null | undefined
})

// ------------------------------------------------------
// JSON Schema
// ------------------------------------------------------

const T = {
  type: 'object',
  properties: {
    prop: { // <------ additionalProperties: false checks Object.keys(T.properties)
      anyOf: [
        { type: 'string' },
        { type: 'null' }
      ]
    }
  }
}

// ------------------------------------------------------
// additionalProperties: false test
// ------------------------------------------------------

const allowed = Object.keys(T.properties)

Object.keys({ prop: null }).every(key => allowed.includes(key)) // ok
Object.keys({ }).every(key => allowed.includes(key))            // ok
Object.keys({ foo: 1 }).every(key => allowed.includes(key))     // fail

Constrained schemas (such as the ones implemented in TSON) will likely need to control minimum required and excess properties checks through a combination of required and additionalProperties: false. These two properties essentially let you encode the rules for the current union specialization algorithm, but provides the flexibility to allow additional properties if necessary.

Again, hope this helps!

@samchon
Copy link
Owner

samchon commented Oct 22, 2022

How about this case that assigning undefined value on non-declared property?

To anticipate below case, TSON.equals() function is even checking whether the excess property value is undefined or not.

I've implemented such logic because following JS rule, undefined valued property and real undefined property are equivalent.

When undefined value comes

{
    x: 1,
    y: 1,
    z: 1,
    excess: undefined,
}

TypeBox

return function check(value) {
  return (
    (typeof value === 'object' && value !== null && !Array.isArray(value)) &&
    (Object.keys(value).length === 3) &&
    (typeof value.x === 'number') &&
    (typeof value.y === 'number') &&
    (typeof value.z === 'number')
 )
}

TSON

(input, path, exceptionable) =>
    "number" === typeof input.x &&
    "number" === typeof input.y &&
    "number" === typeof input.z &&
    Object.entries(input).every(([key, value]) => {
        if (undefined === value) return true;
        if (["x", "y", "z"].some((prop) => key === prop))
            return true;
        return false;
    }),

@sinclairzx81
Copy link
Contributor Author

JSON Schema requires you to be explicit about allowing (or disallowing) additionalProperties. The default is to allow them.

const T = Type.Object({
  x: Type.Number(),
  y: Type.Number(),
  z: Type.Number()
})

{ x: 1, y, 2, z: 3 }                    // ok
{ x: 1, y, 2, z: 3, excess: undefined } // ok
const T = Type.Object({
  x: Type.Number(),
  y: Type.Number(),
  z: Type.Number()
}. { additionalProperties: false })

{ x: 1, y, 2, z: 3 }                    // ok
{ x: 1, y, 2, z: 3, excess: undefined } // fail

@samchon
Copy link
Owner

samchon commented Oct 22, 2022

I mean that the additionalProperties: false should not consider undefined value as superfluous property.

You suggested that additionalProperties: false can be a good solution for validating implicit union type like ObjectUnionImplicit type, but such undefined superfluous property can break integrity of TypeBox. When collaborating with many other web developers in Korea, I've seen lots of people overusing the undefined when creating union typed object and it's the reason why not blocking the undefined value.

That is the case what I've seen in field. Of course, below implementation seems really terrible, but it has been introduced as an exemplary way for developing backend program composing union typed data (may be from DB?). Of course, whether supporting below terrible case or not is totally up to your policy. But while looking your solution additionalProperties: false, I kept getting confused, and I finally realized that it was because of the case below.

const value: A|B|C = {
    id: "a",
   name: "b",
   something: "c",
   ...{
      ...properties of A with undefined value
   },
   ...{
     ...B,
   },
  ...{
    ...C
  }
};

@sinclairzx81
Copy link
Contributor Author

I mean that the additionalProperties: false should not consider undefined value as superfluous property.

Sure, but it's not superfluous because a property with an undefined value still yields a key (which would be an unexpected key/value if additionalProperties:false was enabled)

Object.keys({ x: 1, y: undefined }) // ['x', 'y'] - 'y' is unexpected

I've seen lots of people overusing the undefined when creating union typed object and it's the reason why not blocking the undefined value.

Undefined in JavaScript is a little ambiguous, as it could mean "the key does not exist" or "the key exists but it's value is undefined". I expect this is another reason why JSON schema doesn't have a specification for undefined values.

const A = {} 
const B = { prop: undefined }

if(A.prop === undefined) console.log('ok')
if(B.prop === undefined) console.log('also ok')

// ^ as a user, you wouldn't know by running this test that `A` had a missing `prop` key. The user would
// need to explicitly call `Object.keys(A | B)` to determine if the property actually existed. This is a problem
// if there is dependent logic based on the property keys. The JSON Schema specification provides 
// assurances here, but it does so by way of NOT having a specification for undefined.

Overall, I do think it's important to have a distinction between {} and { prop: undefined } for validation purposes. So, I tend to agree with the JSON Schema specification here (there's been very few cases where JSON Schema hasn't been able to express the semantics of TS types, even if the schema can be quite verbose)

@samchon
Copy link
Owner

samchon commented Oct 22, 2022

Within framework of JSON schema, your policy seems right. How to handle undefined property, it would be just a difference between our libraries. Thanks for detailed description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants