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

[jeffhandley]

Describe the need

octokit/octokit.net#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

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

[github-actions]

👋 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]

I noticed a similar type in the model:

octokit.graphql.net/Octokit.GraphQL/Model/Issue.cs

Line 118 in e6b1e02

public ID Id { get; }

octokit.graphql.net/Octokit.GraphQL.Core/ID.cs

Line 10 in e6b1e02

[JsonConverter(typeof(IDConverter))]

AFAICT it's treated as a string and never parsed to an int.

octokit.graphql.net/Octokit.GraphQL.Core/Internal/IDConverter.cs

Line 22 in e6b1e02

return new ID(serializer.Deserialize<string>(reader));

[ericstj]

I see - @jeffhandley is referring to this property which I agree looks suspicious:

octokit.graphql.net/Octokit.GraphQL/Model/Issue.cs

Lines 97 to 100 in e6b1e02

	/// <summary>
	/// Identifies the primary key from the database.
	/// </summary>
	public int? DatabaseId { get; }

[kfcampbell]

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]

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]

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]

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]

Thank you for the help reproducing this! I've created #312 and tested that it fixes the issue.

[kfcampbell]

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]

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!