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

JSONSchemaType requires "required" and "additionalProperties", unless it is oneOf schema #1521

Open
psteinroe opened this issue Mar 29, 2021 · 32 comments

Comments

@psteinroe
Copy link

What version of Ajv are you using? Does the issue happen if you use the latest version?
8.0.1, yes

Ajv options object

{}

Your code

type Test = {
  some: string;
  other: boolean;
};

const t: JSONSchemaType<Test> = {
  type: 'object',
  properties: {
    other: {
      type: 'boolean',
    },
    some: {
      type: 'string',
    },
  },
};

What results did you expect?
Should be a valid type, instead the compiler throws

TS2322: Type '{ type: "object"; properties: { other: { type: "boolean"; }; some: { type: "string"; }; }; }' is not assignable to type 'JSONSchemaType<test, false>'.   Type '{ type: "object"; properties: { other: { type: "boolean"; }; some: { type: "string"; }; }; }' is not assignable to type '{ oneOf: readonly JSONSchemaType<test, false>[]; } & { [keyword: string]: any; $id?: string; $ref?: string; $defs?: { [x: string]: JSONSchemaType<Known, true>; }; definitions?: { ...; }; }'.     Property 'oneOf' is missing in type '{ type: "object"; properties: { other: { type: "boolean"; }; some: { type: "string"; }; }; }' but required in type '{ oneOf: readonly JSONSchemaType<test, false>[]; }'

@epoberezkin
Copy link
Member

epoberezkin commented Mar 29, 2021

I will check it - what typescript version do you use? Could you try upgrading to 4.2.3?

@aMoniker
Copy link

aMoniker commented Mar 30, 2021

I'm running into the same error, using typescript 4.2.3 and ajv 8.0.1 (same error in 8.0.0 as well)

@psteinroe
Copy link
Author

I upgraded to 4.2.3 and still have the error.

@epoberezkin
Copy link
Member

epoberezkin commented Mar 30, 2021

I understand the problem - this is not a bug, but with the addition of the union support error reporting became a bit more confusing - maybe something can be done about it (cc @erikbrinkman).

The minimal correct schema for this type is (and it compiles correctly):

const schema: JSONSchemaType<Test> = {
  type: "object",
  properties: {
    other: {
      type: "boolean",
    },
    some: {
      type: "string",
    },
  },
  required: ["other", "some"],
}

The reason for that is that the properties in the type are required, but JSON Schema without required keyword makes them optional.

Please note that this schema still allows additional properties.

An equivalent JSON Type Definition schema is more concise (the type is implied, properties are required and additional properties are not allowed by default):

const schema: JTDSchemaType<Test> = {
  properties: {
    other: {
      type: "boolean",
    },
    some: {
      type: "string",
    },
  },
  additionalProperties: true,
}

@epoberezkin epoberezkin changed the title JSONSchemaType oneOf is missing in simple object type JSONSchemaType requires "required" and "additionalProperties", unless it is oneOf schema Mar 30, 2021
@epoberezkin
Copy link
Member

epoberezkin commented Mar 30, 2021

The reason error message mentions oneOf - typescript probably chooses the latest branch in the union - maybe swapping the branches will fix it...

@psteinroe
Copy link
Author

Thanks for the quick response! Highly appreciated. :)

@aMoniker
Copy link

Likewise, thank you for explaining this!
Would you recommend using JTDSchemaType rather than JSONSchemaType for most simple use cases?

@psteinroe
Copy link
Author

I am sorry, but I have to come back to this. I now get another type error using JTDSchemaType. The example from the docs

import { JTDSchemaType } from 'ajv/dist/jtd';

interface MyData {
  foo: number;
  bar?: string;
}

const schema: JTDSchemaType<MyData> = {
  properties: {
    foo: { type: 'int32' },  // error here
  },
  optionalProperties: {
    bar: { type: 'string' }, // and here
  },
};

fails with TS2322: Type 'string' is not assignable to type 'never'..

@psteinroe
Copy link
Author

Ive looked into the code and it seems like the problem comes from the second generic which default to Record<string, never>.

@epoberezkin
Copy link
Member

epoberezkin commented Mar 30, 2021

@steinroe error using JTDSchemaType

This example compiles for me, possibly you need TS 4.2.3. See these tests: https://github.com/ajv-validator/ajv/blob/master/spec/types/jtd-schema.spec.ts#L184

@aMoniker Would you recommend using JTDSchemaType rather than JSONSchemaType for most simple use cases?

This is a non-trivial trade-off - I've tried to summarise it here: https://ajv.js.org/guide/schema-language.html#comparison

@erikbrinkman
Copy link
Collaborator

@epoberezkin I have the same hunch you have about branches and will look into fixing it, because that's definitely not an ideal error message.

erikbrinkman added a commit to erikbrinkman/ajv that referenced this issue Apr 1, 2021
typescript seems to report errors with the last member of a valid union
first, so this rearranges the type so the simple union types come first,
and the type that represents the meaty types comes last. This should
help make errors like the one in ajv-validator#1521 more clear.
@epoberezkin
Copy link
Member

released in 8.0.3

@BenjD90
Copy link

BenjD90 commented Apr 1, 2021

With version 8.0.3 and TypeScript 4.2.3, I get a wired error Type 'string' is not assignable to type 'never' (same as comment #1521 (comment)) if I set the strict mode of TypeScript to false 🤔

Here an exemple to reproduce the error : https://replit.com/@BenjaminDaniel1/AJV-issue-1521

Does any one has any idea about that ?

@psteinroe
Copy link
Author

@BenjD90 thanks for figuring out it is because of strict mode is set to false! I can confirm that this is causing the issue above!

@epoberezkin
Copy link
Member

if this is strictNullCheck - then yes, it’s required to be true for this type to work correctly. It was in the docs previously but looks like with the site migration it was lost - needs to be added.

Btw, using this type is completely optional - it’s only needed for your validation function to be type guard, but you can also pass type parameter directly to ajv.compile - needs to be better documented.

@epoberezkin epoberezkin reopened this Apr 1, 2021
@epoberezkin
Copy link
Member

Obviously, without using JSONSchemaType there is no type level check for consistency of data type and schema - but if you need strictNullChecks to be false, it is not possible to achieve

@BenjD90
Copy link

BenjD90 commented Apr 1, 2021

It's the strictNullChecks that is required to be true, as you said. Thanks to add it to the doc :)

@crobinson42
Copy link

I landed here because TS is yelling at me for this:

image

Versions:

ajv: 8.1.0
typescript: 4.2.4

@erikbrinkman
Copy link
Collaborator

@crobinson42 what is the actual error your getting? from the paste email is a number, but in properties you've declared it as a string, which should be why typescript is correctly erroring.

@fatihky
Copy link

fatihky commented Apr 27, 2021

Hi, I am also getting a similar error message. I couldn't find out the solution.
I am using an example code from this page: https://ajv.js.org/api.html#ajv-compile-schema-object-data-any-boolean-promise-any
The code:

interface Foo {
  foo: number
}

const FooSchema: JSONSchemaType<Foo> = {
  type: "object",
  properties: {foo: {type: "number"}},
  required: ["foo"],
  additionalProperties: false,
}

Here is my environment:

❯ jq .version node_modules/typescript/package.json
"4.2.4"

❯ jq .version node_modules/ts-node/package.json
"9.1.1"

example.ts:

import { JSONSchemaType } from 'ajv';

interface Foo {
  foo: number
}

const FooSchema: JSONSchemaType<Foo> = {
  type: "object",
  properties: {foo: {type: "number"}},
  required: ["foo"],
  additionalProperties: false,
}

The output I get:

❯ ./node_modules/.bin/ts-node example.ts          

/dev/node_modules/ts-node/src/index.ts:513
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
example.ts:7:7 - error TS2322: Type '{ type: "object"; properties: { foo: { type: "number"; }; }; required: string[]; additionalProperties: false; }' is not assignable to type 'JSONSchemaType<Foo, false>'.
  Type '{ type: "object"; properties: { foo: { type: "number"; }; }; required: string[]; additionalProperties: false; }' is not assignable to type '{ type: "object"; required: readonly never[]; additionalProperties?: boolean | JSONSchemaType<unknown, false>; unevaluatedProperties?: boolean | JSONSchemaType<unknown, false>; ... 7 more ...; maxProperties?: number; } & { ...; } & { ...; }'.
    Type '{ type: "object"; properties: { foo: { type: "number"; }; }; required: string[]; additionalProperties: false; }' is not assignable to type '{ type: "object"; required: readonly never[]; additionalProperties?: boolean | JSONSchemaType<unknown, false>; unevaluatedProperties?: boolean | JSONSchemaType<unknown, false>; ... 7 more ...; maxProperties?: number; }'.
      Types of property 'required' are incompatible.
        Type 'string[]' is not assignable to type 'readonly never[]'.
          Type 'string' is not assignable to type 'never'.

7 const FooSchema: JSONSchemaType<Foo> = {
        ~~~~~~~~~

    at createTSError (/dev/node_modules/ts-node/src/index.ts:513:12)
    at reportTSError (/dev/node_modules/ts-node/src/index.ts:517:19)
    at getOutput (/dev/node_modules/ts-node/src/index.ts:752:36)
    at Object.compile (/dev/node_modules/ts-node/src/index.ts:968:32)
    at Module.m._compile (/dev/node_modules/ts-node/src/index.ts:1056:42)
    at Module._extensions..js (node:internal/modules/cjs/loader:1137:10)
    at Object.require.extensions.<computed> [as .ts] (/dev/node_modules/ts-node/src/index.ts:1059:12)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Function.Module._load (node:internal/modules/cjs/loader:828:14)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12)

I couldn't figure out the root cause of this, I hope this helps.

@erikbrinkman
Copy link
Collaborator

@fatihky this seems to be a bug in RequiredProperties that's not picking up on foo being required. I'll try to take a look tonight. As a dirty quick fix in the meantime you can forcibly cast required with as unknown as never[]

@fatihky
Copy link

fatihky commented Apr 27, 2021

@erikbrinkman thank you, and I'm sorry if my comment is out of the scope of this issue.

@erikbrinkman
Copy link
Collaborator

erikbrinkman commented Apr 28, 2021

@fatihky I originally thought this was related to a recent diff I put up, but actually it's the same as the original post. You don't have strictNullChecks enabled, and that's causing the error.

@epoberezkin This seems to be a recurring problem, people forgetting to or not realizing the problem of not enabling strictNullChecks. It seems we can use null extends undefined (or some other sentinel type to detect if strictNullChecks is enabled. I have a few proposals.

  1. redefine JSONSchemaType to be null extends undefined ? "must enable 'strictNullChecks' in tsconfig to use JSONSchemaType" : ActualJSONSchemaType which will probably throw an error on use, something like:
    Type 'true' is not assignable to type '"must enable strict null checks for JSONSchemaType to work"'.
    
    (this issue is still open unfortunately)
  2. use the same check, but expand the possible options for required to keyof T when we know the required check is going to fail. This might cause other issues with JSONSchemaType not fully verifying as it might, but it will prevent people not understanding why their schema is failing.
  3. Switch to type RequiredMembers<T> = {[K in keyof T]-?: Record<string, never> extends Pick<T, K> ? never : K }[keyof T] which works whether or not strictNullChecks is enabled.

Do you have a preference or another idea to address this?

@epoberezkin
Copy link
Member

This seems to be a recurring problem

indeed

I like option 1 with better error reporting. I think we might be digging ourselves a hole trying to support both modes. I’m also not sure if RequiredMembers is the only problematic part without strictNullChecks - I think there will be some other issues with nullable/optional properties…

Arguably, it’s beneficial to have strictNullChecks enabled anyway…

In any case, better errors is a good short term option that would at least explain the problem on the spot (that is, if people read error messages - I often don’t…)

@erikbrinkman
Copy link
Collaborator

@epoberezkin I'm sorry, but I'm ultimately not sure what you're suggesting? Just option 1? Unfortunately I think our only options are making it never, with some documentation that people hopefully look back to, or making it a string literal, and assuming that's "good enough" until typescript gives something better.

@epoberezkin
Copy link
Member

Yes, option 1.

andriyl pushed a commit to Redocly/ajv that referenced this issue Jun 16, 2021
typescript seems to report errors with the last member of a valid union
first, so this rearranges the type so the simple union types come first,
and the type that represents the meaty types comes last. This should
help make errors like the one in ajv-validator#1521 more clear.
@HendrikPetertje
Copy link

HendrikPetertje commented Oct 12, 2022

I think i triggered some bug that looks a lot like this now:

If i use the following snippet:

import { JSONSchemaType } from 'ajv-draft-04';

// { sub1: { foo: 'bar' }, sub2: { foo: 'baz' } }
type DataObject = Record<string, Record<string, unknown>>;
type DataObjectThatWillNotBugOut = Record<string, unknown>;

interface Article {
  id: string;
  title: string;
  content: string;
  graphData: DataObject;
}

const ArticleResponse: JSONSchemaType<Article> = {
  type: 'object',
  properties: {
    id: {
      type: 'string',
      nullable: false,
    },
    title: {
      type: 'string',
      nullable: false,
    },
    content: {
      type: 'string',
      nullable: false,
    },
    graphData: {
      type: 'object',
      nullable: false,
      properties: {},
      additionalProperties: true,
    },
  }
}

Then I am getting the following error:
image

Simplifying the interface

Funnily however when i simplify my graphData object from Record<string, Record<string, unknown>> to Record<string, unknown> (which is not entirely correct, but correct enough) then all my problems go away and i don't have to specify a required field anymore.

what if I include an empty required: []

well that's not possible, the validation will fail because required needs to have at least one property https://json-schema.org/understanding-json-schema/reference/object.html#id8

adding or removing additionalPrperties or properties has no effect

@erikbrinkman
Copy link
Collaborator

This isn't actually related because the past issue had to do with enabling strict null checks, whereas this seems to be an issue with accepting / requiring the "required" field, even though it can't be empty in draft 4.

@epoberezkin I'm not immediately sure how JSONSchemaType is handled between versions, so I'm not sure what the appropriate fix is, since the requirement for "required" was to prevent other user errors.

@HendrikPetertje in the short term, the schema is only used to help you, so if you really know that you don't want required you could do something like required: undefined as unknown as [] which should fool the type checker and allow the schema to validate appropriately.

@epoberezkin
Copy link
Member

The properties in the TS interface are required, so the schema must have “required” with all properties to correspond to the interface. nullable: false is no-op, it doesn’t make them required, it can be just removed.

@erikbrinkman
Copy link
Collaborator

But in the example, stripped down, this schema type checks fine:

type DataObject = Record<string, Record<string, unknown>>;

const graphData: JSONSchemaType<DataObject> = {
  type: 'object',
  properties: {},
  additionalProperties: {
    type: 'object',
    properties: {},
    additionalProperties: true,
  },
  required: [],
};

However, draft 4 says that "required" can't be empty. But given that it's a record, there are no required fields. Digging in a little further, it seems the problem is that here we check if there are any required members:

: [UncheckedRequiredMembers<T>] extends [never]

What's happening is that the inner Record isn't undefined, so JSONSchemaType thinks every string must have a defined record attached. Switching to type DataObject = Record<string, Record<string, unknown>> or type DataObject = Partial<Record<string, Record<string, unknown>>> solves the problem, which I guess is one way to look at the appropriate solution. However, it's not clear in typescript that there's a meaningful distinction between Record<...> and Partial<Record<...>>. Maybe there should be, but I think like indexing, people want slightly incorrect behavior.

This raises the question of JSONSchemaType should handle Record<string, ...> differently from other object types. I think probably yes, but I also don't really want to think through all the edge cases with doing that.

This question also raises another weakness of JSONSchemaType handing of records. JSONSchemaType should probably mandate additionalProperties for all records, and if the type isn't unknown, should probably mandate a schema for inner types, in the same way that JTD does.

@erikerikson
Copy link

Meta: JSONSchemaType is awesome and I love it, thank you so much for this library and this class. My system's OpenAPI specification is my schema definition and type declaration, required, type checked, and automatically validated for every HTTP request and other input. This is accelerating a standard of secure development quite a lot.

I landed here because I had a bug in my code resulting in Type 'string[]' is not assignable to type 'readonly never[]' (and then string is not never). Leaving a trail for anyone else confused like me.

I had an object with optional and nullable attributes ($type extends 'aws-lambda'.APIGatewayProxyStructuredResultV2) that I was narrowing. That object included headers which was its own object but which at the layer I was describing I would only declare as { [header: string]: string }. I've simplified things for "brevity" below and in my my sequence listing. I share hoping the user experience of how I got there might be useful to maintainers but feel welcome to ignore.

5 snazzy steps to non-existence (my simplified compiler error/fix sequence over too long a period of time):

  1. TS: missing 'nullable' on 'number'; ME: add nullable to attr1
  2. TS: missing 'required' on 'object'; ME add 'required' to top level, doesn't work, add attributes to required, doesn't work, add 'required' to 'headers' but forget to remove 'required' from top level (seemed like it might be needed and I paperred it over with an escape hatch for a time)
  3. TS: missing 'nullable' on 'object'; ME: add 'nullable' to attr2
  4. TS: missing 'nullable' on 'string'; ME: add 'nullable' to attr3
  5. TS: 'string' is not 'never'; ME: wat? ... OH! yeah... <mutter>the string is attr1 and the concepts of being required and nullable are contradictory</mutter>

It seems that a check for the contradiction could be useful for users

interface Base {
  attr1?: number | undefined
  attr2?: {
    [attr3: string]: boolean | number | string
  } | undefined
  attr3?: string | undefined
}
interface Mine extends Base {}
const schema1: JSONSchemaType<Mine> = {
  type: 'object',
  properties: {
    attr1: {
      type: 'number',
      // nullable: true, // missing
    },
    attr2: {
      type: 'object',
      patternProperties: {
        '^.+$': { type: 'string' },
      },
      propertyNames: {
        pattern: '^[a-zA-Z0-9_-]+$',
      },
    },
    attr3: {
      type: 'string',
    },
  },
}
const schema2: JSONSchemaType<Mine> = {
  type: 'object',
  properties: {
    attr1: {
      type: 'number',
      nullable: true,
    },
    attr2: {
      type: 'object',
      patternProperties: {
        '^.+$': { type: 'string' },
      },
      propertyNames: {
        pattern: '^[a-zA-Z0-9_-]+$',
      },
      // required: [], // missing
    },
    attr3: {
      type: 'string',
    },
  },
}
const schema3: JSONSchemaType<Mine> = {
  type: 'object',
  properties: {
    attr1: {
      type: 'number',
      nullable: true,
    },
    attr2: {
      type: 'object',
      patternProperties: {
        '^.+$': { type: 'string' },
      },
      propertyNames: {
        pattern: '^[a-zA-Z0-9_-]+$',
      },
      // nullable: true, // missing
      required: [],
    },
    attr3: {
      type: 'string',
    },
  },
  required: ['attr1', 'attr2', 'attr3'], // my error, forgotten and left behind
}
const schema4: JSONSchemaType<Mine> = {
  type: 'object',
  properties: {
    attr1: {
      type: 'number',
      nullable: true,
    },
    attr2: {
      type: 'object',
      patternProperties: {
        '^.+$': { type: 'string' },
      },
      propertyNames: {
        pattern: '^[a-zA-Z0-9_-]+$',
      },
      nullable: true,
      required: [],
    },
    attr3: {
      type: 'string',
      // nullable: true, // missing
    },
  },
  required: ['attr1', 'attr2', 'attr3'], // my error, forgotten and left behind
}
const schema5: JSONSchemaType<Mine> = {
  type: 'object',
  properties: {
    attr1: {
      type: 'number',
      nullable: true,
    },
    attr2: {
      type: 'object',
      patternProperties: {
        '^.+$': { type: 'string' },
      },
      propertyNames: {
        pattern: '^[a-zA-Z0-9_-]+$',
      },
      nullable: true,
      required: [],
    },
    attr3: {
      type: 'string',
      nullable: true,
    },
  },
  // ERROR: contadiction with nullable
  required: ['attr1', 'attr2', 'attr3'], // my error, forgotten and left behind
}
const schema6: JSONSchemaType<Mine> = {
  type: 'object',
  properties: {
    attr1: {
      type: 'number',
      nullable: true,
    },
    attr2: {
      type: 'object',
      patternProperties: {
        '^.+$': { type: 'string' },
      },
      propertyNames: {
        pattern: '^[a-zA-Z0-9_-]+$',
      },
      nullable: true,
      required: [],
    },
    attr3: {
      type: 'string',
      nullable: true,
    },
  },
  required: [], // fixed
}

@robogeek
Copy link

robogeek commented Apr 9, 2024

I landed on this issue with a similar error from the TypeScript compiler.

VERSIONS:

$ npx tsc --version
Version 5.4.4
$ jq .version node_modules/ajv/package.json 
"8.12.0"

I was able to make a simple fix for the problem after a lot of reading and puzzling over different things.

The JSON schema is written in YAML format, then compiled to both a JSON format schema and to a TypeScript interface using quicktype.

import { promises as fsp } from 'node:fs';
import Ajv, { JSONSchemaType } from "ajv";
const ajv = new Ajv.default({
    // strict: true,
    // allowUnionTypes: true,
    // validateFormats: true
});
import { OperatingCosts } from '../types-evchargingspec/operating-costs.js';

import _schema from '../schemas/operating-costs.json' with { type: "json" };

const schema: JSONSchemaType<OperatingCosts> = _schema.definitions.OperatingCosts;

This imports the generated type and -- since the generated schema is in JSON format, I thought why not use an import statement. But, that gave error messages similar to the above.

The fix is:

import { promises as fsp } from 'node:fs';
import Ajv, { JSONSchemaType } from "ajv";
const ajv = new Ajv.default({
    // strict: true,
    // allowUnionTypes: true,
    // validateFormats: true
});
import { OperatingCosts } from '../types-evchargingspec/operating-costs.js';

// import _schema from '../schemas/operating-costs.json' with { type: "json" };
const _json = await fsp.readFile('../schemas/operating-costs.json', 'utf-8');
const _schema = JSON.parse(_json);

const schema: JSONSchemaType<OperatingCosts> = _schema.definitions.OperatingCosts;

That is, do not use import to load the JSON, but to do it the old school way with fs.readFile and JSON.parse.

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

No branches or pull requests

10 participants