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

[FEATURE] Smithy mixin input and de-nested API call #460

Closed
2 tasks
zetashift opened this issue Jun 5, 2023 · 9 comments
Closed
2 tasks

[FEATURE] Smithy mixin input and de-nested API call #460

zetashift opened this issue Jun 5, 2023 · 9 comments
Assignees
Labels
backlog documentation Improvements or additions to documentation

Comments

@zetashift
Copy link
Contributor

Describe the feature

If I have the following Smithy shapes:

resource Customer {
    identifiers: {
        /// Unique customer ID.
        customerId: CustomerId
    }
    properties: {
        /// Name of the customer.
        name: String
        createdAt: Timestamp
    }
    create: CreateCustomer
}

/// The data needed to create a new customer.
@mixin
structure CustomerData for Customer {
    @required
    $name
}

/// A single record from all the known customers.
structure CustomerRecord for Customer with [CustomerData] {
    @required
    $customerId
    @required
    $createdAt
    @required
    $name
}


/// Create a new customer.
@http(method: "POST", uri: "/customers")
operation CreateCustomer {
    input := with [CustomerData] {}
    output: CustomerRecord
    errors: [AlreadyExistsError]
}

@pattern("^[A-Za-z0-9 ]+$")
string CustomerId

And then generate the client, I have to call the CreateCustomer method as follows:

const response = await apiClient.createCustomer({
  createCustomerRequestContent: { name: customerName },
});

The nesting is slightly annoying, is there anyway to get it flattened, so that the { name: ... } could just be defined as the first parameter:

const response = await apiClient.createCustomer({ name: customerName });

The current case for is that I get the following error:

Argument of type '{ name: string; }' is not assignable to parameter of type 'CreateCustomerRequest'.
  Object literal may only specify known properties, and 'name' does not exist in type

Use Case

I don't need it, it's just a nice to have :P.

Proposed Solution

No response

Other Information

Just do it the current way!

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

PDK version used

0.19.15

What languages will this feature affect?

Typescript

Environment details (OS name and version, etc.)

MacOS ARM64, Ventura

@cogwirrel
Copy link
Member

Hey! I agree this would be a nicer interface! 😄 It might be a bit tricky to support however, as the client is generated by the typescript-fetch OpenAPI Generator. We can customise it through templates (which is how we generate the lambda handler wrappers etc) but the capabilities are still a bit limited.

The interface each client method takes accepts all the request parameters (ie path and query params), plus an argument for the request body, eg. an operation like POST /customers/{pathParam}?queryParam=someValue might end up with an interface like: { pathParam: string; queryParam: string; createCustomerRequestContent: { ... } }.

"Unpacking" the body parameter might lead to naming conflicts for users using ModelLanguage.OPENAPI as request parameters and the body are defined separately rather than in the same input structure in Smithy. I'm also not 100% sure if we can easily access each member of the request body parameter to create that kind of input - could be possible but would be a bit of work I think!

Since you're using Smithy, generating code directly from Smithy could be the way to create a more "user friendly" client however, as the generated code can align more closely to the Smithy model rather than the generated OpenAPI specification. There's a plugin called typescript-codegen which can generate a client directly from Smithy, with the docs here: https://smithy.io/2.0/guides/using-code-generation/generating-a-client.html - I haven't used this before so I'm not sure how easy it is to customise things like middleware for auth etc though!

Perhaps this could be an option? You can customise model.options.smithy.smithyBuildOptions to add the dependency and configure the plugin. You might need to add some additional commands to your preCompile task for the model project to copy the generated code out of the build directory and into the src directory of another TypeScriptProject you set up as part of your monorepo to get everything playing nicely together...

Perhaps you could give that a try and see if it works nicely? If it's an improvement we could look at adding a developer guide to the docs which explains how to do it in more detail, or even look at building an option in to TypeSafeApi for Smithy users :)

@zetashift
Copy link
Contributor Author

Perhaps this could be an option? You can customise model.options.smithy.smithyBuildOptions to add the dependency and configure the plugin. You might need to add some additional commands to your preCompile task for the model project to copy the generated code out of the build directory and into the src directory of another TypeScriptProject you set up as part of your monorepo to get everything playing nicely together...

I'll try it out this weekend! Can't make any guarentees I'll succeed tho :P

@cogwirrel
Copy link
Member

Keen to hear how you go! 😄 I might also give it a try over the next few days if I get a spare moment :)

@cogwirrel
Copy link
Member

Had a bit of time this evening and had a go! Example repo here! https:/cogwirrel/tsapi-smithy-client-example

It seems to work alright! Note that the auth is built in to the generated client based on what you specify in your Smithy model, so you need eg @sigv4(name: "execute-api") on your service shape for sigv4 auth.

I also extended that example out of curiosity to use the Smithy TypeScript Server SDK for implementing the lambda handlers :) Error handling in the Smithy generated client seems to be a little bit nicer when you use it in combination with the Smithy server sdk, as you can use instanceof checks.

Hope that helps :)

cogwirrel added a commit that referenced this issue Jun 13, 2023
…d client in handler wrappers

Clients generated directly from Smithy (using the typescript-codegen plugin) can deserialise error
responses into the appropriate generated model classes, but they do this by checking for a special
`x-amzn-errortype` header. This change updates the generated handler wrappers to include the
appropriate value for this header for error responses. Note that OpenAPI specs generated from Smithy
add "ResponseContent" as a suffix to all response data types, so we remove this in order to match
the appropriate error structure ID. If the response data type doesn't end with ResponseContent, it
didn't come from Smithy so there's no need to return this header anyway.

re #460
cogwirrel added a commit that referenced this issue Jun 13, 2023
…d client in handler wrappers (#468)

Clients generated directly from Smithy (using the typescript-codegen plugin) can deserialise error
responses into the appropriate generated model classes, but they do this by checking for a special
`x-amzn-errortype` header. This change updates the generated handler wrappers to include the
appropriate value for this header for error responses. Note that OpenAPI specs generated from Smithy
add "ResponseContent" as a suffix to all response data types, so we remove this in order to match
the appropriate error structure ID. If the response data type doesn't end with ResponseContent, it
didn't come from Smithy so there's no need to return this header anyway.

re #460
@cogwirrel
Copy link
Member

#468 adds the error header expected by the Smithy generated client so that you don't lose out on better error handling when using the native type-safe-api handler wrappers.

I think the final step for this issue would be to either:

  1. Add documentation to explain how to generate and use the Smithy typescript client and/or server sdk in your monorepo (ie the steps to reproduce this example repo)
  2. Add "native" support for the Smithy typescript client and server sdk as "libraries", (eg libraries: [Library.SMITHY_TYPESCRIPT_CLIENT, Library.SMITHY_TYPESCRIPT_SERVER_SDK]) to abstract away the changes to smithy-build.json and TypeScriptProject setup, plus documentation on how to set things up.

I'm leaning towards (1), as (2) will add the challenge of keeping dependency versions in sync since we can't use the Smithy generated list of deps/devDeps in a projen TypeScriptProject, which means things may break if users pick a different Smithy version. This is the same for the OpenAPI generated clients though so perhaps it's not too much of a stretch :)

@zetashift
Copy link
Contributor Author

  1. Add documentation to explain how to generate and use the Smithy typescript client and/or server sdk in your monorepo (ie the steps to reproduce this example repo)

This is a fine solution for me! It doesn't need to be fancy and the nesting isn't that big of a problem.

@cogwirrel cogwirrel self-assigned this Jun 21, 2023
@zetashift
Copy link
Contributor Author

zetashift commented Jul 18, 2023

Ah this doesn't only happen with mixins, but also with the shorthand operation syntax:

@tags(["Journeys", "Qrd"])
@idempotent
@http(method: "POST", uri: "/qrds/safeqrd/activate")
operation ValidateTag {
    input := {
        @required
        activationCode: String
    }
    output := {
        @required
        isValid: Boolean
    }
}

I have to call this the following way:

const isValid = await journeyClient.validateTag({
  validateTagRequestContent: {
    activationCode,
  },
});

@cogwirrel
Copy link
Member

That key like validateTagRequestContent will always be there in the generated client if the operation accepts a request body unfortunately, regardless of how it’s defined in the Smithy spec. Smithy generates an object schema with that name in the OpenApi spec it creates, which is then used as the key name in the generated clients.

It’s still on my todo list to document generating a client directly from Smithy (without going via OpenApi), which results in a client which has types that align much closer to the Smithy model :)

@cogwirrel cogwirrel added the documentation Improvements or additions to documentation label Nov 6, 2023
@cogwirrel
Copy link
Member

I'm going to close this issue in favour of #687 - as it'd be great if we could natively generate Smithy clients/servers etc

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

No branches or pull requests

2 participants