Azure / autorest.typescript

Extension for AutoRest (https://github.com/Azure/autorest) that generates TypeScript code. The transpiled javascript code is isomorphic. It can be run in browser and in node.js environment.
MIT License
176 stars 75 forks source link

[Codegen/TypeSpec] Handle apiVersion as Path Parameter #2148

Open masons-msft opened 9 months ago

masons-msft commented 9 months ago

Microsoft Fabric's API guidelines require the API version to be a path parameter rather than a query parameter. We would like to use this tool for generating our TypeScript SDK from our TypeSpec definition but incorporating the API version as a path parameter cannot be accomplished as cleanly as we would like, at least from what we know of the tool. There are two things we would like to be able to do.

First, we would like to set the API version as part of the client creation or route definition rather than having to specify the API version as a parameter in every request. It seems like this is already a feature in the autorest.python project.

Second, we would like api-version to not be automatically added as a query parameter to all requests. I believe there is already an issue that has been created related to this along with a pull request to resolve it.

Are there already features to support this use case that we are simply not aware of? If not, would it be possible to add this as a feature?

Here is a quick example of what our TypeSpec looks like.

model ApiVersionPathParameter {
  apiVersion: string;
}

@resource("exampleResources")
model ExampleResource {
  @Key
  id: string;
}

@tag("ExampleResources")
interface ExampleResourceInterface {
  @autoRoute
  @readsResource(ExampleResource)
  get(
    ...ApiVersionPathParameter
    ...Resource.ResourceParameters<ExampleResource>
  ): ExampleResource;
}

The generated clientDefinition.ts file would then have a Routes interface like the following.

export interface Routes {
  (
    path: "/{apiVersion}/exampleResources/{id}",
    apiVersion: string,
    id: string
): ExampleResourcesGet;

Is there a way to either set the path to "/v1/exampleResource/{id}" with no apiVersion parameter or set the client's baseUrl to include the api version at the end and then have all paths look like "/exampleResource/{id}"?

qiaozha commented 9 months ago

I guess, what you need is to put the api version in the @server in TypeSpec, something like below.

@server(
  "{host}.{subdomain}.{sufix}.com/{apiVersion}",
  "Confidential Ledger Service",
  {
    @path
    host?: string = "one",
    @path
    subdomain?: string = "two",
    @path
    sufix?: string = "three",
    @path
    apiVersion?: string = "v1"
  }
)

let me know if it helps.

sjiherzig commented 9 months ago

@qiaozha Thanks for the suggestion - unfortunately, this doesn't work well in scenarios where we have more than one API version. We still use the OpenAPI Spec as the official contract for specific versions. As @masons-msft mentioned, we are also getting the "api-version" query parameter produced in addition to the API version appearing in the path.

This seems like a discrepancy to other autorest modules to me, where the TypeSpec apiVersion parameter isn't interpreted consistently. See the linked Python issue here: https://github.com/Azure/autorest.python/issues/1778

qiaozha commented 9 months ago

Just want to confirm, are you saying you want use the api version twice both in the path and in the query but only want to define it once ?

sjiherzig commented 9 months ago

No, we only want to use it once - in the path.

qiaozha commented 9 months ago

Maybe there's some misunderstanding here, what I am trying to suggest is, you put the apiVersion into the server decorator and then remove it from all the operation path, that has references to it. The premise is that all your operation path would be like {endpoint}/{apiVersion}/someOtherPath, if we move it to server decorator, when you create client, the base url would become {endpoint}/apiVersion, and the operation path would just be /someOtherPath.

sjiherzig commented 9 months ago

Yes, this was clear, and we tried it out - however:

  1. This does not work well when you introduce multiple API versions. It expands the enum for the api version attribute in spec documents for older API versions whenever you introduce a new one. I.e., the swagger for v3 will reference v2 and v1. But, the swagger for v1 will also reference v3.
  2. The generated SDK still includes an "?api-version=" query parameter for all requests, which means the api-version is referenced twice, in two different ways - once in the path, and once as a query parameter (@masons-msft linked a PR above that seems to fix this, though).

It should also not be necessary to work around this. Including the API version in the path is a fairly common way of versioning an API. The use of the "api-version" query string is widely used in Azure, but is not universal. Other language emitters seem to support specifying the api version in the path, so it seems this is a missing feature / bug of the TS emitter.

qiaozha commented 9 months ago

Thanks for the feedback, I got your point now.

  1. We haven't support multiapi version yet. And the current behavior is just generated based on latest api version, which means, if you have a path that are removed in apiVersion v1, you will probably can not find it in the generated packages, and if you have a path that was not added until version v3, you will find that customers can still pass v1 and v2 to call the path. @xirzec not sure if we should prioritize the multi api support, now that we have customer requests on this ?
  2. I think this is a known issue for us, I will work on a fix earlier next week.

There's another issue here which is to promote the common path parameter which doesn't have to be apiVersion as I think to the client constructor, which we probably need some furthur internal discussion about this.

masons-msft commented 9 months ago

Hi @qiaozha , thanks for your responses on this issue!

  1. It's good to know about possibility for clients to pass invalid api versions to api calls. I agree that better built-in support for this would be useful, but I think the primary issue that @sjiherzig is calling out here is that if we follow your proposed work-around, the OpenAPI spec then includes a direct reference to the Versions enum. When the OpenAPI specs are regenerated after adding a new version, the copy produced for older versions would be modified to expand the possible values of that enum. Changing the OpenAPI spec in this way is considered a breaking change, so it would be great to have a solution that avoids this problem. Like you called out in your last comment, perhaps this fits into adding more support for common path parameters; however, I think api version is a special case since it plays an important role when generating the OpenAPI spec from the TypeSpec.
  2. Sounds great, thanks for looking into that! In my original post for this issue I included a link to a PR that seems like it should address this issue, so I just want to make sure you see it before accidentally duplicating any work.
qiaozha commented 9 months ago

Thanks for providing the details. I think that OpenAPI spec include api versions that are newly added may not be expected from OpenAPI spec emitter. I will confirm with TypeSpec team https://github.com/microsoft/typespec/issues/2715.

xirzec commented 9 months ago

The API version being a path parameter is a reasonable ask, but am I correct in understanding we are also looking to have a single SDK package that supports multiple API versions that have had properties/operations added or removed?

From the RLC perspective this isn't something we intend to support (one RLC === one API version), and it's not clear how we would generate a DPG package that supports multiple versions in a cohesive manner.

One idea is we could provide separate subpath exports for each API version, though that would bloat the package size quite a bit and it's not entirely clear why a consumer would want to toggle between two supported API versions in a single integration?

masons-msft commented 9 months ago

Hi @xirzec - I think I had gotten confused when responding to an earlier comment by @qiaozha. I agree it makes sense to have a 1:1 mapping between API version and generated TypeScript library rather than having one TypeScript library that supports all API versions. The main thing would be for API version in the TypeScript client to be a fixed constant that aligns with the version of the API the TypeScript SDK is generated for. This would ensure that all API calls that are able to be made through the TypeScript client have a valid path.

xirzec commented 9 months ago

That sounds good to me!

qiaozha commented 9 months ago

@masons-msft I want to confirm a few things,

  1. do you still want api version to be a client constructor parameter or even a path parameter if we generate it as a fixed constant ? I mean client could just set the constant value to it, right ?
  2. if it's a fix constant, changing from v1 to v2 will be a api layer breaking change right ? does this mean you want each api version to be released as a major version ?
  3. Does it work for you if we still generate api version as string, but will give it a default value if we have agreed to promote the path level api version parameter into client constructor ? which means we allows customer to set a different version, but don't promise that call will be successfully returned.

@xirzec I wonder if we are going to promote api version for both modular and RLC if it's defined in path parameter ? Also, how are we going to handle default value if we decide to not promote it ? Which is kind of conflict with what we agreed before for default value ? https://github.com/Azure/autorest.typescript/issues/2049#issuecomment-1758794386

masons-msft commented 9 months ago

@qiaozha - Thanks for your questions. Here are the thoughts of me and @sjiherzig :

  1. I would leave this up to you, @xirzec, and the rest of your team. If you want to support multi-api version clients, then it's fine to expose the api version to the client as a constructor parameter. If you want each generated TypeSpec library to map to exactly one API version, then it shouldn't be. Our team is ok with whichever approach you want to take.
  2. We want our APIs to always be backwards-compatible, so changing the API version shouldn't be a breaking change for the client. Perhaps I'm misunderstanding your question, though.
  3. That seems fine. Your team is welcome to control the implementation details. We would just like to be able to have the api version as a path parameter that is configured at the time of client construction, and not have the api-version query parameter appended added to every call. The level of control the customer has in choosing the api version and any guarantees about hitting valid API endpoints are up to your team's decision in Question 1.
xirzec commented 9 months ago

If I'm understanding @masons-msft correctly, it sounds like we can internally set the API version when constructing the URL for the request to be a constant that is fixed in the generated code. If the SDK is regenerated later against updated TypeSpec, this constant may be updated, but as long as what is exposed to the consumer stays consistent (modulo any breaks in the REST API itself), we are okay with it.

It doesn't sound like there is a goal here for allowing multiple API versions or having the API version be controllable by the SDK consumer, which aligns with our current design philosophy.

I think the sticking point that @qiaozha may want clarity on is since RLC uses the paths themselves as the operation identifier for the user, ideally we'd have some work in the code generator so the consumer could write something like

client.path("/exampleResources/{id}", "id-goes-here").get();

but the client would actually request {endpoint}/{api-version}/exampleResources/{id}.

versus today where we might naively generate something like:

client.path("/2024-01-01/exampleResources/{id}", "id-goes-here").get();

Which would then be broken when the API version changed.

Put another way: we want to ensure the api-version in the path is set without being exposed explicitly to the user.

masons-msft commented 9 months ago

Yes, that seems right! I don't think there was any question for me there, but if there was feel free to let me know.

This is a belated question, but for my own learning, could you spell out what RLC means for me?

xirzec commented 9 months ago

This is a belated question, but for my own learning, could you spell out what RLC means for me?

Here's a helpful overview of RLC: https://github.com/Azure/azure-sdk-for-js/blob/main/documentation/rest-clients.md

Let us know if you still have any questions after perusing it! 👍

qiaozha commented 9 months ago

Yes, @xirzec that is the breaking I worried about. I feel like there's no elegant way to support path parameter to have default value in RLC. if we have something like

client.path("{api-version}/exampleResources/{id}", "id-goes-here").get();

and then try to pass the api-version when build the url, that would be weird right ? it's ambiguous to me where exactly "id-goes-here" should match.

xirzec commented 9 months ago

@qiaozha agreed, I think we have to hide it from the public surface somehow, not sure what the best way is to prevent it from leaking out into either the endpoint url or the path map, maybe we'd have to have some special handling inside the client?

xirzec commented 9 months ago

one simple option we could do for now is write a simple policy for this package that parsed the request URL and prepended the api-version to the path?

qiaozha commented 8 months ago

Reopen it, as we are not done yet.