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

aws-apigateway: JsonSchema expects type as enum type #20055

Open
dbartholomae opened this issue Apr 23, 2022 · 7 comments
Open

aws-apigateway: JsonSchema expects type as enum type #20055

dbartholomae opened this issue Apr 23, 2022 · 7 comments
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@dbartholomae
Copy link
Contributor

Describe the bug

When hand-writing a schema or importing a schema from somewhere else to use in multiple places, including apigateway, TypeScript rejects the schema:

const analyticsEventInputSchema = {
  type: 'object',
  properties: {
    name: { type: 'string' }
  }
}
const analyticsEventModel = api.addModel("AnalyticsEvent", {
  modelName: "AnalyticsEvent",
  contentType: "application/json",
  schema: analyticsEventInputSchema,
});

Expected Behavior

This should just work

Current Behavior

 Types of property 'type' are incompatible.
Type '"object"' is not assignable to type 'JsonSchemaType | JsonSchemaType[] | undefined'.

Reproduction Steps

If the example above is not enough to show the issue, I can create a reproducing repo.

Possible Solution

Introduce a string union type for JsonSchemaTypeString and use it here:
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-apigateway/lib/json-schema.ts#L34

Keep the exported JsonSchemaType enum to avoid a breaking change and deprecate it.

Additional Information/Context

No response

CDK CLI Version

2.20.0

Framework Version

No response

Node.js Version

16.13.1

OS

Windows 11

Language

Typescript

Language Version

No response

Other information

No response

@dbartholomae dbartholomae added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 23, 2022
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Apr 23, 2022
@peterwoodworth
Copy link
Contributor

What's wrong with our current JsonSchemaType enum? This works perfectly fine so there's no need to deprecate it

        const schema: JsonSchema = {
            type: JsonSchemaType.OBJECT,
            properties: {
                name: {
                    type: JsonSchemaType.STRING
                }
            },
            required: ['name']
        }
        api.addModel('event', {
            modelName: 'aanalytivcevent',
            contentType: 'application/json',
            schema: schema
        })

@peterwoodworth peterwoodworth added feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 25, 2022
@dbartholomae
Copy link
Contributor Author

This assumes that you create the JSON schema solely for AWS. But that's not what JSON schema is for - it's a universal format usable across different other technologies, e.g. I'm also using the same schema to run validations inside the lambda (specifically to differentiate between multiple different inputs that would all be valid). The code which creates the JSON schema isn't even aware of AWS, so importing something from AWS in there would force me to make a application module dependent on AWS CDK.

This problem also shows up when importing a json file directly: TypeScript can detect its structure, but will complain that the string 'object' is not the same as JSONSchemaType.OBJECT.

The other way around, I don't know what would be lost by expecting a const string instead of an Enum, so it would make use cases either without losing anything (unless there is some jsii magic I'm not aware of).

@peterwoodworth peterwoodworth added p1 effort/medium Medium work item – several days of effort and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Apr 25, 2022
@peterwoodworth
Copy link
Contributor

This all makes sense @dbartholomae, thanks for the explanation. Forcing users to use our enum when it would otherwise be easily portable sounds less than ideal.

I disagree with deprecating the enum - but I think we could make it so that the properties accept the enum or a string. We can easily add checks to make sure the string inputs are valid as well

@dbartholomae
Copy link
Contributor Author

Thanks! Deprecating the enum might not be worth it, I agree. It would have the following benefits:
a) give users a clear recommendation on how to use the interface
b) make it easier to sync the enum with the actual type used
For b), that would also be a solution with template literal types, but they are only available since TypeScript 4.1 and, if I understand correctly, AWS CDK tries to keep backward dependency up to 3.8.3.

I'm fine with a decision in either direction and could then also create a PR. I would recommend, though, to then also reopen the following issue:
#20056 (comment)
It is also needed to be able to use externally defined JSON schemas.

@dbartholomae
Copy link
Contributor Author

Moving the discusison from the PR into this issue:

This won't be an effective way to proceed here. JSII prevents us from using union types due to strongly typed languages (Java, Go) losing compile time safety. On top of this, we can't use private utility types like ReadonlyArray either. (I was right when I said that the existing ones are fine because they aren't customer facing i.e. compiled through JSII)

Instead, a path forward here is to create some new class with a static method which will import a schema and convert it to the JsonSchema type. Something like this:

export class JsonSchemaImporter {
  static fromObj(input: Object): JsonSchema {
    // convert input to JsonSchema type
  }
}

I suggest creating a new branch & PR for this. Feel free to reach out to me here or on slack for any more questions!

Ideally, this would use a meaningful input type (basically the existing JsonSchema type, but with unions and ReadOnlyArrays), and, ideally be in a meaningful place. A really nice interface for this would be if JsonSchema became a class with a static JsonSchema.fromObject, but both the input type and the change from interface to class might run into problems with JSII.

Do you have an example for me for what is the preferred way for these kind of utils in this repository? Then I could make an alternate suggestion.

The code for this than would be straightforward:

  return input as JsonSchema

@eliasschoof
Copy link

I agree with @dbartholomae that this would be really useful. My use case is deriving the API Gateway model validation schema from a TypeScript type:

my-handler.ts

type MyEventBody = {
  arg1: string;
  arg2: number;
};

export const handler = (event: { body: MyEventBody }): Promise<APIGatewayProxyResult> => {
  const arg1 = event.body.arg1;
  const arg2 = event.body.arg2;
  // ...
};

Ideally, I could reuse the MyEventBody type in the CDK stack to set up the correct request model validator. For example, using typescript-json-schema:

import { Model } from 'aws-cdk-lib/aws-apigateway';
...

export class MyApiStack extends Stack {
  constructor() {
    ...

    const schema = ... // use typescript-json-schema to convert MyEventBody type to JSON schema
    const model = new Model(this, 'MyLambdaEventBody', {
      restApi,
      schema,
    };
    restApi.root.addMethod('POST', myLambdaIntegration, {
      requestModels: {
        'application/json': model,
      },
    );
  }
}

This would mean that I do not have to keep the event body type and the validation model in sync manually. Currently this does not work because typescript-json-schema uses type strings like "object" while the Model schema expects JsonSchemaType.OBJECT.

@dbartholomae
Copy link
Contributor Author

Currently, the PR is stuck as I'm waiting on input on the solution design.

@estrehle one workaround for now is to just model as JsonSchema. Usually, as is problematic, but in this case, you are not really losing any type safety as you are anyways relying on an external library to create a correct JSON Schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants