-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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]: Int32 overflow for Github object ids #2893
Comments
Upgrading the |
I can still repro this with 11.0.1 for deployment environments. I have one with an ID of octokit.net/Octokit/Models/Response/DeploymentEnvironment.cs Lines 28 to 31 in 615dee9
|
I would do a PR to fix it, but fixing it properly is a lottttttttt of breaking changes. |
I still getting the error on version await client.Issue.Comment.GetAllForIssue("myOrganization", "MyRepository", 195325) or await client.Issue.Comment.Create("myOrganization", "MyRepository", 195325, "Some Comment"); 500 - 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)
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\ |
Also having the issue here with 11.0.1. We're calling And for dotnet/maui#18618 we get the exception But dotnet/maui#22580 and dotnet/maui#20012 work fine. |
Also I'm getting this issue in 11.0.1 Calling
|
bump. 👍🏼 |
I guess this issue is related: #2927 |
Yeah, it's now a general issue with the library. Various resources have gone past The fix would be simple, change all the There'd only be any point doing such changing if the team would accept such a change, as otherwise it'd be a waste of people's effort. |
@nickfloyd Thoughts? |
Hey @martincostello it turns out that we had a lot more of these than first thought due to commit id / comment id being so prolific. I'm working on a release (breaking change) this morning for the others. |
Looks like this also affects the new .NET SDK too: System.Text.Json.JsonException
HResult=0x80131500
Message=The JSON value could not be converted to System.Int32. Path: $ | LineNumber: 0 | BytePositionInLine: 10.
Source=System.Text.Json
StackTrace:
at System.Text.Json.ThrowHelper.ReThrowWithPath(ReadStack& state, Utf8JsonReader& reader, Exception ex)
at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
at Microsoft.Kiota.Serialization.Json.JsonParseNode.GetIntValue()
at GitHub.Models.EnvironmentObject.<GetFieldDeserializers>b__42_3(IParseNode n)
at Microsoft.Kiota.Serialization.Json.JsonParseNode.AssignFieldValues[T](T item)
at Microsoft.Kiota.Serialization.Json.JsonParseNode.GetObjectValue[T](ParsableFactory`1 factory)
at Microsoft.Kiota.Serialization.Json.JsonParseNode.<GetCollectionOfObjectValues>d__19`1.MoveNext()
at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
at GitHub.Repos.Item.Item.Environments.EnvironmentsGetResponse.<GetFieldDeserializers>b__14_0(IParseNode n)
at Microsoft.Kiota.Serialization.Json.JsonParseNode.AssignFieldValues[T](T item)
at Microsoft.Kiota.Serialization.Json.JsonParseNode.GetObjectValue[T](ParsableFactory`1 factory)
at Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter.<SendAsync>d__20`1.MoveNext()
at Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter.<SendAsync>d__20`1.MoveNext()
at GitHub.Repos.Item.Item.Environments.EnvironmentsRequestBuilder.<GetAsync>d__4.MoveNext()
This exception was originally thrown at this call stack:
[External Code]
Inner Exception 1:
FormatException: Either the JSON value is not in a supported format, or is out of bounds for an Int32. |
Unfortunately v12 just released. But I'd 👍🏼 a v13 to get this fixed across the board. |
Hey @martincostello would you mind providing the SDK call path that you were using in the new SDK. We haven't wired up "robust" error messages yet in the generative SDKs yet. I see that you were calling Thanks for catching this / finding a use case in the new SDK as well ❤ |
@nickfloyd No worries. The repo isn't public so I can't link to it directly, but this is the relevant line: var environments = (await github.Repos[owner][name].Environments.GetAsync())!.Environments!; |
Yeah I just verified. It looks like we are missing the format attribute for that OpenAPI definition. I'm going to see if I can get an idea of how many are out of sync with the backing data. I think the move forward here is to:
We could use a hammer and just update all of the id's to longs 😬️ but that has the potential to cause heaps of issues when we could be much more surgical here. I'm going to start tracking down all of this and will come back with a more solid plan/execution. Apologies for all of the trouble everyone. We'll get this cleaned up; thank you for the patience. |
Just a quick follow up. I have found about 68 OpenAPI components that should have an Int64 formatter but do not. This means that there is a possibility that there are 68 endpoints used octokit.net that need their corresponding objects updated from As an aside, @kfcampbell and I are working on a few ways to prevent this from happening in the future. Apologies again for all of the trouble here. |
We have determine that the following resources from the GitHub REST API are at risk of overflow issues. Some have been addressed, we'll need to see if there are other APIs/representations in octokit.net that need to be changed as well.
|
This #2941 should cover the remaining fields that were defined as |
The attached PR doesn't appear to address environments/deployments from its current title or contents. |
Apologies... I should've clarified. Deployments and environments had already been covered (including updating repository Id in those method signatures). The items listed above represent resources where the OpenAPI definitions were not changed to match the backing data types - so "at risk" of being wrong in Octokit.net but needed research to find out. The new PR fixes the existing issues in Octokit.net where the OpenAPI definitions were already changed but had not been updated in octokit.net. The changes for the Generated SDKs (like the environments one above) should self correct once the changes that I made to the OpenAPI definitions ship to the rest_api_descriptions (should be in this changeset) repo and generation runs (every night). Let me know if that matches up with what you were highlighting here. |
Hmm - I'll need to recheck tomorrow. I thought deployments hadn't been fixed yet, maybe I misremembered. But the list in this comment doesn't reference a fix PR: #2893 (comment) |
Yeah, this definitely isn't already fixed in v12.0.0 @nickfloyd. The DTO still uses octokit.net/Octokit/Models/Response/DeploymentEnvironment.cs Lines 17 to 31 in 16cea25
If you look at {
"total_count": 2,
"environments": [
{
"id": 2296494046
},
{
"id": 2296498412
}
]
} |
Also reported here recently |
Thanks for the catch @martincostello & @jessehouwing - not sure how I missed it but I did, thanks for helping to round out the corners on this. I'm going to put the PR back into draft, do another sweep to make sure I catch what you found and possibly others and then open it back up for a review. Apologies for the delay and thanks again for the follow up ❤ |
Just an FYI: I found the flaw in my analysis scripts. I was only looking at migrations - where ids shifted from I was able to find a few more and will be updating those as well:
This will make for a pretty big change set but I feel that's better than introducing more (smaller) major releases for this effort - that feels like it would create extra noise and confusion given things would still be broken until they were all shipped complete. |
The PR should be good to go now... this should hopefully cover all areas above. |
I've got one more round of changes to octokit.net before I close this out. It will handle two thing: |
If this helps you: |
Thanks @0x5bfa! It does. The backing API sometimes uses the strict Id when dealing with the resource itself (these are typically of type int64|long|bigint - like with the issues API for instance, but will use the scoped id named number to refer to a resource that is contained within another resource - as in repo/issues, int32 | int I'm trying to track and parse out which should be which and ensure that their types are accurate and they are named appropriately. Given the one @joscol found It's prompted me to review the entire SDK to ensure it is doing what it should. |
What happened?
Running
Returns
System.OverflowException: 'Value was either too large or too small for an Int32.'
When attempting to parse the Github.id field in the response.
Versions
Octokit.nupkg 10.0.0 (.net)
Relevant log output
Code of Conduct
The text was updated successfully, but these errors were encountered: