Skip to content
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

Value was either too large or too small for an Int32 #598

Closed
Jericho opened this issue Jun 20, 2024 · 6 comments · Fixed by #599
Closed

Value was either too large or too small for an Int32 #598

Jericho opened this issue Jun 20, 2024 · 6 comments · Fixed by #599
Assignees
Milestone

Comments

@Jericho
Copy link
Contributor

Jericho commented Jun 20, 2024

GRM references Octokit 10.0.0 which has known bugs regarding int vs long. We should upgrade to Octokit 12.0.0 which resolves some of these bugs (such as octokit/octokit.net#2928 for instance).

BTW: my guess is that more of these int vs long issues will be discovered and solved in the near future so I wouldn't be surprised if we had to upgrade again in the future.

Here's an example demonstrating that my build is failing due to this issue:
https://ci.appveyor.com/project/Jericho/stronggrid/builds/50054134/job/k3i5irek8a2bsa33#L487

@Jericho
Copy link
Contributor Author

Jericho commented Jun 20, 2024

Upgrading Octokit is a good first step but it's not sufficient to resolve this issue. I was able to reproduce this problem even after upgrading the reference. Here's a more detailed stack trace:

 ---> System.OverflowException: Value was either too large or too small for an Int32.
   at System.Convert.ThrowInt32OverflowException()
   at System.Convert.ToInt32(Int64 value)
   at lambda_method117(Closure , Object , IEnumerable`1 , ResolutionContext )
   --- End of inner exception stack trace ---
   at lambda_method117(Closure , Object , IEnumerable`1 , ResolutionContext )
   --- End of inner exception stack trace ---
   at lambda_method117(Closure , Object , IEnumerable`1 , ResolutionContext )
   at GitReleaseManager.Core.Provider.GitHubProvider.<>c__DisplayClass14_0.<<GetIssueCommentsAsync>b__0>d.MoveNext()
--- End of stack trace from previous location ---
   at GitReleaseManager.Core.Provider.GitHubProvider.ExecuteAsync[T](Func`1 action)
   --- End of inner exception stack trace ---
   at GitReleaseManager.Core.Provider.GitHubProvider.ExecuteAsync[T](Func`1 action)
   at GitReleaseManager.Core.VcsService.CommentsIncludeStringAsync(String owner, String repository, Issue issue, String comment)
   at GitReleaseManager.Core.VcsService.AddIssueCommentsAsync(String owner, String repository, Milestone milestone)
   at GitReleaseManager.Core.VcsService.CloseMilestoneAsync(String owner, String repository, String milestoneTitle)
   at GitReleaseManager.Core.Commands.CloseCommand.ExecuteAsync(CloseSubOptions options)
   at GitReleaseManager.Cli.Program.Main(String[] args)

I notice that stack trace mentions GetIssueCommentsAsync which, as its name implies, retrieves comments for a given issue. So I investigated all code and classes related to this functinoality and noticed the model for a comment is defined in GRM with the following property:

public class IssueComment
{
    /// <summary>
    /// Gets or sets the issue comment Id.
    /// </summary>
    public int Id { get; set; }

Probably should change the type of the Id property to match the type of the data returned from the GitHub API.

I scanned all the model classes and noticed a few more properties currently defined as int which should probably be changed to long:

  • Milestone.InternalNumber
  • Release.Id
  • ReleaseAsset.Id

@Jericho
Copy link
Contributor Author

Jericho commented Jun 20, 2024

After further investigation it appears that Octokit still defines Release.Id and ReleaseAsset.Id as int therefore we shouldn't change these two properties in our model classes.

@Jericho
Copy link
Contributor Author

Jericho commented Jun 20, 2024

I can confirm that after upgrading the Octokit reference and changing IssueComment.Id to long, my build conpletes successfully

========================================
Publish-GitHub-Release
========================================

   ____ _ _   ____      _                     __  __
  / ___(_) |_|  _ \ ___| | ___  __ _ ___  ___|  \/  | __ _ _ __   __ _  __ _  ___ _ __
 | |  _| | __| |_) / _ \ |/ _ \/ _` / __|/ _ \ |\/| |/ _` | '_ \ / _` |/ _` |/ _ \ '__|
 | |_| | | |_|  _ <  __/ |  __/ (_| \__ \  __/ |  | | (_| | | | | (_| | (_| |  __/ |
  \____|_|\__|_| \_\___|_|\___|\__,_|___/\___|_|  |_|\__,_|_| |_|\__,_|\__, |\___|_|
                                                                       |___/
                                                               0.17.0-collaborators0001

[VRB] Loading configuration from file: D:\_build\StrongGrid\GitReleaseManager.yaml
[VRB] Successfully deserialized configuration!
Using GitHub as VCS Provider
Closing milestone 0.109.0
[VRB] Finding open milestone with title '0.109.0' on 'Jericho/StrongGrid'
[VRB] Closing milestone '0.109.0' on 'Jericho/StrongGrid'
[VRB] Finding issues with milestone: '127
[VRB] Finding issue comment created by GitReleaseManager for issue #527
Issue #527 already contains release comment, skipping...
[VRB] Finding issue comment created by GitReleaseManager for issue #526
Adding release comment for Issue #526
[VRB] Finding issue comment created by GitReleaseManager for issue #525
Adding release comment for Issue #525
[VRB] Finding issue comment created by GitReleaseManager for issue #524
Adding release comment for Issue #524
[VRB] Finding issue comment created by GitReleaseManager for issue #523
Adding release comment for Issue #523

----------------------------------------
TearDown
----------------------------------------
Finished running tasks.

Task                          Duration
--------------------------------------------------
Setup                         00:00:00.4135320
Publish-GitHub-Release        00:00:03.6450926
Teardown                      00:00:00.0009156
--------------------------------------------------
Total:                        00:00:04.0595402

@Jericho
Copy link
Contributor Author

Jericho commented Jun 20, 2024

Submitted a PR with necessary changes. I did not change Milestone.InternalNumber to long since it's not needed to resolve this issue but I'll be happy to amend my PR if the consensus is that we should proactively change it as well.

@gep13
Copy link
Member

gep13 commented Jul 9, 2024

@Jericho thank you for working on this! Appreciate the help (again)!

I have marked this issue as a breaking change, as the required change will need to modify some of the internals.

Rather than stray too far from the underlying dependencies, I think we should wait before making any additional changes. Reading the issue that you linked to, it would appear that a new Octokit library is in the works, that is generated from the underlying API, so issues like this shouldn't happen going further, so perhaps better to wait until that is ready, and then switch over to using that.

gep13 pushed a commit to Jericho/GitReleaseManager that referenced this issue Jul 9, 2024
To addresses issues with int overflows. This was fixed in Octokit by
changing int to long, in some places.

In addition, changes IssueComment.Id to long to match the underlying
type.
@gep13 gep13 closed this as completed in #599 Jul 9, 2024
gep13 added a commit that referenced this issue Jul 10, 2024
* release/0.18.0:
  (#580) Mark the integration test as "inconclusive"
  (test) Remove usage of Assert.AreEqual
  Bump NUnit from 3.14.0 to 4.1.0 in /src
  (test) Introduce NUnit.Analyzers
  Bump NGitLab from 6.39.0 to 6.51.1 in /src
  Bump Serilog.Sinks.Console and Serilog in /src
  Bump Microsoft.Extensions.DependencyInjection in /src
  Bump Microsoft.SourceLink.GitHub from 1.1.1 to 8.0.0 in /src
  Bump ApprovalTests from 5.9.0 to 6.0.0 in /src
  Bump Scriban from 5.9.0 to 5.10.0 in /src
  Bump Destructurama.Attributed and Serilog in /src
  Bump Microsoft.CodeAnalysis.NetAnalyzers from 7.0.4 to 8.0.0 in /src
  Bump Roslynator.Analyzers from 4.6.2 to 4.12.4 in /src
  (build) Skip running Codecov
  (#598) Upgrade Octokit reference
gittools-bot pushed a commit that referenced this issue Jul 10, 2024
Merge branch 'release/0.18.0'

* release/0.18.0:
  (#580) Mark the integration test as "inconclusive"
  (test) Remove usage of Assert.AreEqual
  Bump NUnit from 3.14.0 to 4.1.0 in /src
  (test) Introduce NUnit.Analyzers
  Bump NGitLab from 6.39.0 to 6.51.1 in /src
  Bump Serilog.Sinks.Console and Serilog in /src
  Bump Microsoft.Extensions.DependencyInjection in /src
  Bump Microsoft.SourceLink.GitHub from 1.1.1 to 8.0.0 in /src
  Bump ApprovalTests from 5.9.0 to 6.0.0 in /src
  Bump Scriban from 5.9.0 to 5.10.0 in /src
  Bump Destructurama.Attributed and Serilog in /src
  Bump Microsoft.CodeAnalysis.NetAnalyzers from 7.0.4 to 8.0.0 in /src
  Bump Roslynator.Analyzers from 4.6.2 to 4.12.4 in /src
  (build) Skip running Codecov
  (#598) Upgrade Octokit reference
@gittools-bot
Copy link
Contributor

🎉 This issue has been resolved in version 0.18.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

gittools-bot pushed a commit that referenced this issue Jul 10, 2024
Merge branch 'release/0.18.0'

* release/0.18.0:
  (#580) Mark the integration test as "inconclusive"
  (test) Remove usage of Assert.AreEqual
  Bump NUnit from 3.14.0 to 4.1.0 in /src
  (test) Introduce NUnit.Analyzers
  Bump NGitLab from 6.39.0 to 6.51.1 in /src
  Bump Serilog.Sinks.Console and Serilog in /src
  Bump Microsoft.Extensions.DependencyInjection in /src
  Bump Microsoft.SourceLink.GitHub from 1.1.1 to 8.0.0 in /src
  Bump ApprovalTests from 5.9.0 to 6.0.0 in /src
  Bump Scriban from 5.9.0 to 5.10.0 in /src
  Bump Destructurama.Attributed and Serilog in /src
  Bump Microsoft.CodeAnalysis.NetAnalyzers from 7.0.4 to 8.0.0 in /src
  Bump Roslynator.Analyzers from 4.6.2 to 4.12.4 in /src
  (build) Skip running Codecov
  (#598) Upgrade Octokit reference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants