Fix panic on edit events in issue_comment handler #379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For an unknown reason, GitHub Enterprise 3.2.7 is not including the 'changes" field in issue_comment payloads for edited comments. This causes the handler to panic when it tried to access the previous body of the comment. This field is still included on GitHub.com, so hopefully it will be fixed in some future release. For now, fix the panic by using the "safe" accessors, but at the cost of not actually auditing the edit correctly. Hard failing if the old body is missing will break a lot of legitimate workflows, particularly our internal changelog generation app. A quick survey of the API suggests you can only look up comment history using the GraphQL API, and it gives you a diff instead of the previous content. I'm not sure if the edit auditing functionality is important enough to justify a fallback if the payload is missing fields. Now that the 'ignore_edited_comments' property exists, users who really care about this should probably use that instead.
asvoboda
approved these changes
Feb 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For an unknown reason, GitHub Enterprise 3.2.7 is not including the
'changes" field in issue_comment payloads for edited comments. This
causes the handler to panic when it tried to access the previous body of
the comment. This field is still included on GitHub.com, so hopefully it
will be fixed in some future release.
For now, fix the panic by using the "safe" accessors, but at the cost of
not actually auditing the edit correctly. Hard failing if the old body
is missing will break a lot of legitimate workflows, particularly our
internal changelog generation app.
A quick survey of the API suggests you can only look up comment history
using the GraphQL API, and it gives you a diff instead of the previous
content. I'm not sure if the edit auditing functionality is important
enough to justify a fallback if the payload is missing fields. Now that
the 'ignore_edited_comments' property exists, users who really care
about this should probably use that instead.