octokit / octokit.net

A GitHub API client library for .NET
https://octokitnet.readthedocs.io/en/latest/
MIT License
2.62k stars 1.07k forks source link

[BUG]: Error deserializing new issues #2889

Closed brantburnett closed 2 months ago

brantburnett commented 2 months ago

What happened?

I've begun noticing errors deserializing the issue ID number when receiving issue webhooks. The value appears to be an int in the models but it seems GitHub has finally crossed the threshold and needs a long. My internal issue ID number was 2147620872, and the log was for a comment created webhook request.

Versions

1.0.0 (we haven't upgraded in a while) but bug appears to still be present in the main branch

Relevant log output

JSON integer 2147620872 is too large or small for an Int32. Path 'issue.id', line 1, position 612.
  at Newtonsoft.Json.JsonTextReader.ParseReadNumber(ReadType readType, Char firstChar, Int32 initialPosition)\n   at Newtonsoft.Json.JsonTextReader.ReadNumberValue(ReadType readType)\n   at Newtonsoft.Json.JsonTextReader.ReadAsInt32()

Code of Conduct

SeanFlemingMathsPathway commented 2 months ago

I am using version 9.1.2 and I am experiencing a similar failure for newly created issues. Calls to: client.Issue.Get(OrgName, AppName, issueNumber);

Result in the following error and stack trace:

System.OverflowException: Value was either too large or too small for an Int32. at System.Convert.ThrowInt32OverflowException() at System.Convert.ToInt32(Int64 value) at System.Int64.System.IConvertible.ToInt32(IFormatProvider provider) at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider) at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in //Octokit/SimpleJson.cs:line 1437 at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in //Octokit/Http/SimpleJsonSerializer.cs:line 183 at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in //Octokit/SimpleJson.cs:line 1492 at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in //Octokit/Http/SimpleJsonSerializer.cs:line 183 at Octokit.SimpleJson.DeserializeObject(String json, Type type, IJsonSerializerStrategy jsonSerializerStrategy) in //Octokit/SimpleJson.cs:line 584 at Octokit.SimpleJson.DeserializeObject[T](String json, IJsonSerializerStrategy jsonSerializerStrategy) in //Octokit/SimpleJson.cs:line 596 at Octokit.Internal.SimpleJsonSerializer.Deserialize[T](String json) in //Octokit/Http/SimpleJsonSerializer.cs:line 22 at Octokit.Internal.JsonHttpPipeline.DeserializeResponse[T](IResponse response) in //Octokit/Http/JsonHttpPipeline.cs:line 60 at Octokit.Connection.Run[T](IRequest request, CancellationToken cancellationToken, Func 2 preprocessResponseBody) in //Octokit/Http/Connection.cs:line 748 at Octokit.ApiConnection.Get[T](Uri uri, IDictionary 2 parameters) in //Octokit/Http/ApiConnection.cs:line 75

bifacil commented 2 months ago

It seems ans an bug: #1979 (or very very related)

For some reason many are experiencing the issue now...

njagerAAANE commented 2 months ago

experienced this exact issue this morning using 9.1.2 and an older version 8.1.1.

image

matdavies commented 2 months ago

We're experiencing the same issue. Hopefully Michael's PR can be pulled in and released ASAP.

patrickklaeren commented 2 months ago

+1 we're also facing this issue. Has taken a service down. Requires a hotfix IMHO.

System.OverflowException: Value was either too large or too small for an Int32.
  File "Convert.cs", line 303, in void Convert.ThrowInt32OverflowException()
    private static void ThrowInt32OverflowException() { throw new OverflowException(SR.Overflow_Int32); }
  File "Convert.cs", line 264, in object Convert.ChangeType(object value, Type conversionType, IFormatProvider provider)
    return ic.ToInt32(provider);
  File "/_/Octokit/SimpleJson.cs", line 1437, col 17, in object PocoJsonSerializerStrategy.DeserializeObject(object value, Type type)
  File "/_/Octokit/SimpleJson.cs", line 1368, col 13, in object PocoJsonSerializerStrategy.DeserializeObject(object value, Type type)
  File "/_/Octokit/Http/SimpleJsonSerializer.cs", line 205, col 17, in object GitHubSerializerStrategy.DeserializeObject(object value, Type type)
  File "/_/Octokit/SimpleJson.cs", line 1368, col 13, in object PocoJsonSerializerStrategy.DeserializeObject(object value, Type type)
  File "/_/Octokit/Http/SimpleJsonSerializer.cs", line 205, col 17, in object GitHubSerializerStrategy.DeserializeObject(object value, Type type)
  File "/_/Octokit/SimpleJson.cs", line 596, col 13, in T SimpleJson.DeserializeObject<T>(string json, IJsonSerializerStrategy jsonSerializerStrategy)
  File "/_/Octokit/Http/JsonHttpPipeline.cs", line 44, col 13, in IApiResponse<T> JsonHttpPipeline.DeserializeResponse<T>(IResponse response)
  File "/_/Octokit/Http/Connection.cs", line 747, col 13, in async Task<IApiResponse<T>> Connection.Run<T>(IRequest request, CancellationToken cancellationToken, Func<object, object> preprocessResponseBody)
  ?, in async Task<IReadOnlyPagedCollection<TU>> ApiConnection.GetPage<TU>(Uri uri, IDictionary<string, string> parameters, string accepts, ApiOptions options, Func<object, object> preprocessResponseBody)
  ?, in async Task<IReadOnlyList<T>> ApiConnection.GetAll<T>(Uri uri, IDictionary<string, string> parameters, string accepts)+(?) => { }
  ?, in async Task<IReadOnlyList<T>> ApiPagination.GetAllPages<T>(Func<Task<IReadOnlyPagedCollection<T>>> getFirstPage, Uri uri)
  File "D:\a\1\s\src\REDACTED\GitHub\GitHubOrganisationClient.cs", line 36, col 9, in async Task<IReadOnlyList<Issue>> GitHubOrganisationClient.GetIssues(string repositoryName)
JimSuplizio commented 2 months ago

+1, I'm seeing this as well.

sandupetrasco commented 2 months ago

We are also impacted by this. Waiting for a hotfix here 👀

JimSuplizio commented 2 months ago

We're experiencing the same issue. Hopefully Michael's PR can be pulled in and released ASAP.

I don't believe this PR is the right fix. Changing the Id to long isn't going to change the underlying APIs that use this Id, which will still expect integers. It's unclear if this is somehow a problem with GitHub's issue numbering but I believe just changing things to longs might not be the right thing if the underlying API doesn't support it.

brantburnett commented 2 months ago

I don't believe this PR is the right fix. Changing the Id to long isn't going to change the underlying APIs that use this Id, which will still expect integers. It's unclear if this is somehow a problem with GitHub's issue numbering but I believe just changing things to longs might not be the right thing if the underlying API doesn't support it.

The underlying API spec appears to indicate that it is an int64 to me:

{
  "title": "Issue",
  "description": "Issues are a great way to keep track of tasks, enhancements, and bugs for your projects.",
  "type": "object",
  "properties": {
    "id": {
      "type": "integer",
      "format": "int64"
    },
JimSuplizio commented 2 months ago
{
  "title": "Issue",
  "description": "Issues are a great way to keep track of tasks, enhancements, and bugs for your projects.",
  "type": "object",
  "properties": {
    "id": {
      "type": "integer",
      "format": "int64"
    },

Maybe? I'm looking at the published docs for the latest API version which say integer which could totally be wrong. Even then, the couple of PRs out there don't fix the problem. Just changing the Id to the integer in the Issue isn't enough as there are a number of other classes in Octokit which would require being changed to longs. Like the IIssuesClient which calls the update API that is documented to be an int.

brantburnett commented 2 months ago

Maybe? I'm looking at the published docs for the latest API version which say integer which could totally be wrong. Even then, the couple of PRs out there don't fix the problem. Just changing the Id to the integer in the Issue isn't enough as there are a number of other classes in Octokit which would require being changed to longs. Like the IIssuesClient which calls the update API that is documented to be an int.

I pulled the JSON I linked before from those published docs. integer in an OpenAPI spec just means its integral, it doesn't imply a size like it does in C#. Thus the format which indicates size, which is listed as int64.

As to other spots in Octokit, yeah it makes sense that there may be other areas impacted. But I will say that for my specific problem this would fix it as we don't call update issue APIs. Though I understand if the desire is to do a single more complete fix.

JimSuplizio commented 2 months ago

@brantburnett if the underlying API for integer is int64 then I stand corrected as that would be a non-issue. It doesn't change the fact that just changing Issue's ID would be enough. I am using these classes for both deserialization and modifying issues, it can't be a partial fix just for deserialization, that int->long would need to be made to every class that's using an int Issue number.

It's also unclear if this is actually an issue with Octokit which needs to be fixed or if this is an issue with GitHub, itself, that needs to be fixed on their end. Being that this started happening about 24 hours ago, it seems like someone made a change to allow long Issue IDs and broke this and the fix might end up being there. shrug At this point, who can say?

Leh2 commented 2 months ago

@JimSuplizio, the core issue is that Octokit uses int for Id, where the maximum is 2,147,483,647, and the GitHub issue ID has just surpassed that number, causing deserialization to fail. This will also become a problem for comments and other elements, although comments still have about 163 million comments to go before it becomes an issue 😬

kfcampbell commented 2 months ago

I've done some research internally and both the API and underlying data store are set up to handle types bigger than a 32-bit integer, so this bug should go away as soon as we handle the behavior more gracefully on the Octokit side.

dougbu commented 2 months ago

Waiting for a hotfix here 👀

now that #2060 and #2092 are merged and (ignore, those were historical references) #2890 is approved, is the confidence high that everything is fixed❔ if yes, how soon should that hotfix be available❔

jeffhandley commented 2 months ago

Does Octokit.GraphQL need a corresponding change here too? https://github.com/octokit/octokit.graphql.net/issues/311

danmoseley commented 2 months ago

@JimSuplizio, the core issue is that Octokit uses int for Id, where the maximum is 2,147,483,647, and the GitHub issue ID has just surpassed that number, causing deserialization to fail. This will also become a problem for comments and other elements, although comments still have about 163 million comments to go before it becomes an issue 😬

@kfcampbell does this need a fix also, while we're at it? "long all the things"

tibitoth commented 2 months ago

IssueComment Id should be long as well, because we are at 1.900.000.000