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

#2927: comment id model update to long instead of int #2928

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

arxange1
Copy link
Contributor

@arxange1 arxange1 commented Jun 4, 2024

Resolves #2927


Before the change?

  • GetIssueComments() leads to System.OverflowException: Value was either too large or too small for an Int32.

After the change?

  • GetIssueComments() returns comments.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@Bazeloth
Copy link

Bazeloth commented Jun 5, 2024

If i could approve this i would. This is holding us back big time.

Edit:
What the hell i can? I'll patiently wait for an official approval though.

image

@kevinobruno
Copy link

I think we also need to change IIssueCommentsClient and its implementation IssueCommentsClient?

@arxange1
Copy link
Contributor Author

arxange1 commented Jun 5, 2024

I think we also need to change IIssueCommentsClient and its implementation IssueCommentsClient?

Indeed, thank you!

@ioshm
Copy link

ioshm commented Jun 5, 2024

I believe RepositoryCommentsClient & its interface should also be updated?

@arxange1
Copy link
Contributor Author

arxange1 commented Jun 5, 2024

I believe RepositoryCommentsClient & its interface should also be updated?

Please check

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a few comment id/number parameters were not updated to long in the other methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a few comment id/number parameters were not updated to long in the other methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a few comment id/number parameters were not updated to long in the other methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -75,7 +75,7 @@ public interface IPullRequestReviewCommentReactionsClient
/// <param name="commentId">The issue id</param>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is wrong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -85,6 +85,6 @@ public interface IPullRequestReviewCommentReactionsClient
/// <param name="commentId">The issue id</param>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation is wrong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -18,17 +18,17 @@ public interface IRepositoryCommentsClient
/// <remarks>http://developer.github.com/v3/repos/comments/#get-a-single-commit-comment</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The comment id</param>
/// <param name="commentId">The comment id</param>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we want to rename this to commentId. Perhaps the maintainers should chime in on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a breaking change fix anyway. I would like to see the maintainer here to reduce the id/number confusion around the code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a few comment id/number parameters were not updated to long in the other methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number should stay as int. The fix is about comment id only. I believe there is a huge mess around numbers/ids, and I changed it to long in all places where I think it is comment_id actually.

Copy link

@ioshm ioshm Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I am mistaken, number do not exists on comments, and only have ids which should be int64 by now. It seems number and id are used interchangeably when it comes to comments.

Take for example this method

Definition:

/// <summary>
/// Updates a specified Commit Comment.
/// </summary>
/// <remarks>http://developer.github.com/v3/repos/comments/#update-a-commit-comment</remarks>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The comment number</param>
/// <param name="commentUpdate">The modified comment</param>
Task<CommitComment> Update(string owner, string name, int number, string commentUpdate);

Implementation:

/// <summary>
/// Updates a specified Commit Comment.
/// </summary>
/// <param name="owner">The owner of the repository</param>
/// <param name="name">The name of the repository</param>
/// <param name="number">The comment number</param>
/// <param name="commentUpdate">The modified comment</param>
/// <remarks>http://developer.github.com/v3/repos/comments/#update-a-commit-comment</remarks>
[ManualRoute("PATCH", "/repos/{owner}/{repo}/comments/{comment_id}")]
public Task<CommitComment> Update(string owner, string name, int number, string commentUpdate)
{
Ensure.ArgumentNotNullOrEmptyString(owner, nameof(owner));
Ensure.ArgumentNotNullOrEmptyString(name, nameof(name));
Ensure.ArgumentNotNull(commentUpdate, nameof(commentUpdate));
return ApiConnection.Patch<CommitComment>(ApiUrls.CommitComment(owner, name, number), new BodyWrapper(commentUpdate));
}

The number parameter still has the type int when it should be long. The docs states that the comment_id (in this case, this parameter is called number) is of type integer which means any double without a fractional part - long should be good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@nickfloyd nickfloyd added Type: Breaking change Used to note any change that requires a major version bump Type: Bug Something isn't working as documented labels Jun 5, 2024
@nickfloyd
Copy link
Contributor

Hey y'all,

First off, thank you for tackling this and thank you for moving forward on these updates to make things work/better - apologies for the trouble. This was fallout from a recent change. You are correct this issue and ones like it are why we are moving to a fully generated SDK.

It looks like the tests need to be updated as well before we can ✅ and get this merged in and shipped.

To answer the question regarding scope of change right now the only ID which cascades across a number of SDK APIs is comment_id, which in some cases is simply named number as a parameter. Changing the name of this parameter and it's type is the right move here. Especially since this will be releases as a breaking change.

Side note: The newer, generated .NET SDK has already accounted for this. That SDK gets generated nightly against the latest OpenAPI description from the GitHub REST API.

@nickfloyd
Copy link
Contributor

If i could approve this i would. This is holding us back big time.

Edit: What the hell i can? I'll patiently wait for an official approval though.

image

Hey @Bazeloth While you can "approve" - because we want/need community involvement and input the merge criteria would not be met given that requires an approval from a core maintainer/owner. We do take those ☑️ checks seriously as we are going through approval though ❤️ .

@BrianCArnold
Copy link
Contributor

It looks like there's a lot more places where int number should actually be long commentId, I'm going to see if I can patch all of them, looks like a bit of work.

@BrianCArnold
Copy link
Contributor

@arxange1 @nickfloyd

Take a look at https://github.com/BrianCArnold/octokit.net/tree/bugfix/%232927
I fixed what I think are the remainder of all the Int32 commentId and reactionId issues, and all the tests should be passing now.

Just cherry-pick that commit onto this branch if it looks good, I'm trying to get my bot working again.

@Bazeloth
Copy link

Bazeloth commented Jun 6, 2024

If i could approve this i would. This is holding us back big time.
Edit: What the hell i can? I'll patiently wait for an official approval though.
image

Hey @Bazeloth While you can "approve" - because we want/need community involvement and input the merge criteria would not be met given that requires an approval from a core maintainer/owner. We do take those ☑️ checks seriously as we are going through approval though ❤️ .

Thanks that explains a lot. I was just wondering, since in our repo people that can approve usually can merge as well. Good to see you guys took precautions 👍

@BrianCArnold
Copy link
Contributor

... did you copy my changes into your own commit, or just do (virtually) the same thing?

@arxange1
Copy link
Contributor Author

arxange1 commented Jun 6, 2024

... did you copy my changes into your own commit, or just do (virtually) the same thing?

I made the changes independently and didn't see your message before doing so.

@BrianCArnold
Copy link
Contributor

Ahh, gotcha, yeah, I looked through and I see that as well. There's actually several fixes that I made that look like they got missed, let me see if I can make a patch to fix them as well.

@BrianCArnold
Copy link
Contributor

(Also, you fixed several things I missed as well)

@BrianCArnold
Copy link
Contributor

If we're doing a breaking change on this release to avoid confusions on parameter names, such as replacing "number" with "commentId", I'd like to include fixing as many of them as possible.

Should we include them on this PR?

(https://github.com/BrianCArnold/octokit.net/tree/FixParameterNames)

@nickfloyd
Copy link
Contributor

Should we include them on this PR?

Normally I'd be a little more risk adverse, but given the scope of change here (an id field, the no longer matches the backing type + breaking change - I can definitely get behind a little bigger changeset to resolve these issues in one go.

@nickfloyd
Copy link
Contributor

I'd like to land this as soon as possible. It looks like @arxange1 had pulled in a few changes from other sources. I'll give it another day here but after that we'll accept more PRs to address this and at least get these fixes shipped out on Monday unless anyone has any issues where this should be held up.

@arxange1
Copy link
Contributor Author

arxange1 commented Jun 9, 2024

If we're doing a breaking change on this release to avoid confusions on parameter names, such as replacing "number" with "commentId", I'd like to include fixing as many of them as possible.

Should we include them on this PR?

(https://github.com/BrianCArnold/octokit.net/tree/FixParameterNames)

Done

@jeny-amatya-qed
Copy link

Any ETA on the new version with this fix?

@nickfloyd
Copy link
Contributor

Any ETA on the new version with this fix?

Hey @jeny-amatya-qed I am working on a release right now... I'll let you all know when it drops.

@nickfloyd nickfloyd merged commit 6c43183 into octokit:main Jun 10, 2024
5 checks passed
@nickfloyd
Copy link
Contributor

These changes have been released - https://www.nuget.org/packages/Octokit

dotnet add package Octokit --version 12.0.0

@jessehouwing
Copy link

This applies to other models as well I found today. My guess is that any int id should be a long id.

See #2931

Mankarse added a commit to Mankarse/Zero-K-Infrastructure that referenced this pull request Jul 1, 2024
Old Octokit versions used int rather than long for Comment.Id;
which causes overflow error when parsing responses from the GitHub Comments API.
See octokit/octokit.net#2928
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Error deserializing issue comments
10 participants