Support oneOf path paramaters

[caleblloyd]

Given the schema

{
  "paths": {
    "/repos/{owner}/{repo}/actions/workflows/{workflow_id}": {
      "get": {
        "summary": "Get a workflow",
        "description": "Gets a specific workflow. You can replace `workflow_id` with the workflow\nfile name. For example, you could use `main.yaml`.\n\nAnyone with read access to the repository can use this endpoint.\n\nOAuth app tokens and personal access tokens (classic) need the `repo` scope to use this endpoint with a private repository.",
        "tags": [
          "actions"
        ],
        "operationId": "actions/get-workflow",
        "externalDocs": {
          "description": "API method documentation",
          "url": "https://docs.github.com/rest/actions/workflows#get-a-workflow"
        },
        "parameters": [
          {
            "$ref": "#/components/parameters/owner"
          },
          {
            "$ref": "#/components/parameters/repo"
          },
          {
            "$ref": "#/components/parameters/workflow-id"
          }
        ],
     ...
  }
  "components": {
    "parameters":  {
      "workflow-id": {
        "name": "workflow_id",
        "in": "path",
        "description": "The ID of the workflow. You can also pass the workflow file name as a string.",
        "required": true,
        "schema": {
          "oneOf": [
            {
              "type": "integer"
            },
            {
              "type": "string"
            }
          ]
        }
      }
  }
}

A query paramater indexer is only made for the integer use case:

        public global::GitHub.Repos.Item.Item.Actions.Workflows.Item.WithWorkflow_ItemRequestBuilder this[int position]
        {
            get
            {
                var urlTplParams = new Dictionary<string, object>(PathParameters);
                urlTplParams.Add("workflow_id", position);
                return new global::GitHub.Repos.Item.Item.Actions.Workflows.Item.WithWorkflow_ItemRequestBuilder(urlTplParams, RequestAdapter);
            }
        }

Could it also add an indexer for the string use case?

Generated using dotnet tool install --global Microsoft.OpenApi.Kiota --version 1.22.3

[baywet]

Hi @caleblloyd
Thank you for using kiota and for reaching out.

Purely from a description perspective, what's the value of describing the parameter as a one of string / number? It is functionally equivalent to just a string type since path segments are not "typed" in any way.

[caleblloyd]

Purely from a description perspective, what's the value of describing the parameter as a one of string / number? It is functionally equivalent to just a string type since path segments are not "typed" in any way.

Great point! I think it would mainly be the ToString() serialization benefit? For example a fictitious start field that takes a date/time or a duration:

{
  "components": {
    "parameters": {
      "start": {
        "name": "start",
        "in": "path",
        "description": "The start date/time, can be an exact date/time or duration from the current time, such as -4H",
        "required": true,
        "schema": {
          "oneOf": [
            {
              "type": "string",
              "format": "date-time"
            },
            {
              "type": "string",
              "format": "duration"
            }
          ]
        }
      }
    }
  }
}

Of course this could be described as a string but then it would be up to the caller to choose the right type and call ToString(). Whereas overloaded setters could ensure that only the DateTime or Duration objects could be set.

If it is too difficult to implement overloaded setters, should Kiota fallback to generating string input when it encounters a oneOf in query parameter? In the original issue description with the int/string parameter from the GitHub client, it appears to have chosen the first oneOf entry which is an int, so it's not possible to set it as a string

[baywet]

Thank you for the additional information.

The format argument makes sense, there's no point sending a request to the service if the format doesn't match and the request is going to end up with a 400 response.

The challenge is mostly for other languages than dotnet. I believe that dotnet will be happy with multiple indexer overloads as long as the argument type is different. For other languages, these get translated as methods. And some of them won't accept defining methods with the same name, number of parameters, return type, but different parameter type. (Java, Go, TypeScript...)

We could give different names to those methods based on the argument type like "byWorkflowIdInt" and "byWorkflowIdString", but this would represent a breaking change and lead to a poor developer experience.

Alternatively, when a one of is presented, we could order the types by a deterministic ordering scheme, probably string first, and always present "the largest type available" to avoid blocking scenarios like yours.

What do you think?

[caleblloyd]

For other languages, these get translated as methods. And some of them won't accept defining methods with the same name, number of parameters, return type, but different parameter type.

Ah yes, that is certainly a restriction.

We could give different names to those methods based on the argument type like "byWorkflowIdInt" and "byWorkflowIdString", but this would represent a breaking change and lead to a poor developer experience.

Another option could be introducing wrapper types, which are essentially a container for a string with a setter for each of the valid types. This could be done in all languages, or only languages that don't support overloads or Discriminated Unions. This is still a more complex Developer Experience but at least keeps the method names the same, and still a breaking change for oneOf path params.

Alternatively, when a one of is presented, we could order the types by a deterministic ordering scheme, probably string first, and always present "the largest type available" to avoid blocking scenarios like yours.

If a single type must be chosen, I am not sure how many types would potentially overlap. It'd probably wind up being a string most of the time - in both the int | string example and the date-time | duration example, the lowest common denominator for both of those is string.

I guess int | float could be presented as just a float, but still there can be weird serialization issues where a float 3 becomes 3.00000000001 so even that may be better off presented as a string and let the user serialize the string.

[baywet]

Thank you for your patience.

A - I like the wrapper type approach, since the fluent API surface would essentially need to call ToString (or equivalent) when building the set of parameters, and all the logic can be offloaded there. It can also evolve over time nicely (description changes and additional types are added) and doesn't require complex overloads.

B - The alternative being to throw our hands in the air and say "anything more complex than a single scalar type is a string".

I want to be transparent about the fact that:

1. our team is seriously understaffed given the ongoing projects.
2. this falls in the extremely low priorities on our end given the "lack of popular demand" at this point.
3. we'd still be happy to review any contribution.

With that in mind, I feel like the B option is probably a more reasonable ask here. Thoughts?

Is this something you'd like to submit a pull request for provided some guidance?

[caleblloyd]

Sure, I could submit a PR given some guidance. Would it need to go to the main Kiota library and support all languages? Or only this repo for .NET?

[baywet]

I believe this will be a generation only kind of change, and it'll only need to be implemented with the core generation (which will apply to all languages) if we go with option B.

Here is where the indexer parameter gets created.

kiota/src/Kiota.Builder/KiotaBuilder.cs

Line 1058 in 06340d5

private CodeParameter GetIndexerParameter(OpenApiUrlTreeNode currentNode, OpenApiUrlTreeNode parentNode)

And here is an example of a unit test.

kiota/tests/Kiota.Builder.Tests/KiotaBuilderTests.cs

Line 5096 in 06340d5

public void IdsResultInIndexers()

Let us know if you have any additional comments or questions.