search

octokit / dotnet-sdk

MIT License
57 stars 8 forks source link

[BUG]: `Repos[owner][repo].Issues.Comments[comment_id].GetAsync()` always throws #117

Open IEvangelist opened 2 months ago

IEvangelist commented 2 months ago

What happened?

When I attempt to perform the following REST call using the SDK client, it throws an error:

var tokenProvider = new TokenProvider(Environment.GetEnvironmentVariable("GITHUB_TOKEN") ?? "");
var adapter = RequestAdapter.Create(new TokenAuthProvider(tokenProvider));

var client = new GitHubClient(adapter);

var owner = "DecimalTurn";
var repo = "VBA-on-GitHub-Automations";
var issueCommentId = 2310943199;

try
{
    var issueComment = await client.Repos[owner][repo].Issues.Comments[(int)issueCommentId].GetAsync();
    if (issueComment is { })
    {
        // This is never hit, as the above API always throws.
    }
}
catch (Exception ex)
{
    if (Debugger.IsAttached)
    {
        Debugger.Break();
    }
}

The error I'm seeing is as follows:

Message: 
GitHub.Models.BasicError : Exception of type 'GitHub.Models.BasicError' was thrown.

  Stack Trace: 
HttpClientRequestAdapter.ThrowIfFailedResponse(HttpResponseMessage response, Dictionary`2 errorMapping, Activity activityForAttributes, CancellationToken cancellationToken)
HttpClientRequestAdapter.SendAsync[ModelType](RequestInformation requestInfo, ParsableFactory`1 factory, Dictionary`2 errorMapping, CancellationToken cancellationToken)
HttpClientRequestAdapter.SendAsync[ModelType](RequestInformation requestInfo, ParsableFactory`1 factory, Dictionary`2 errorMapping, CancellationToken cancellationToken)
WithComment_ItemRequestBuilder.GetAsync(Action`1 requestConfiguration, CancellationToken cancellationToken)
GitHubClientTests.GitHubClientGetsIssueCommentTest() line 45
--- End of stack trace from previous location ---

However, if I run the following code-to manually use an HttpClient outside the library (it works!):

var client = new HttpClient();

client.DefaultRequestHeaders.UserAgent.Add(new("Test", "0.1"));

var token = Environment.GetEnvironmentVariable("GITHUB_TOKEN");
client.DefaultRequestHeaders.Authorization = new("Bearer", token);

var owner = "DecimalTurn";
var repo = "VBA-on-GitHub-Automations";
var issueCommentId = 2310943199;

try
{
    var response = await client.GetAsync($"""
        https://api.github.com/repos/{owner}/{repo}/issues/comments/{issueCommentId}
        """);

    response.EnsureSuccessStatusCode();

    var json = await response.Content.ReadAsStringAsync();
    if (json is { })
    {
        // This gets here, and works with the expected JSON
    }
}
catch (Exception ex)
{
    if (Debugger.IsAttached)
    {
        Debugger.Break();
    }
}

The expected JSON payload is as follows:

Click to expand JSON ```json { "url": "https://api.github.com/repos/DecimalTurn/VBA-on-GitHub-Automations/issues/comments/2310943199", "html_url": "https://github.com/DecimalTurn/VBA-on-GitHub-Automations/issues/79#issuecomment-2310943199", "issue_url": "https://api.github.com/repos/DecimalTurn/VBA-on-GitHub-Automations/issues/79", "id": 2310943199, "node_id": "IC_kwDOMhkZW86JvjHf", "user": { "login": "DecimalTurn", "id": 31558169, "node_id": "MDQ6VXNlcjMxNTU4MTY5", "avatar_url": "https://avatars.githubusercontent.com/u/31558169?u=60e9c89711d781983b36f3701037bb2af138456c&v=4", "gravatar_id": "", "url": "https://api.github.com/users/DecimalTurn", "html_url": "https://github.com/DecimalTurn", "followers_url": "https://api.github.com/users/DecimalTurn/followers", "following_url": "https://api.github.com/users/DecimalTurn/following{/other_user}", "gists_url": "https://api.github.com/users/DecimalTurn/gists{/gist_id}", "starred_url": "https://api.github.com/users/DecimalTurn/starred{/owner}{/repo}", "subscriptions_url": "https://api.github.com/users/DecimalTurn/subscriptions", "organizations_url": "https://api.github.com/users/DecimalTurn/orgs", "repos_url": "https://api.github.com/users/DecimalTurn/repos", "events_url": "https://api.github.com/users/DecimalTurn/events{/privacy}", "received_events_url": "https://api.github.com/users/DecimalTurn/received_events", "type": "User", "site_admin": false }, "created_at": "2024-08-26T19:39:32Z", "updated_at": "2024-08-26T19:39:32Z", "author_association": "OWNER", "body": "Let's strap on your boots!", "reactions": { "url": "https://api.github.com/repos/DecimalTurn/VBA-on-GitHub-Automations/issues/comments/2310943199/reactions", "total_count": 1, "+1": 0, "-1": 0, "laugh": 0, "hooray": 0, "confused": 0, "heart": 0, "rocket": 0, "eyes": 1 }, "performed_via_github_app": null } ```

Versions

Code of Conduct

github-actions[bot] commented 2 months 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 2 months ago

This is interesting! It's due to an issue in our specification.

What's going on is that issueCommentId is originally defined as an unsigned integer. You're casting it to an integer because that's what Issues.Comments requires, but 2310943199 is larger than the Int32 max value of 2147483647, so we're getting an overflow and what's actually going over the wire is a request for the comment ID of -1984024097. The request is then 404ing, resulting in the error.

The offending integer definition is located at https://github.com/octokit/dotnet-sdk/blob/3cdade92ad2737f1e304a2ebde5028e5b0f36d2e/src/GitHub/Repos/Item/Item/Issues/Comments/CommentsRequestBuilder.cs#L23.

The offending JSON schema looks like: 

      "comment-id": {
        "name": "comment_id",
        "description": "The unique identifier of the comment.",
        "in": "path",
        "required": true,
        "schema": {
          "type": "integer"
        }
      },

and it resides in github/rest-api-description. This is something we'll have to fix on our backend before the changes can percolate through to the generator.

IEvangelist commented 2 months ago

This is something we'll have to fix on our backend before the changes can percolate through to the generator.

Thank you so much, but how can I track when this happens? Also, there have historically been issues between int and long in the Octokit.net SDK. I'm curious if this is something that can be evaluated as a whole to ensure that all values are correct in the API description.

IEvangelist commented 2 months ago

For example, every place that references an issue or pull request by int is wrong.

They're all over the place... /cc @kfcampbell and @nickfloyd

nickfloyd commented 1 week ago

Circling back around on this. In our data model there are two ids representing an issue for instance.

One not scoped, which is id that is of type int64 (or long) GET /issues/{id} One scoped, which is number that is of type int32 (or int) GET /repo/{repo_id}/issues/{number}

Are you seeing an issue with the scoped id? I had a look at the DB schema on the back end, I definitely could've missed it but I didn't see instances where these were longs. If it is a problem then we'll need to do another sweep on number as well, just to cover our bases.

Let me know what you are seeing if you get the chance, and sorry for the trouble again.