[MAINT]: Is Octokit.GraphQL affected by the Int32 overflow issue?

jeffhandley commented 7 months ago

Describe the need

https://github.com/octokit/octokit.net/issues/2889 is being tracked as needing an urgent hot-fix as the ecosystem of applications using Octokit have all stopped working due to an Int32 overflow.

Does Octokit.GraphQL need an urgent hot-fix to make a compensating change too?

SDK Version

No response

API Version

No response

Relevant log output

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

github-actions[bot] commented 7 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 labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

ericstj commented 7 months ago

https://github.com/octokit/octokit.graphql.net/blob/e6b1e027715bb173631e8833604889cd023aadc8/Octokit.GraphQL.Core/ID.cs#L10

AFAICT it's treated as a string and never parsed to an int. https://github.com/octokit/octokit.graphql.net/blob/e6b1e027715bb173631e8833604889cd023aadc8/Octokit.GraphQL.Core/Internal/IDConverter.cs#L22

ericstj commented 7 months ago

I see - @jeffhandley is referring to this property which I agree looks suspicious: https://github.com/octokit/octokit.graphql.net/blob/e6b1e027715bb173631e8833604889cd023aadc8/Octokit.GraphQL/Model/Issue.cs#L97-L100

kfcampbell commented 7 months ago

That does look suspicious! I've attempted to create a reproductive case here but I'm unfamiliar with this package and receiving ambiguous errors so I'm hoping someone can double-check my work.

My request:

using Octokit.GraphQL;
using Octokit.GraphQL.Model;
using static Octokit.GraphQL.Variable;

var token = System.Environment.GetEnvironmentVariable("GITHUB_TOKEN");

var productInformation = new ProductHeaderValue("int_overflow_bug_repro", "v0.0.1");
var connection = new Connection(productInformation, token);
var owner = "octokit";
var name = "octokit.graphql.net";

var query = new Query()
    .Repository(owner: owner, name: name)
    .Issues(first: 100, states: new[] { IssueState.Open })
    .AllPages()
    .Select(issue => new
    {
        issue.Number,
        issue.Title,
        issue.Body,
        issue.State,
        issue.CreatedAt,
        issue.DatabaseId
    });

var result = await connection.Run(query);

Error:

 sh$ dotnet run
Unhandled exception. Octokit.GraphQL.Core.Deserializers.ResponseDeserializerException: There can be only one argument named "first"
   at Octokit.GraphQL.Core.Deserializers.ResponseDeserializer.Deserialize(String data)
   at Octokit.GraphQL.Core.PagedQuery`1.Runner.RunPage(CancellationToken cancellationToken)
   at Octokit.GraphQL.ConnectionExtensions.Run[T](IConnection connection, ICompiledQuery`1 query, Dictionary`2 variables, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in /home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs:line 26
   at Program.<Main>(String[] args)

If I remove the .AllPages(), I seem to get:

 sh$ dotnet run
/home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs(18,9): error CS1061: 'IssueConnection' does not contain a definition for 'Number' and no accessible extension method 'Number' accepting a first argument of type 'IssueConnection' could be found (are you missing a using directive or an assembly reference?) [/home/kfcampbell/github/dev/octokit.graphql.net/temp/temp.csproj]
/home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs(19,9): error CS1061: 'IssueConnection' does not contain a definition for 'Title' and no accessible extension method 'Title' accepting a first argument of type 'IssueConnection' could be found (are you missing a using directive or an assembly reference?) [/home/kfcampbell/github/dev/octokit.graphql.net/temp/temp.csproj]
/home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs(20,9): error CS1061: 'IssueConnection' does not contain a definition for 'Body' and no accessible extension method 'Body' accepting a first argument of type 'IssueConnection' could be found (are you missing a using directive or an assembly reference?) [/home/kfcampbell/github/dev/octokit.graphql.net/temp/temp.csproj]
/home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs(21,9): error CS1061: 'IssueConnection' does not contain a definition for 'State' and no accessible extension method 'State' accepting a first argument of type 'IssueConnection' could be found (are you missing a using directive or an assembly reference?) [/home/kfcampbell/github/dev/octokit.graphql.net/temp/temp.csproj]
/home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs(22,9): error CS1061: 'IssueConnection' does not contain a definition for 'CreatedAt' and no accessible extension method 'CreatedAt' accepting a first argument of type 'IssueConnection' could be found (are you missing a using directive or an assembly reference?) [/home/kfcampbell/github/dev/octokit.graphql.net/temp/temp.csproj]
/home/kfcampbell/github/dev/octokit.graphql.net/temp/Program.cs(23,9): error CS1061: 'IssueConnection' does not contain a definition for 'DatabaseId' and no accessible extension method 'DatabaseId' accepting a first argument of type 'IssueConnection' could be found (are you missing a using directive or an assembly reference?) [/home/kfcampbell/github/dev/octokit.graphql.net/temp/temp.csproj]

The build failed. Fix the build errors and run again.

which doesn't make any sense to me, so I'm thoroughly confused. Hopefully somebody can help me fine-tune this repro case so we can be sure we need to make a change here!

ericstj commented 7 months ago

I was able to confirm that this repros with GraphQL. Reducing my sample and will share shortly.

Here's the callstack that throws:

>   System.Private.CoreLib.dll!System.Convert.ThrowInt32OverflowException() Line 303    C#
    System.Private.CoreLib.dll!System.Convert.ToInt32(long value) Line 1038 C#
    System.Private.CoreLib.dll!long.System.IConvertible.ToInt32(System.IFormatProvider provider) Line 223   C#
    Newtonsoft.Json.dll!Newtonsoft.Json.Linq.JToken.explicit operator int?(Newtonsoft.Json.Linq.JToken value) Line 835  C#
    Newtonsoft.Json.dll!Newtonsoft.Json.Linq.JToken.ToObject(System.Type objectType) Line 1554  C#
    Newtonsoft.Json.dll!Newtonsoft.Json.Linq.JToken.ToObject<int?>() Line 1499  C#
    [Lightweight Function]  
    System.Linq.dll!System.Linq.Enumerable.SelectIListIterator<Newtonsoft.Json.Linq.JToken, <>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>.Fill(System.Collections.Generic.IList<Newtonsoft.Json.Linq.JToken> source, System.Span<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>> results, System.Func<Newtonsoft.Json.Linq.JToken, <>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>> func) Line 434    C#
    System.Linq.dll!System.Linq.Enumerable.SelectIListIterator<Newtonsoft.Json.Linq.JToken, <>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>.ToList() Line 427    C#
    [Lightweight Function]  
    Octokit.GraphQL.Core.dll!Octokit.GraphQL.Core.Builders.Rewritten.Value.Select<<>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>>(Newtonsoft.Json.Linq.JToken source, System.Func<Newtonsoft.Json.Linq.JToken, <>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>> selector) Line 44    C#
    [Lightweight Function]  
    Octokit.GraphQL.Core.dll!Octokit.GraphQL.Core.Deserializers.ResponseDeserializer.Deserialize<<>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>>(System.Func<Newtonsoft.Json.Linq.JObject, <>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>> deserialize, Newtonsoft.Json.Linq.JObject data) Line 44  C#
    Octokit.GraphQL.Core.dll!Octokit.GraphQL.Core.Deserializers.ResponseDeserializer.Deserialize<<>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>>(System.Func<Newtonsoft.Json.Linq.JObject, <>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>> deserialize, string data) Line 35    C#
    Octokit.GraphQL.Core.dll!Octokit.GraphQL.Core.SimpleQuery<<>f__AnonymousType0<string, bool, int, System.Collections.Generic.List<<>f__AnonymousType1<int, string, string, string, System.DateTimeOffset, int?>>>>.Runner.RunPage(System.Threading.CancellationToken cancellationToken) Line 41  C#

ericstj commented 7 months ago

Here's my repro - not exactly minimal but it might help you fix the problem with yours.

https://gist.github.com/ericstj/b3ff24b68f389c68acbc68299a76c800

I think the result of Issues call is a connection - so access the Nodes member.

ericstj commented 7 months ago

Here's a minimal repro:

using Octokit.GraphQL;
using Octokit.GraphQL.Model;

var token = System.Environment.GetEnvironmentVariable("GITHUB_TOKEN");

var productInformation = new ProductHeaderValue("int_overflow_bug_repro", "v0.0.1");
var connection = new Connection(productInformation, token);
var owner = "octokit";
var name = "octokit.graphql.net";

var query = new Query()
    .Repository(owner: owner, name: name)
    .Issues(states: new[] { IssueState.Open })
    .AllPages()
    .Select(issue => new
    {
        Id = issue.DatabaseId
    });

var result = await connection.Run(query);

Console.WriteLine(result.Count());

This fails with

Unhandled exception. System.OverflowException: Value was either too large or too small for an Int32.
   at System.Convert.ThrowInt32OverflowException()
   at System.Convert.ToInt32(Int64 value)
   at Newtonsoft.Json.Linq.JToken.op_Explicit(JToken value)
   at Newtonsoft.Json.Linq.JToken.ToObject(Type objectType)
   at issue => new <>f__AnonymousType0`1(Id = issue.DatabaseId)(Closure, JToken)
   at System.Linq.Enumerable.SelectIListIterator`2.Fill(IList`1 source, Span`1 results, Func`2 func)
   at System.Linq.Enumerable.SelectIListIterator`2.ToList()
   at lambda_method6(Closure, JObject)
   at Octokit.GraphQL.Core.Deserializers.ResponseDeserializer.Deserialize[TResult](Func`2 deserialize, JObject data)
   at Octokit.GraphQL.Core.PagedQuery`1.Runner.RunPage(CancellationToken cancellationToken)
   at Octokit.GraphQL.ConnectionExtensions.Run[T](IConnection connection, ICompiledQuery`1 query, Dictionary`2 variables, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in C:\scratch\octokitgraphql\Program.cs:line 21
   at Program.<Main>(String[] args)

Though this one seems less severe due to the way that graphQL executes this query. It will only bind members that are part of the select expression, so if I remove the reference to DatabaseId it's successful. This seems much less likely to cause folks an issue since I doubt many folks are using that member.

kfcampbell commented 7 months ago

Thank you for the help reproducing this! I've created https://github.com/octokit/octokit.graphql.net/pull/312 and tested that it fixes the issue.

kfcampbell commented 7 months ago

I've ended up tying myself in knots on this a little bit. First, manually changing the data type of the model causes request deserialization to succeed, though this isn't a permanent solution since those models are generated off of the spec given by https://api.github.com/graphql and periodically regenerated.

Next, I attempted to alter parsing of the spec to account for long values, such that the manual change might be persisted. However, this approach was misinformed, since long isn't one of the GraphQL scalar types, and thus never appears in the spec.

Finally, I've landed on the approach of running a post-processing step for each .cs file that contains a public int? DatabaseId { get; } and replacing it with a long: see this commit.

andrewlock commented 7 months ago

Hi @kfcampbell - thanks for getting that fix in asap! Do you have a timeline for releasing a hotfix? This is currently blocking our release pipeline which is becoming urgent. Thanks!

octokit / octokit.graphql.net