Fix nodes merging to update children paths and path parameters

[andreaTP]

After much thinking about it, I believe that this is the correct solution even if it might have performance implications.

Fixes #4151
fixes #4085 .

What's happening?

The MergeIndexNodesAtSameLevel deduplicates endpoints containing different path parameters forcing their name to become {entity-id}.

Why?

The specification seems to indicate that this pattern is invalid.
I tend to believe that this section is slightly under-specified, as it doesn't clearly state what should happen when the path parameter names are diverging, and, the matching of the path template is unique, e.g.:

  /pets/{petId}/fur
  /pets/{name}/hair

cc. Maybe @darrelmiller can shed light on this

Current tradeoff

Under the assumption that the Path Templating Matching is invalid when the names of path segments are diverging the current optimization of merging the index nodes can be performed.
This will somehow also "help" as @baywet mentioned:

since this is a mistake often made in descriptions, kiota attempts to merge the nodes

I believe that the current approach is misleading as we tend to agree that the path parameter names should be preserved and we got reports about collision issues with small and large API definitions.

Proposed solution

Assuming that:

• we want the path parameter names to exactly match the definition
• we accept that any heuristic for deduplicating the path param names is going to generate bugs or, at least, unexpected results sooner or later(I don't know how to meaningfully distinguish user/users-id, foo/users and template_owner/owner at the same time)
• this might increase the number of operations that Kiota needs to do on a definition

Simply removing the merge function, we will be more closely mapping the original OpenAPI definition and avoiding this entire class of issues.

[andreaTP]

The integration tests are reporting some failures we have to decide how to tackle:

• apisguru::meraki.com uses {networkId} and {network_id} in the same positional place and it breaks Go and Python
• the issues with Ruby should be easy to resolve as it's only about lining
• seems like this change is triggering another bug/something in Python, I'll have a closer look if we decide to move on with this change

[baywet]

Thanks for looking into this.
NOT merging the nodes has multiple downstream effects:

• some of the request builders won't be usable as when we project the nodes to the CodeDOM, we only carry one of the templates, and in the other cases, it'll be inaccurate/won't match the parameter names (especially for sub-nodes)
• the filtering will not be inaccurate.
• UI might be confusing for the show command/extensibility that shows the URI space (like the vscode extension), and in turns lead to selection inaccuracies

This change won't have performance impacts however (actually it'll be the other way around)

I do believe we should keep the merging, but fix it so it works as expected for subnodes and so on.

[andreaTP]

I do believe we should keep the merging, but fix it so it works as expected for subnodes and so on.

Do you have something in mind?

To keep the merging we need anyhow to generate new "SegmentParameterNames" and I'm not able to come up with a heuristic proposal that is not going to fail on edge cases.

[baywet]

I think the problem lies in the fact the method needs to be splat.

It's mixing concerns of detecting nodes to merge, and applying the replacement to a whole branch.
And on top of that, I don't think it's replacing parameter names in sub-nodes today (haven't looked at the code in a while)

[baywet]

We might also take this opportunity to move that method outside of the request builder to ease up testability. I'll try to start looking at that momentarily (once I get to the never-ending list of GitHub notifications)

[andreaTP]

Agreed, I'll wait for your follow-up, I might be missing something.

[baywet]

I have just force pushed to this PR, at this point the unit test you've added still fails, but this is expected as I didn't change the logic yet.
This moves things to OpenAPIUrlTreeNodeExtensions to de-clutter kiota builder and kiota builder tests.
This also has the benefits to simplify the unit tests structure (not reliant on the code DOM), and make them faster.
Now I'll start looking at the test case you've added.
If in the meantime you could start adding one for #4085 even if it fails, it'd be super helpful.

[baywet]

I've just pushed another patch here that fixes a couple of things:

• the parameters in the template for child nodes were not getting renamed accordingly to their parents, I believe this was impacting Overlapping paths cause invalid URLs #4085
• the path parameters were also not getting renamed (if they were present) this also most likely impacted Overlapping paths cause invalid URLs #4085
• we were using the parent node to build the child node parameter name, while this works well for Microsoft Graph, it creates confusion for any other API (especially for GitHub which happens to have repos/{owner}/{repos} which would be translated to repos/{repos-id}/{owner-id})

I don't think the changes implemented here would lead to any breaking changes in the vast majority of cases.

As you know I have personal things going on right now that prevent me from giving this my 100%, so here are my requests:

• @andreaTP can you 1. add a unit test for Overlapping paths cause invalid URLs #4085 and see if we actually solved it, 2. run some integration tests with GitHub's API please?
• @andrueastman can you 1. carefully read the changes in expected paths in this commit especially the first unit test and see if any reg flag that would impact how we project namespaces/classes, 2. run a smoke test generation against Graph for dotnet and another language that doesn't support indexers.

Thanks for the help in all this!

[andrueastman]

I've just pushed another patch here that fixes a couple of things:

• the parameters in the template for child nodes were not getting renamed accordingly to their parents, I believe this was impacting Overlapping paths cause invalid URLs #4085
• the path parameters were also not getting renamed (if they were present) this also most likely impacted Overlapping paths cause invalid URLs #4085
• we were using the parent node to build the child node parameter name, while this works well for Microsoft Graph, it creates confusion for any other API (especially for GitHub which happens to have repos/{owner}/{repos} which would be translated to repos/{repos-id}/{owner-id})

I don't think the changes implemented here would lead to any breaking changes in the vast majority of cases.

As you know I have personal things going on right now that prevent me from giving this my 100%, so here are my requests:

• @andreaTP can you 1. add a unit test for Overlapping paths cause invalid URLs #4085 and see if we actually solved it, 2. run some integration tests with GitHub's API please?
• @andrueastman can you 1. carefully read the changes in expected paths in this commit especially the first unit test and see if any reg flag that would impact how we project namespaces/classes, 2. run a smoke test generation against Graph for dotnet and another language that doesn't support indexers.

Thanks for the help in all this!

Just to confirm, the changes result in no diff in graph SDKs (tested with Java,Go,C#) so no regressions introduced from that side.

From the looks of it, it also should have no impact on classes (as indexer requestBuilder classes get normalized as a ItemRequestBuilder class and do not depend on the segment name). Same story for namespaces as they also do not depend on the segment name and end up being remapped to a namespace called Item by the Kiota builder. Will keep checking as @andreaTP updates the tests for the other scenario here.

[andreaTP]

For full transparency, I'm a bit busy today, but I'll try to target the comments by the end of Mon 👍 on my TODO.

[andreaTP]

@baywet I added the requested tests, take aways:

• I still have doubts that the proposed resolution is going in the right direction
• found a bug in the Integration tests (e.g.) the Mock Server tests where not running as expected

[baywet]

I have just pushed a couple of updates:

• the fluent API path in the integration test, you can now see this is clean, and there's no post operation under /repos/{owner}/{repo} or under /repos/{template_owner}/{template_repo}. it's in fact under /repos/{template_owner}/{template_repo}/generate
• the expected tree node structure in your repro test. Again this is because we're going at it alphabetically, and not by word proximity. (see my other comment), we could argue about the pertinence of the -id suffix.

The integration test seems to be building except for an instance of #3398 at it\java\src\apisdk\repos\item\item\autolinks\AutolinksPostRequestBody.java:33 (remove the quotes after generation).

Let me know what you think, I believe we're getting close to a final resolution on this one.

[andreaTP]

Thanks for the update @baywet I'll have a closer look tomorrow!
Regarding appending the -id, my preference would be to not do so, as it can lead to more clashes and overall doesn't 1:1 map the user input description.
But I have to say that this might be a bit subjective, so, not fussy on it.

[andreaTP]

I've reviewed the PR, and I genuinely appreciate your effort to address this issue while maintaining the current design, @baywet.

However, I believe the outcome still falls short of delivering an optimal user experience and may result in a confusing fluent API.

I may not fully grasp the underlying reason for merging the nodes. Could you kindly explain it to me again? 🙏

I'll be away until Monday. If you proceed with these changes in my absence, I kindly request that you thoroughly document this behavior at the very least.

[baywet]

@andreaTP thank you for the continuous feedback on this one.

On merging the nodes

A description with ambiguous path items is invalid according to the specification. We could perfectly say to the user "sorry, your spec is invalid, fix it first", but experience has shown this is hostile to API consumers who don't necessarily understand the semantics of OpenAPI, don't want to maintain a copy of the description, etc...

The other reason why we're merging the nodes is because it's impossible to have multiple indexers of the same types on the same class in C#. We could perfectly go a different route and replace those indexers by methods with specific names so they don't collide, but I believe this gets us further from the specification's intent to have a consistent API path segmentation. Maybe @darrelmiller could weight in on this one.

The alphabetical selection

This is simply because I couldn't come up with a better rationale, other than using a language library and running words proximity with the previous path segment, which would be costly and sometimes unpredictable. At least it's easy to understand and fast this way.

The id suffix

On this one I'm open to stop appending it, no hard opinion on the matter.

Additional cases

Reviewing the specification, I noticed there might be cases we haven't talked about:

1. /pets/{petId} with /pets/{name}: this is what this PR addresses by merging the nodes. Ending up with client.pets().byPetId("")...
2. /pets/{petId} with /pets/mine: no merging will happen, and I believe we'll end up with both (Java) client.pets.mine()... and client.pets.byPetId("")....
3. /{entity}/me with /books/{id} I don't believe any merging will be done either here. And we'll most likely end up with client.byEntityId("ooo").me().... and client.books().byId("ooo")...

[andreaTP]

Thanks for the feedback!

A description with ambiguous path items is invalid according to the specification. We could perfectly say to the user "sorry, your spec is invalid, fix it first", but experience has shown this is hostile to API consumers who don't necessarily understand the semantics of OpenAPI, don't want to maintain a copy of the description, etc...

This is a bit dissimilar to how I read the spec, and explained on the top my interpretation. I might be wrong, but I would love feedback from @darrelmiller on this one.

Achieving consensus on this aspect is probably key to move forward the discussion.

I'm out till Mon, if you want to merge this one as is, since it's anyhow a step forward, I don't oppose; but I would love to keep the conversation going.

[darrelmiller]

Notes from a conversation between myself and @baywet

The current OpenAPI specification says that the following is invalid

  /pets/{petId}/fur
  /pets/{name}/hair

However, I expect future iterations of the specification will relax this constraint and we know that these already exist in the wild. The notion of a collection of things having multiple candidate keys is a common concept and we should be able to model this.

We currently model these parameter segments as a CodeIndexer DOM model. Currently the CodeIndexer doesn't support the notion of alternate keys. We think it is worth exploring adding support for alternate keys to the CodeIndexer. We can continue to use the first parameter alphabetically as the default primary key, and all other parameters as alternate keys. This allows us to introduce support for a x-primary-key hint in the OpenAPI description that enables a particular path to be stable in its use of syntax like the C# indexer.

It may be possible that different candidate keys return different types. From a logical perspective the different types should be closely related, but from code perspective the types could be different. This does warp the concept of CodeIndexer slightly, but may be tolerable.

Adding this notion of candidate/alternate keys to the code indexer would remove the need for merging of nodes and the language refiners can emit specific named indexer methods for alternate keys and some default indexer method for the primary key, or they can emit specific named indexer methods for all the candidate keys.

[andreaTP]

Thanks a lot @darrelmiller for chiming in, appreciated!

I think that this statement is enough to me:

The current OpenAPI specification says that the following is invalid

Please proceed with the implementation in this PR which reflects the current state of the art and we will iterate next.

Thanks a lot for bearing with me on this @baywet !

[andreaTP]

@baywet I noticed this failure on this PR:
https://github.com/microsoft/kiota/actions/runs/8048623276/job/21987867459#step:14:165

Do you already have an issue tracking it?

[baywet]

Merged, and created #4241
Thanks everyone for the help!

[baywet]

@andreaTP see this comment #4174 (comment)