octokit / dotnet-sdk

MIT License
54 stars 8 forks source link

[BUG]: Repos[org][repo].Contents[path].GetAsync does not return file contents #105

Open qin-guan opened 1 month ago

qin-guan commented 1 month ago

What happened?

Calling the Repo[org][repo].Contents[path].GetAsync() function does not deserialize the response correctly.

In the HTTP response, there is a content key which should be deserialized, but the parser appears to look at content-file instead.

For now I am using a workaround by manually getting the request information and sending it to HttpClient, which provides the correct response.

Versions

GitHub.Octokit.SDK v0.0.23 .NET v8

Code of Conduct

github-actions[bot] commented 1 month ago

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

kfcampbell commented 1 month ago

Thank you for reporting. Do you mind sharing a code snippet of the workaround you're using?

I wonder if we have a broader pattern of schema errors with discriminator property names.

qin-guan commented 1 month ago

Hey, sure! Some rough code below

public record GetRepoContentResponse(
    [property: JsonPropertyName("download_url")]
    string DownloadUrl,
    [property: JsonPropertyName("sha")] string Sha
);

public class GitHubFileContentHttpClient(HttpClient httpClient)
{
    /// <summary>
    /// Sends a request for <see cref="WithPathItemRequestBuilder.WithPathItemRequestBuilderGetQueryParameters"/>
    /// </summary>
    /// <param name="requestMessage">Message after using <see cref="Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter"/> ConvertToNativeRequestAsync</param>
    /// <param name="ct">Cancellation token</param>
    /// <returns>If the file was found, return <see cref="GetRepoContentResponse"/>. Else, return null</returns>
    /// <exception cref="Exception">Non success status code response, other than NotFound</exception>
    public async Task<(string, GetRepoContentResponse)?> GetFileContentAsync(
        HttpRequestMessage requestMessage,
        CancellationToken ct
    )
    {
        var rawRes = await httpClient.SendAsync(requestMessage, ct);
        if (rawRes.StatusCode == HttpStatusCode.NotFound)
        {
            return null;
        }

        if (rawRes.StatusCode != HttpStatusCode.OK)
        {
            throw new Exception(
            $"Expected success status code for {requestMessage.RequestUri} but received {rawRes.StatusCode}.");
        }

        var res = await rawRes.Content.ReadFromJsonAsync<GetRepoContentResponse>(cancellationToken: ct);
        if (res is null)
        {
            return null;
        }

        return (await httpClient.GetStringAsync(res.DownloadUrl, ct), res);
    }
}

To use


var kiotaFileRequestInformation =
    gitHubClient.Repos[orgName][repo.Name]
        .Contents[filePath]
        .ToGetRequestInformation(
            options =>
            {
                options.QueryParameters.Ref = branch.Name;
                options.Headers["accept"] = ["application/vnd.github+json"];
            });

var reqInfo = await requestAdapter.RequestAdapter
    .ConvertToNativeRequestAsync<HttpRequestMessage>(
        kiotaFileRequestInformation);

var fileContentResponse = await gitHubFileContentClient.GetFileContentAsync(reqInfo);
if (!fileContentResponse.HasValue)
{
    return;
}

I cannot remember exactly why I decided to go with using download_url instead of fetching from content directly. It may be because the GitHub API does not return content entirely in base64, I believe I experienced a few instances where the field values contained \n values and didn't have enough time to figure it out 😆

qin-guan commented 1 month ago

I wonder if we have a broader pattern of schema errors with discriminator property names.

It could be the case, the OpenAPI spec file looks correct to me, so this could also be an upstream issue with Kiota

nickfloyd commented 3 weeks ago

Hey @qin-guan we finally tracked this down - it has to do with the fact that our OpenAPI definitions for content does not have a discriminator in it to help it understand what, of the 4 types, should it be.

I'm working to get a PR in to change this in the GitHub OpenAPI definitions and will let you know when that has shipped.

nickfloyd commented 2 weeks ago

@qin-guan Just a quick update. I have created a PR for the GitHub OpenAPI descriptions that should fix this issue. Once merged our auto generation should take over and update the the SDKs to fix the issue. I'll let you know when that happens.

qin-guan commented 2 weeks ago

Hey @nickfloyd thanks for the updates!! So glad to see that a fix has been found, thank you! Happy to test it out once it's been released

nickfloyd commented 1 week ago

@qin-guan Hey just a quick follow up, we have made the schema change and are in the middle of making a tooling change to support it. Thanks for the patience here.

qin-guan commented 4 days ago

Hey! Thanks for the updates yet again!! I look forward to seeing it out 🎉