Throw error if required property is not present

[tshelburne]

We had a response typecheck that should not have because we made a typo in the properties list, as below:

required: [organization]
type: object
properties:
  account: { $ref: '#/components/schemas/IOrganization' }

We were expecting the response typechecking to fail if organization wasn't present, but instead it was only looking for an optional account value. It would be nice to have this throw an error on build - something like "Required property "organization" is not defined in the "properties" at [path.to.this.schema]".

[drwpow]

Thanks for the suggestion! I think this isn’t really outlined in the README well, but one goal of this project is for it to not be a validator for your schema; we assume you’ve handled that elsewhere. So basically in order to do this, we’d have to add validation to schemas, which could possibly slow down generation times.

But that said, if this provides utility, I’m not opposed to adding it. Maybe it could be opt-in?

Also is there a particular validator that seems to work better for OpenAPI schemas (personally I’ve dabbled with a few, but I don’t know if any of them would catch a missing reference like this)?

[tshelburne]

I get that - I was thinking of it less as validation of the schema and more as throwing on invalid type generation. If you say a property is required, but there's no description of the type, a type can't be generated to match the description.

I won't argue if you consider it out of scope, but I think it definitely walks a line. I'm certainly meaning to come at it from the perspective of generating valid types rather than making sure swagger is correct.

[tshelburne]

Oh also - I wouldn't implement by validating the entire schema. I would just have a check in place for this particular step (when schema.required exists, throw if schema.property.[...required] doesn't).

[drwpow]

That makes perfect sense; I don’t see why this couldn’t be added easily.

For something this minimal, I’m thinking it’s on-by-default, but maybe we could add an opt-out flag (--allow-missing or something; TBD) in case this new error blocks someone’s workflow (e.g. they’re loading in remote schemas or something that this generator may not know about).

[tshelburne]

I'm happy in either direction, but I agree with your suggestion.

[drwpow]

Update: it seems that #620 seems to address this by appending the schema, rather than throwing an error. Perhaps this is a good solution to this? Or is it still expected that required should throw an error?

[tshelburne]

It's certainly better than nothing - it works well when you receive a value of the response because unknown will force you to make some kind of type assertion, so it works well for an API client. However, it still let's you assign anything you want to the response type, which isn't super helpful for use in validating server-side responses. For example:

interface UserResponse {
	email: unknown
	firstName?: string
	lastName: string
}

// Error on the client - Type 'unknown' is not assignable to type 'string'
function getEmail(res: UserResponse): string {
	return res.email
}

// No error on the server
ctx.body: UserResponse = {
	email: 5,
	lastName: ''
}

To me, it comes down to the use-case - it could be considered a feature in development to create your response properties before you really settle on shape of the values. But if you are just setting up an API, it doesn't really offer much.

[tshelburne]

Also, I think this presents a pretty good argument for an --allow-missing flag - that way, devs can opt-in when working out their response shapes, and lock it back once things are settled.

[drwpow]

Update on this: a lot of improvements are landing in v6 (#968), and these behaviors have been improved. But a note has been added to the README that a validator is a requirement to use this library, and it should be run before this library is run for best results.