search

microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.92k stars 203 forks source link

Requests that redirect to download resources are not performed #5208

Closed kfcampbell closed 1 month ago

kfcampbell commented 2 months ago

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Nuget tool

Client library/SDK language

Csharp

Describe the bug

I am trying to download a tarball of a GitHub repo using a Kiota-generated SDK with Kiota version v1.14.0.

Actual behavior: the request simply returns a Task, so I cannot get the content of the repo even though the request succeeds.

Expected behavior

I should be able to receive a Task<byte[]> or a Task<Stream> so that I may download the repository contents.

How to reproduce

Use octokit/dotnet-sdk and edit the the example file stage/dotnet/dotnet-sdk/cli/Authentication/PersonalAccessToken.cs provided, or else construct and authenticate your own GitHub client from Kiota, and make the following request:

// create client
var tokenProvider = new TokenProvider(Environment.GetEnvironmentVariable("GITHUB_TOKEN") ?? "");
var adapter = RequestAdapter.Create(new TokenAuthProvider(tokenProvider));
var gitHubClient = new GitHubClient(adapter);

// no return value
await gitHubClient.Repos["octokit"]["dotnet-sdk"].Tarball["main"].GetAsync();

Open API description file

Full description: https://raw.githubusercontent.com/github/rest-api-description/main/descriptions/api.github.com/api.github.com.json

Relevant content: 

    "/repos/{owner}/{repo}/tarball/{ref}": {
      "get": {
        "summary": "Download a repository archive (tar)",
        "description": "Gets a redirect URL to download a tar archive for a repository. If you omit `:ref`, the repository’s default branch (usually\n`main`) will be used. Please make sure your HTTP framework is configured to follow redirects or you will need to use\nthe `Location` header to make a second `GET` request.\n\n> [!NOTE]\n> For private repositories, these links are temporary and expire after five minutes.",
        "tags": [
          "repos"
        ],
        "externalDocs": {
          "description": "API method documentation",
          "url": "https://docs.github.com/rest/repos/contents#download-a-repository-archive-tar"
        },
        "operationId": "repos/download-tarball-archive",
        "parameters": [
          {
            "$ref": "#/components/parameters/owner"
          },
          {
            "$ref": "#/components/parameters/repo"
          },
          {
            "name": "ref",
            "in": "path",
            "required": true,
            "x-multi-segment": true,
            "schema": {
              "type": "string"
            }
          }
        ],
        "responses": {
          "302": {
            "description": "Response",
            "headers": {
              "Location": {
                "example": "https://codeload.github.com/me/myprivate/legacy.zip/master?login=me&token=thistokenexpires",
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        },
        "x-github": {
          "githubCloudOnly": false,
          "enabledForGitHubApps": true,
          "category": "repos",
          "subcategory": "contents"
        }
      }
    },

Kiota Version

v1.14.0

Latest Kiota version known to work for scenario above?(Not required)

No response

Known Workarounds

No response

Configuration

No response

Debug output

No response

Other information

As specified in https://github.com/microsoft/kiota/issues/2597#issuecomment-1625603326, the MSGraph reports API is a similar case, and Kiota already (correctly) generates a Task<Stream>.

This is probably another spec issue on our part, and I'd love some help/pointers for how to correct it!

kfcampbell commented 2 months ago

It looks like in the MS Graph spec that does this correctly, the spec makes no mention of a 302 at all: 

  '/reports/getSharePointSiteUsageDetail(date={date})':
    description: Provides operations to call the getSharePointSiteUsageDetail method.
    get:
      tags:
        - reports.Functions
      summary: Invoke function getSharePointSiteUsageDetail
      description: Get details about SharePoint site usage.
      externalDocs:
        description: Find more info here
        url: https://learn.microsoft.com/graph/api/reportroot-getsharepointsiteusagedetail?view=graph-rest-1.0
      operationId: reports.getSharePointSiteUsageDetail-a4c0
      responses:
        2XX:
          description: Success
          content:
            application/octet-stream:
              schema:
                type: object
                properties:
                  value:
                    type: string
                    format: base64url
        4XX:
          $ref: '#/components/responses/error'
        5XX:
          $ref: '#/components/responses/error'
      x-ms-docs-operation-type: function
    parameters:
      - name: date
        in: path
        description: 'Usage: date={date}'
        required: true
        schema:
          pattern: '^[0-9]{4,}-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])$'
          type: string
          format: date
    x-ms-docs-grouped-path:
      - '/reports/getSharePointSiteUsageDetail(period=''{period}'')'

and simply defines the final path as a 2xx octet-stream, so we don't "document" the redirect in the spec at all and trust the middleware to handle it. Is that the preferred path here?

kfcampbell commented 2 months ago

One thing I've noticed in https://github.com/OAI/OpenAPI-Specification/issues/2512#issuecomment-2130179975 is that it's possible to define the content of the Location header inside the redirect object. However, Kiota does not appear to parse that when given as:

"302": {
            "description": "Response",
            "headers": {
              "Location": {
                "content": {
                  "application/octet-stream": {
                    "schema": {
                      "type": "string",
                      "format": "binary"
                    }
                  }
                },
                "example": "https://codeload.github.com/me/myprivate/legacy.zip/master?login=me&token=thistokenexpires",
                "schema": {
                  "type": "string"
                }
              }
            }
          }

or even 

"302": {
            "description": "Response",
            "headers": {
              "Location": {
                "content": {
                  "application/octet-stream": {
                    "schema": {
                      "type": "string",
                      "format": "binary"
                    }
                  }
                }
              }
            }
          }

The only snippet that correctly generates a Task<Stream> response capable of downloading a repo tarball is to leave the redirect out of the schema entirely, which I don't love: 

          "2XX": {
            "description": "Success",
            "content": {
              "application/octet-stream": {
                "schema": {
                  "type": "string",
                  "format": "binary"
                }
              }
            }
          }

Does Kiota indeed respect the content of the redirect given in the response, and I've just presented it incorrectly?

baywet commented 2 months ago

Hi @kfcampbell Thanks for reaching out and for the very detailed information here.

and simply defines the final path as a 2xx octet-stream, so we don't "document" the redirect in the spec at all and trust the middleware to handle it. Is that the preferred path here?

That's a design decision we made, here is the reasoning behind it:

it's possible to define the content of the Location header inside the redirect object

I wasn't aware this was possible, which would solve point 3 of the previous paragraph. It's not implemented today in kiota.

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

kfcampbell commented 2 months ago

Thanks for the background. One other thing I'm curious about: with both a 302 and a 2XX response defined, Kiota seems to favor the 302 response, and a Task method is generated, rather than a Task<Stream> method. Is that intended? It seems like it should be the other way around.

baywet commented 2 months ago

This is probably a side effect of this condition: https://github.com/microsoft/kiota/blob/9b943845ea8e90b3d5d7d4ee30b4fee0f9d36ab1/src/Kiota.Builder/KiotaBuilder.cs#L1250

There should probably be a case that takes precedence when application/octet-stream is explicitly defined for a status code, it should be binary before it evaluates the no content status codes.

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

kfcampbell commented 2 months ago

it's possible to define the content of the Location header inside the redirect object

Now that I reread https://github.com/OAI/OpenAPI-Specification/issues/2512#issuecomment-2130179975 with fresh eyes, and read the OpenAPI spec directly, I actually think I was mistaken and just wishfully thinking. I think the comment is referring to how to add a Location header URL that's not URI-encoded, and I saw the schema keyword and got carried away.

with both a 302 and a 2XX response defined, Kiota seems to favor the 302 response, and a Task method is generated, rather than a Task<Stream> method.

For what it's worth, the more I think about this, the more I'm uncomfortable with the spec returning a 2xx when the API really returns a 302 (especially as the domain for the resulting URL could be something completely different, like githubusercontent.com or from Azure Blob Storage). Shouldn't the spec define what the API itself returns, and the OpenAPI spec for whatever other service is redirected to then define the 2xx response? What if that service itself has to redirect, or 404s?

There should probably be a case that takes precedence when application/octet-stream is explicitly defined for a status code, it should be binary before it evaluates the no content status codes.

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

I do think this is a reasonable change to make, and I'd be happy to get my feet wet contributing to Kiota (finally).

I'm picturing something like this below line 1161 in KiotaBuilder.cs: 

    private const string RequestBodyPlainTextContentType = "text/plain";
    private const string RequestBodyOctetStreamContentType = "application/octet-stream";

and then around 1250:

else if (modelType is null)
            {
                string returnType;
                if (operation.Responses.Any(static x => x.Value.Content.ContainsKey(RequestBodyOctetStreamContentType)))
                    returnType = "binary";
                else if (operation.Responses.Any(static x => noContentStatusCodes.Contains(x.Key)))
                    returnType = VoidType;
                else if (operation.Responses.Any(static x => x.Value.Content.ContainsKey(RequestBodyPlainTextContentType)))
                    returnType = "string";
                else
                    returnType = "binary";
                return (new CodeType { Name = returnType, IsExternal = true, }, null);
            }

Am I on the right track?

baywet commented 2 months ago

Shouldn't the spec define what the API itself returns, and the OpenAPI spec for whatever other service is redirected to then define the 2xx response? What if that service itself has to redirect, or 404s?

I think this is going to be opinion based and this is one place where OAS has a gap. I can see reasons to argue for "the description should provide full fidelity with what the host will return", but that obviously has shortcomings when implementing clients (i.e. you're not sure what ultimately you're going to get). For those reasons we could almost shove redirects are being "transport concerns" (especially in the scenario of optimizing for large response bodies), making it a cross cutting concern, and describing what the client is ultimately going to get. We've made the opiniated decision of implementing the later, which adds an expectation on the descriptions.

I think your proposed change makes perfect sense!

As for unit tests, this is where it's located

kfcampbell commented 1 month ago

I've opened https://github.com/microsoft/kiota/pull/5246! I'm struggling a bit on the testing portion, with more details in the PR. Thanks for walking me through this so far!