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

Multiple consecutive path parameters result in unusable code #2527

Closed
andreaTP opened this issue Apr 5, 2023 · 5 comments · Fixed by #2532
Closed

Multiple consecutive path parameters result in unusable code #2527

andreaTP opened this issue Apr 5, 2023 · 5 comments · Fixed by #2532
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Milestone

Comments

@andreaTP
Copy link
Contributor

andreaTP commented Apr 5, 2023

In presence of multiple consecutive path parameters, the generator is emitting unusable code (at least in Go and Java).

And endpoint like:

  /baz/{x}/{y}:
    get:
      responses:
        "200":
          description: Something
          content:
            type: string
      parameters:
        - in: path
          name: x
          required: true
          schema:
            type: string
        - in: path
          name: y
          required: true
          schema:
            type: string

Is going to produce Builders up to the first parameter(x in the example) but will not include the second and the endpoint is unusable from Kiota-generated code (despite the code compiles).

Reproducer

  • git clone [email protected]:andreaTP/kiota.git --depth 1 --branch consecutive-path-params
  • cd kiota
  • (cd src/kiota && dotnet build)
  • ./it/generate-code.ps1 -descriptionUrl ./tests/Kiota.Builder.IntegrationTests/NoUnderscoresInModel.yaml -language java -dev
  • ./it/exec-cmd.ps1 -descriptionUrl ./tests/Kiota.Builder.IntegrationTests/NoUnderscoresInModel.yaml -language java

The test will execute, but you can notice that the generated code doesn't allows to call the baz endpoint: https://github.com/andreaTP/kiota/blob/a70d00049a3135fc7be0e0420db51854a9c2919a/it/java/no-underscores/src/test/java/BasicAPITest.java#L18-L20

Proposed solution

In presence of consecutive path parameters, we can generate "synthetic" path segments(e.g. and or then) enabling the rest of the generation to be effective.

@lucamolteni is stepping up as a volunteer to take a stab at this issue if we have an agreement on the solution.

@baywet baywet self-assigned this Apr 5, 2023
@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. and removed Needs: Triage 🔍 labels Apr 5, 2023
@baywet baywet added this to Kiota Apr 5, 2023
@baywet baywet moved this to Todo in Kiota Apr 5, 2023
@baywet baywet added this to the Kiota v1.3 milestone Apr 5, 2023
@baywet
Copy link
Member

baywet commented Apr 5, 2023

Thanks for reporting this. Before we invest time into fixing this, I'd like to make sure I understand the problem properly.

GitHub has this API pattern /repos/{owners}/{repo}/pulls.
While I've been running all my demos in dotnet without problem, this

client.Repos["owner"]["repo"].Pulls.GetAsync();

Should roughly give us that in Java

client.ReposByOwner("owner").ByRepo("repo").Get();

Are you saying that an API segment is missing here?

If so the problem is probably located somewhere around here.

protected static void ReplaceIndexersByMethodsWithParameter(CodeElement currentElement, bool parameterNullable, string? methodNameSuffix = default)

Also as we try to correct this, we really need to respect the pattern of on path segment maps to one code path segment. So no aggregating to ByOwnerAndRepo in this example.

@andreaTP
Copy link
Contributor Author

andreaTP commented Apr 5, 2023

Are you saying that an API segment is missing here?

That's correct and is included in the reproducer.
A possibility is that the bug applies only when the pattern appears at the end of the path, we should definitely check that!

@baywet
Copy link
Member

baywet commented Apr 5, 2023

Alright, thanks for the precision here. This last us to close on a conversation we had forgotten about. See the details in #2528
Please hold any work on this until this other issue is resolved. It's become the highest priority for me to unblock internal product release so expect to see some movement pretty soon.

@baywet
Copy link
Member

baywet commented Apr 6, 2023

update on this one, here is the PR #2532
I believe it's going to fix at the same time the issue you were mentioning, but if you could do an extra round of testing it'd be very much appreciated.

It should look something like that now

client.repos().withOwner("owner").withRepo("repo").get();

@baywet baywet moved this from Todo to In Progress in Kiota Apr 6, 2023
@baywet baywet modified the milestones: Kiota v1.3, Kiota v1.2 Apr 6, 2023
@baywet baywet linked a pull request Apr 6, 2023 that will close this issue
@baywet
Copy link
Member

baywet commented Apr 6, 2023

and while working on testing the previous PR, I found another edge case, which I don't thing impacts your scenario here but just in case #2534

@github-project-automation github-project-automation bot moved this from In Progress to Done in Kiota Apr 11, 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