[BUG]: Error deserializing issue comments

[brantburnett]

What happened?

I've begun noticing errors deserializing the comment 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. The log was for a comment created webhook request.

Note: This is very similar to bug #2889 fixed in 10.0.0, but on comment IDs rather than issue IDs.

Versions

10.0.0 but bug appears to still be present in the main branch

Relevant log output

JSON integer 2147666618 is too large or small for an Int32. Path 'comment.id', line 1, position 4002.
   at Newtonsoft.Json.JsonTextReader.ParseReadNumber(ReadType readType, Char firstChar, Int32 initialPosition)
   at Newtonsoft.Json.JsonTextReader.ParseNumber(ReadType readType)
   at Newtonsoft.Json.JsonTextReader.ReadNumberValue(ReadType readType)
   at Newtonsoft.Json.JsonTextReader.ReadAsInt32()
   at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.SetPropertyValue(JsonProperty property, JsonConverter propertyConverter, JsonContainerContract containerContract, JsonProperty containerProperty, JsonReader reader, Object target)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.PopulateObject(Object newObject, JsonReader reader, JsonObjectContract contract, JsonProperty member, String id)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(JsonReader reader, Type objectType, JsonContract contract, JsonProperty member, JsonContainerContract containerContract, JsonProperty containerMember, Object existingValue)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
   at CenterEdgeBot.Events.ActivityPayloadParser.ParseActivityPayload(String payloadType, String payloadText) in /jenkins/workspace/DevOps_CenterEdgeBot_main/src/CenterEdgeBot/Events/ActivityPayloadParser.cs:line 43
   at CenterEdgeBot.Events.GitHubWebHookProcessor.ProcessGitHubWebHookPayloadAsync(String payloadType, String payloadText) in /jenkins/workspace/DevOps_CenterEdgeBot_main/src/CenterEdgeBot/Events/GitHubWebHookProcessor.cs:line 43
   at CenterEdgeBot.GitHubLambda.HandleGitHubWebHookAsync(APIGatewayProxyRequest apiGatewayProxyRequest, ILambdaContext lambdaContext) in /jenkins/workspace/DevOps_CenterEdgeBot_main/src/CenterEdgeBot/GitHubLambda.cs:line 71

Code of Conduct

• I agree to follow this project's Code of Conduct

[DavidBoike]

Additionally, all the methods on IIssueCommentsClient use an int for issue number, such as Task<IReadOnlyList<IssueComment>> GetAllForIssue(string owner, string name, int number) so it seems like the int to long transition needs to be plumbed out in quite a few places. EDIT: Oops, my fault, issue number as an int is still fine, at least I don't personally know of a repo with > int.MaxValue issues/PRs. The issue at hand here is for issue ids or maybe comment ids which are not observed until you try to deserialize an API response.

[AlmirJNR]

why don't we change all int to long to avoid this in the future? i don't know if it is possible or if it is, then why things wasn't like this already?

edit:
i just ran into this issue while experimenting with octokit.net

edit2:

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.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 205
   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 205
   at Octokit.PocoJsonSerializerStrategy.DeserializeObject(Object value, Type type) in /_/Octokit/SimpleJson.cs:line 1519
   at Octokit.Internal.SimpleJsonSerializer.GitHubSerializerStrategy.DeserializeObject(Object value, Type type) in /_/Octokit/Http/SimpleJsonSerializer.cs:line 205
   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 44
   at Octokit.Connection.Run[T](IRequest request, CancellationToken cancellationToken, Func`2 preprocessResponseBody) in /_/Octokit/Http/Connection.cs:line 752
   at Octokit.ApiConnection.GetPage[TU](Uri uri, IDictionary`2 parameters, String accepts, ApiOptions options, Func`2 preprocessResponseBody) in /_/Octokit/Http/ApiConnection.cs:line 682
   at Octokit.ApiConnection.<>c__DisplayClass20_0`1.<<GetAll>b__0>d.MoveNext() in /_/Octokit/Http/ApiConnection.cs:line 240
--- End of stack trace from previous location ---
   at Octokit.ApiPagination.GetAllPages[T](Func`1 getFirstPage, Uri uri) in /_/Octokit/Clients/ApiPagination.cs:line 23

i don't know if this helps anyone, but here it is the exceptions that i am getting while trying the following code:
var comments = await client.Issue.Comment.GetAllForIssue(payload.Repository.Id, payload.Number);

client is of type GitHubClient
payload is of type PullRequestEventPayload

[AlmirJNR]

As @DavidBoike said, it seams like the problem (mine at least) is in the property in[Octokit.IssueComment.Id] and every json response that i'm getting is returning values greater than the int.MaxValue

example of the responses that i'm getting List<Octokit.IssueComment>

[{
    "url": "https://redacted/issues/comments/2148124527",
    "html_url": "https://redacted/pull/1#issuecomment-2148124527",
    "issue_url": "https://redacted/issues/1",
    "id": 2148124527,
    "node_id": redacted,
    "user": {
      "login": redacted,
      "id": redacted,
      "node_id": redacted,
      "avatar_url": redacted,
      "gravatar_id": "",
      "url": redacted,
      "html_url": redacted,
      "followers_url": redacted,
      "following_url": redacted,
      "gists_url": redacted,
      "starred_url": redacted,
      "subscriptions_url": redacted,
      "organizations_url": redacted,
      "repos_url": redacted,
      "events_url": redacted,
      "received_events_url": redacted,
      "type": "User",
      "site_admin": false
    },
    "created_at": "2024-06-04T18:12:02Z",
    "updated_at": "2024-06-04T18:12:02Z",
    "author_association": "OWNER",
    "body": "AAAAAAAAAAAAAAAAAAAAA",
    "reactions": {
      "url": redacted,
      "total_count": 0,
      "+1": 0,
      "-1": 0,
      "laugh": 0,
      "hooray": 0,
      "confused": 0,
      "heart": 0,
      "rocket": 0,
      "eyes": 0
    },
    "performed_via_github_app": null
  },
...]

i'm using the v11.0.1

[wolfcomp]

Looking at the response schema for comments on issues, you will see that it lists the type as int64 https://docs.github.com/en/rest/issues/comments?apiVersion=2022-11-28

This is something that should be expressly fixed due to it breaking actions that rely on this library for build processing.

[JimSuplizio]

+1 here, I'm seeing the same thing trying to deserialize.

It's the IssueComment's Id, in Octokit/Models/Response/IssueComment.cs#L30, being an int. The GitHub Actions payload has "id": 2152686870 which is > Int32 max or 2147483647.

This was the same thing that happened with Issues 4 months ago.

@nickfloyd, I'm curious as to how this is getting changed internally without Octokit having an update ready?

[nickfloyd]

@nickfloyd, I'm curious as to how this is getting changed internally without Octokit having an update ready?

Hey @JimSuplizio,

There are a few reasons.

While Octokit.net has served the community well over the years, the static nature of it makes it challenging for the small team and community to make sure it's synchronized with the overwhelming number of changes that are being made on a daily basis to GitHub's OpenAPI descriptions. So as a team, we have had to make decisions to maintain this and build for the future through Generative SDKs.

These issues experienced here are not present in our .NET Generated SDK - which is regenerated nightly. I'd encourage you to give that next generation of SDK a shot and let us know what you think. The generated SDK (GitHub.Octokit.SDK) already has things like client side rate limiting and apps auth with automatic token refresh - two features that are not currently present in Octokit.net.

We'll continue to maintain this SDK but I just wanted to let you know where we are headed.

[JimSuplizio]

@nickfloyd, I'm curious as to how this is getting changed internally without Octokit having an update ready?

Hey @JimSuplizio,

There are a few reasons.

While Octokit.net has served the community well over the years, the static nature of it makes it challenging for the small team and community to make sure it's synchronized with the overwhelming number of changes that are being made on a daily basis to GitHub's OpenAPI descriptions. So as a team, we have had to make decisions to maintain this and build for the future through Generative SDKs.

These issues experienced here are not present in our .NET Generated SDK - which is regenerated nightly. I'd encourage you to give that next generation of SDK a shot and let us know what you think. The generated SDK (GitHub.Octokit.SDK) already has things like client side rate limiting and apps auth with automatic token refresh - two features that are not currently present in Octokit.net.

We'll continue to maintain this SDK but I just wanted to let you know where we are headed.

I've looked at the .NET Generated SDK but I'm going to be delaying anything until the SDK is out of alpha. Mainly because it would require me to effectively re-code/extensively change my GitHub event processor.

[joscol]

This is not fully fixed, I'm still seeing issues with some PRs. I think the problem is the InReplyToId of PullRequestComment ( https://github.com/octokit/octokit.net/blob/main/Octokit/Models/Response/PullRequestReviewComment.cs ) which is still an int.

[nickfloyd]

Hey @joscol looks like you're right, great catch. I'll do another sweep. Apologies for the trouble.

[JimSuplizio]

Hey @joscol looks like you're right, great catch. I'll do another sweep. Apologies for the trouble.

@nickfloyd was the release of 13.0.1 the result of fixing the issue(s) @joscol mentioned above?