Fix for ThrowInt32OverflowException in Issue.Events.GetAllForRepository #2037

terrance00 · 2019-10-24T16:36:21Z

Fixes #2031

Updated Model with long Id
Fixed tests
Updated IssuesClient and interface
Updated IssuesEventsClient and interface
Double checked that the same tests still failing for the same reasons.

- Updated Model with long Id - Fixed tests - Updated IssuesClient and interface - Updated IssuesEventsClient and interface

codecov · 2019-10-24T16:42:11Z

Codecov Report

Merging #2037 into master will not change coverage.
The diff coverage is 50%.

@@           Coverage Diff           @@
##           master    #2037   +/-   ##
=======================================
  Coverage   70.73%   70.73%           
=======================================
  Files         537      537           
  Lines       14106    14106           
=======================================
  Hits         9978     9978           
  Misses       4128     4128

Impacted Files	Coverage Δ
...t.Reactive/Clients/ObservableIssuesEventsClient.cs	`100% <ø> (ø)`	⬆️
Octokit/Helpers/ApiUrls.cs	`98.62% <ø> (ø)`	⬆️
Octokit/Clients/IssuesClient.cs	`98.88% <ø> (ø)`	⬆️
Octokit/Clients/IssuesEventsClient.cs	`100% <ø> (ø)`	⬆️
Octokit.Reactive/Clients/ObservableIssuesClient.cs	`96.77% <ø> (ø)`	⬆️
Octokit/Models/Response/IssueEvent.cs	`30.76% <50%> (ø)`	⬆️

shiftkey · 2019-10-24T19:00:38Z

Double checked that the same tests still failing for the same reasons.

Which tests was this? And how are they failing?

terrance00 · 2019-10-24T19:46:26Z

Four instances of:

CanPopulateObjectFromSerializedData

Unless I missed something at Dev startup - but directly from master - the four of those fail.

Also there are some integration tests that fail - but I assumed that there are some environment variables I don't have.

shiftkey · 2019-10-24T21:21:23Z

Unless I missed something at Dev startup - but directly from master - the four of those fail.

It's weird because both master and this PR don't seem to have issues with those tests.

These are the usages as I understand it, and I can't spot how the changes relate to your PR:
https://github.com/octokit/octokit.net/search?utf8=%E2%9C%93&q=CanPopulateObjectFromSerializedData&type=

shiftkey · 2019-10-24T21:22:58Z

Also there are some integration tests that fail - but I assumed that there are some environment variables I don't have.

There's some setup required - https://github.com/octokit/octokit.net/blob/master/CONTRIBUTING.md#slow-tests - but I'm not worried about them for this PR.

terrance00 · 2019-10-24T22:39:38Z

Yeah it is those four, they do break for me on master as well - without any changes. Any idea what to try?

I get this:

System.Runtime.Serialization.SerializationException : Unable to find assembly 'Octokit, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null

terrance00 · 2019-10-24T22:42:20Z

Seems to be happening in:

Exception thrown: 'System.Runtime.Serialization.SerializationException' in System.Runtime.Serialization.Formatters.dll
Exception thrown: 'System.Reflection.TargetInvocationException' in System.Private.CoreLib.dll

I wonder if it is dotnet core 3 related...

shiftkey · 2019-10-24T23:01:51Z

@terrance00 please open a new issue with details about it - we run the tests for netcoreapp2.0 and net452 so I'm curious which is failing for you.

terrance00 · 2019-10-28T19:21:41Z

Hey man, sorry for the delay.

I have resigned to running the tests in the command line - and that does not fail for those tests.

Leads me to believe that it must be something IDE related in relation the NO_SERIALIZABLE if clause.

shiftkey

@terrance00 thanks for submitting this PR. I think we can avoid changing the number parameter from an int to a long here as these are for the Issue itself, not the EventInfo number that is being deserialized. This will simplify the diff of changes.

shiftkey · 2019-10-29T11:42:06Z

Octokit.Reactive/Clients/IObservableIssuesClient.cs

@@ -55,7 +55,7 @@ public interface IObservableIssuesClient
        /// <param name="number">The issue number</param>
        [SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Get",
             Justification = "Method makes a network request")]
-        IObservable<Issue> Get(string owner, string name, int number);


The number used in this interface is not the one overflowing. Can we undo the changes in this file to simplify the diff?

shiftkey · 2019-10-29T11:43:37Z

Octokit/Clients/IIssuesClient.cs

@@ -52,7 +52,7 @@ public interface IIssuesClient
        /// <param name="number">The issue number</param>
        [SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Get",
             Justification = "Method makes a network request")]
-        Task<Issue> Get(string owner, string name, int number);
+        Task<Issue> Get(string owner, string name, long number);


Same thing here - this number used in this interface is not the one overflowing. Can we undo the changes in this file to simplify the diff?

shiftkey · 2019-10-29T11:45:23Z

Octokit.Reactive/Clients/IObservableIssuesEventsClient.cs

@@ -20,7 +20,7 @@ public interface IObservableIssuesEventsClient
        /// <param name="owner">The owner of the repository</param>
        /// <param name="name">The name of the repository</param>
        /// <param name="number">The issue number</param>
-        IObservable<EventInfo> GetAllForIssue(string owner, string name, int number);
+        IObservable<EventInfo> GetAllForIssue(string owner, string name, long number);


Same thing for this file - number here is the issue number itself, no the EventInfo number that is being deserialized.

shiftkey · 2019-10-29T11:45:56Z

Octokit/Clients/IIssuesEventsClient.cs

@@ -21,7 +21,7 @@ public interface IIssuesEventsClient
        /// <param name="owner">The owner of the repository</param>
        /// <param name="name">The name of the repository</param>
        /// <param name="number">The issue number</param>
-        Task<IReadOnlyList<EventInfo>> GetAllForIssue(string owner, string name, int number);
+        Task<IReadOnlyList<EventInfo>> GetAllForIssue(string owner, string name, long number);


Same thing for this file - number here is the issue number itself, no the EventInfo number that is being deserialized.

shiftkey · 2020-01-20T01:03:13Z

Thanks for taking a shot at this @terrance00!

I received #2060 which was the minimal fix so this can be closed out, but I hope that you can find time in the future for another contribution.

Fix Issue octokit#2031

08b3a54

- Updated Model with long Id - Fixed tests - Updated IssuesClient and interface - Updated IssuesEventsClient and interface

shiftkey changed the title ~~Fix Issue #2031~~ Fix ThrowInt32OverflowException in Issue.Events.GetAllForRepository Oct 24, 2019

shiftkey changed the title ~~Fix ThrowInt32OverflowException in Issue.Events.GetAllForRepository~~ Fix for ThrowInt32OverflowException in Issue.Events.GetAllForRepository Oct 24, 2019

shiftkey requested changes Oct 29, 2019

View reviewed changes

shiftkey self-assigned this Oct 30, 2019

shiftkey added the feedback given label Nov 18, 2019

shiftkey removed their assignment Nov 18, 2019

shiftkey mentioned this pull request Nov 18, 2019

Retrieving events throws Stack.Overflow exception. #1979

Closed

SteveDesmond-ca mentioned this pull request Jan 8, 2020

Update IssueEvent ID field from int to long #2060

Merged

shiftkey closed this Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for ThrowInt32OverflowException in Issue.Events.GetAllForRepository #2037

Fix for ThrowInt32OverflowException in Issue.Events.GetAllForRepository #2037

terrance00 commented Oct 24, 2019 •

edited by shiftkey

Loading

codecov bot commented Oct 24, 2019

shiftkey commented Oct 24, 2019 •

edited

Loading

terrance00 commented Oct 24, 2019 •

edited

Loading

shiftkey commented Oct 24, 2019

shiftkey commented Oct 24, 2019 •

edited

Loading

terrance00 commented Oct 24, 2019

terrance00 commented Oct 24, 2019

shiftkey commented Oct 24, 2019

terrance00 commented Oct 28, 2019

shiftkey left a comment

shiftkey Oct 29, 2019

shiftkey Oct 29, 2019

shiftkey Oct 29, 2019

shiftkey Oct 29, 2019

shiftkey commented Jan 20, 2020

Fix for ThrowInt32OverflowException in Issue.Events.GetAllForRepository #2037

Fix for ThrowInt32OverflowException in Issue.Events.GetAllForRepository #2037

Conversation

terrance00 commented Oct 24, 2019 • edited by shiftkey Loading

codecov bot commented Oct 24, 2019

Codecov Report

shiftkey commented Oct 24, 2019 • edited Loading

terrance00 commented Oct 24, 2019 • edited Loading

shiftkey commented Oct 24, 2019

shiftkey commented Oct 24, 2019 • edited Loading

terrance00 commented Oct 24, 2019

terrance00 commented Oct 24, 2019

shiftkey commented Oct 24, 2019

terrance00 commented Oct 28, 2019

shiftkey left a comment

Choose a reason for hiding this comment

shiftkey Oct 29, 2019

Choose a reason for hiding this comment

shiftkey Oct 29, 2019

Choose a reason for hiding this comment

shiftkey Oct 29, 2019

Choose a reason for hiding this comment

shiftkey Oct 29, 2019

Choose a reason for hiding this comment

shiftkey commented Jan 20, 2020

terrance00 commented Oct 24, 2019 •

edited by shiftkey

Loading

shiftkey commented Oct 24, 2019 •

edited

Loading

terrance00 commented Oct 24, 2019 •

edited

Loading

shiftkey commented Oct 24, 2019 •

edited

Loading