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

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

Closed
1 task done
qin-guan opened this issue Jul 25, 2024 · 10 comments
Closed
1 task done
Assignees
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented

Comments

@qin-guan
Copy link

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

  • I agree to follow this project's Code of Conduct
@qin-guan qin-guan added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Jul 25, 2024
Copy link

👋 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 kfcampbell moved this from 🆕 Triage to 🔥 Backlog in 🧰 Octokit Active Aug 2, 2024
@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Aug 2, 2024
@kfcampbell
Copy link
Member

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
Copy link
Author

qin-guan commented Aug 5, 2024

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
Copy link
Author

qin-guan commented Aug 5, 2024

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 nickfloyd self-assigned this Aug 20, 2024
@nickfloyd nickfloyd moved this from 🔥 Backlog to 🏗 In progress in 🧰 Octokit Active Aug 23, 2024
@nickfloyd
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

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

@nickfloyd
Copy link
Contributor

This has been fixed and released: https://github.com/octokit/dotnet-sdk/releases/tag/v0.0.30

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in 🧰 Octokit Active Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
Archived in project
Development

No branches or pull requests

3 participants