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

Chained builder does not work when parameters change names for subrequests #3757

Closed
ghelyar opened this issue Nov 20, 2023 · 3 comments · Fixed by #3760
Closed

Chained builder does not work when parameters change names for subrequests #3757

ghelyar opened this issue Nov 20, 2023 · 3 comments · Fixed by #3760
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Milestone

Comments

@ghelyar
Copy link

ghelyar commented Nov 20, 2023

I am trying to generate a client for the ConnectWise REST API.

In that API are two get requests, one inside the other:

/finance/agreements/{id}
/finance/agreements/{parentId}/additions/{id}

When building the second request with a C# generated client, the chained syntax does not work:

client.Finance.Agreements["1"].Additions["2"]

This gives a dictionary key conflict exception:
"An item with the same key has already been added. Key: id"

This is because client.Finance.Agreements["1"] added a key called id to the dictionary with id:1, and then Additions["2"] tried to add a key to the same dictionary (a new dictionary based on the existing PathParameters) with the same name, id:2, but actually that parameter changed names during the building process.

You could argue that it's a problem with the API design, but this is a large public API that can't be changed by the consumer, and as far as I know, it's allowed within the OpenAPI spec.

This is not just a problem of conflicts, but also changing names when building subrequests - even if there was not a conflict, the name change would still break it.

There is a work around to create the builder from scratch:

new ApiSdk.Finance.Agreements.Item.Additions.Item.AdditionsItemRequestBuilder(
    new Dictionary<string, object> { { "parentId", "1" }, { "id", "2" }},
    requestAdapter)

but it would be nicer if the chained syntax could handle this e.g. by adjusting the key names when PathParameters is passed to the subrequest, or by building the value into the base url for the subrequest.

@github-project-automation github-project-automation bot moved this to Todo in Kiota Nov 20, 2023
@baywet baywet self-assigned this Nov 20, 2023
@baywet baywet added this to the Backlog milestone Nov 20, 2023
@baywet
Copy link
Member

baywet commented Nov 20, 2023

Hi @ghelyar ,
Thanks for your interest in kiota and for reaching out.
It is in fact invalid, see paths object, under path templating matching, the second example

The following paths are considered identical and invalid:

  • /pets/{petId}
  • /pets/{name}

The easiest thing for you to do at this point is probably to copy the description locally, edit it so the first path's parameter is now named "parentId" and use that as a source for generation.
You should also reach out to the API producer to let them know so they can fix it in the source.

Let us know if you have further questions.

@ghelyar
Copy link
Author

ghelyar commented Nov 20, 2023

In this case I don't think the paths are identical, one is a sub-path of the other, but there is no ambiguity there, because once you add /additions it no longer matches the first template.

It's not particularly nice though and is probably an edge case at least.

If I have to mess with the openapi then there's not much point to using client generation anyway, building a path is not hard, it's mostly useful for request/response generation.

@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. and removed question Needs: Attention 👋 labels Nov 20, 2023
@baywet baywet modified the milestones: Backlog, Kiota v1.9 Nov 20, 2023
@baywet baywet moved this from Todo to In Progress in Kiota Nov 20, 2023
@baywet
Copy link
Member

baywet commented Nov 20, 2023

Fair enough, I might have been extra zealous when reading the specification here.
I've authored #3760 which improves the deduplication logic, and should result in fewer collisions.
Note: collisions are still possible with this new implementation, the only way to check would be to get an inventory of all sub path parameters and check for collisions on every replacement, but this would be costly.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Kiota Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants