Conversation
Contributor
Author
|
@sentry review |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
Comment on lines
118
to
122
| if ( | ||
| comparison.enriched_pull is None | ||
| or comparison.enriched_pull.provider_pull is None | ||
| ): | ||
| log.debug( |
Contributor
There was a problem hiding this comment.
Potential bug: Bypassing `comparison.enriched_pull.provider_pull` validation with `force_notify=True` can lead to `AttributeError` when accessing `provider_pull` attributes (e.g., `["author"]`, `["head"]`) without null checks, causing notification process crashes.
- Description: When "force_notify=True", the validation for
comparison.enriched_pull.provider_pullbeingNoneis bypassed. This allows code execution to proceed even ifprovider_pullisNone. Later, methods likeget_upgrade_messageinapps/worker/services/notification/notifiers/checks/base.py(e.g., line 59:author_username = comparison.enriched_pull.provider_pull["author"].get("username")) and_create_upgrade_messageinapps/worker/services/notification/notifiers/comment/__init__.py(e.g., line 391) directly accessprovider_pull["author"]orprovider_pull["head"]without null checks. Ifcomparison.enriched_pull.provider_pullisNone, attempting to access["author"]or["head"]will raise anAttributeError: 'NoneType' object has no attribute '__getitem__', leading to an unhandled exception and a crash in the notification process. - Suggested fix: Reinstate the
comparison.enriched_pull.provider_pull is Nonevalidation whenforce_notify=Trueor add explicit null checks forcomparison.enriched_pull.provider_pullbefore accessing its attributes in methods likeget_upgrade_messageand_create_upgrade_message.
severity: 0.9, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+98
to
+102
| for condition in self.notify_conditions: | ||
| condition_result = condition.check_condition( | ||
| notifier=self, comparison=comparison | ||
| ) | ||
| return default_result.merge(side_effect_result) | ||
| if condition_result == False: |
Contributor
There was a problem hiding this comment.
Potential bug: Bypassing `comparison.pull` and `comparison.enriched_pull.provider_pull` validation in the comment notifier with `force_notify=True` can cause `AttributeError` when accessing their attributes, leading to crashes.
- Description: When "force_notify=True", the comment notifier bypasses validation checks like
ComparisonHasPull(ensuringcomparison.pull is not None) andPullRequestInProvider(ensuringcomparison.enriched_pull.provider_pull is not None). This allows execution to reachbuild_message()(e.g., line 427) wherecomparison.pull.is_first_coverage_pullis accessed, orcomparison.enriched_pull.provider_pull(line 428) is accessed directly. Similarly,pull.pullid(line 134) andcomparison.enriched_pull.provider_pull["author"]in_create_upgrade_message()are accessed without null checks. If the bypassed objects areNone, these accesses will raise anAttributeError(e.g.,'NoneType' object has no attribute 'is_first_coverage_pull'), leading to an unhandled exception and a crash in the notification process. - Suggested fix: Reinstate
ComparisonHasPullandPullRequestInProvidervalidations in the comment notifier whenforce_notify=True, or add explicit null checks forcomparison.pullandcomparison.enriched_pull.provider_pullbefore accessing their attributes.
severity: 0.9, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
100
to
225
| if not force_notify: | ||
| if comparison.pull is None or (): | ||
| log.debug( | ||
| "Falling back to commit_status: Not a pull request", | ||
| extra={ | ||
| "notifier": self.name, | ||
| "repoid": comparison.head.commit.repoid, | ||
| "notifier_title": self.title, | ||
| "commit": comparison.head.commit, | ||
| }, | ||
| ) | ||
| return NotificationResult( | ||
| notification_attempted=False, | ||
| notification_successful=None, | ||
| explanation="no_pull_request", | ||
| data_sent=None, | ||
| data_received=None, | ||
| ) | ||
| if ( | ||
| comparison.enriched_pull is None | ||
| or comparison.enriched_pull.provider_pull is None | ||
| ): | ||
| log.debug( | ||
| "Falling back to commit_status: Pull request not in provider", | ||
| extra={ | ||
| "notifier": self.name, | ||
| "repoid": comparison.head.commit.repoid, | ||
| "notifier_title": self.title, | ||
| "commit": comparison.head.commit, | ||
| }, | ||
| ) | ||
| return NotificationResult( | ||
| notification_attempted=False, | ||
| notification_successful=None, | ||
| explanation="pull_request_not_in_provider", | ||
| data_sent=None, | ||
| data_received=None, | ||
| ) | ||
| if comparison.pull and comparison.pull.state != "open": | ||
| log.debug( | ||
| "Falling back to commit_status: Pull request closed", | ||
| extra={ | ||
| "notifier": self.name, | ||
| "repoid": comparison.head.commit.repoid, | ||
| "notifier_title": self.title, | ||
| "commit": comparison.head.commit, | ||
| }, | ||
| ) | ||
| return NotificationResult( | ||
| notification_attempted=False, | ||
| notification_successful=None, | ||
| explanation="pull_request_closed", | ||
| data_sent=None, | ||
| data_received=None, | ||
| ) | ||
| # Check branches config for this status before sending the check | ||
| if not self.can_we_set_this_status(comparison): | ||
| return NotificationResult( | ||
| notification_attempted=False, | ||
| notification_successful=None, | ||
| explanation="not_fit_criteria", | ||
| data_sent=None, | ||
| ) | ||
| if not self.required_builds(comparison): | ||
| return NotificationResult( | ||
| notification_attempted=False, | ||
| notification_successful=None, | ||
| explanation="need_more_builds", | ||
| data_sent=None, | ||
| data_received=None, | ||
| ) | ||
| # Check for existing statuses for this commit. If so, retain | ||
| # statuses and don't do a check as well | ||
| statuses = comparison.get_existing_statuses() | ||
| status_title = self.get_status_external_name() | ||
| if statuses and statuses.get(status_title): | ||
| log.debug( | ||
| "Falling back to commit_status: Status already exists for this commit", | ||
| extra={ | ||
| "notifier": self.name, | ||
| "repoid": comparison.head.commit.repoid, | ||
| "notifier_title": self.title, | ||
| "status_title": status_title, | ||
| "commit": comparison.head.commit, | ||
| }, | ||
| ) | ||
| return NotificationResult( | ||
| notification_attempted=False, | ||
| notification_successful=None, | ||
| explanation="preexisting_commit_status", | ||
| data_sent=None, | ||
| data_received=None, | ||
| ) | ||
|
|
||
| payload = None | ||
| try: | ||
| with nullcontext(): |
Contributor
There was a problem hiding this comment.
Deep nesting of conditional logic makes the code hard to read and maintain. Consider extracting the validation logic into separate methods to improve readability and testability.
Suggested change
| if not force_notify: | |
| if comparison.pull is None or (): | |
| log.debug( | |
| "Falling back to commit_status: Not a pull request", | |
| extra={ | |
| "notifier": self.name, | |
| "repoid": comparison.head.commit.repoid, | |
| "notifier_title": self.title, | |
| "commit": comparison.head.commit, | |
| }, | |
| ) | |
| return NotificationResult( | |
| notification_attempted=False, | |
| notification_successful=None, | |
| explanation="no_pull_request", | |
| data_sent=None, | |
| data_received=None, | |
| ) | |
| if ( | |
| comparison.enriched_pull is None | |
| or comparison.enriched_pull.provider_pull is None | |
| ): | |
| log.debug( | |
| "Falling back to commit_status: Pull request not in provider", | |
| extra={ | |
| "notifier": self.name, | |
| "repoid": comparison.head.commit.repoid, | |
| "notifier_title": self.title, | |
| "commit": comparison.head.commit, | |
| }, | |
| ) | |
| return NotificationResult( | |
| notification_attempted=False, | |
| notification_successful=None, | |
| explanation="pull_request_not_in_provider", | |
| data_sent=None, | |
| data_received=None, | |
| ) | |
| if comparison.pull and comparison.pull.state != "open": | |
| log.debug( | |
| "Falling back to commit_status: Pull request closed", | |
| extra={ | |
| "notifier": self.name, | |
| "repoid": comparison.head.commit.repoid, | |
| "notifier_title": self.title, | |
| "commit": comparison.head.commit, | |
| }, | |
| ) | |
| return NotificationResult( | |
| notification_attempted=False, | |
| notification_successful=None, | |
| explanation="pull_request_closed", | |
| data_sent=None, | |
| data_received=None, | |
| ) | |
| # Check branches config for this status before sending the check | |
| if not self.can_we_set_this_status(comparison): | |
| return NotificationResult( | |
| notification_attempted=False, | |
| notification_successful=None, | |
| explanation="not_fit_criteria", | |
| data_sent=None, | |
| ) | |
| if not self.required_builds(comparison): | |
| return NotificationResult( | |
| notification_attempted=False, | |
| notification_successful=None, | |
| explanation="need_more_builds", | |
| data_sent=None, | |
| data_received=None, | |
| ) | |
| # Check for existing statuses for this commit. If so, retain | |
| # statuses and don't do a check as well | |
| statuses = comparison.get_existing_statuses() | |
| status_title = self.get_status_external_name() | |
| if statuses and statuses.get(status_title): | |
| log.debug( | |
| "Falling back to commit_status: Status already exists for this commit", | |
| extra={ | |
| "notifier": self.name, | |
| "repoid": comparison.head.commit.repoid, | |
| "notifier_title": self.title, | |
| "status_title": status_title, | |
| "commit": comparison.head.commit, | |
| }, | |
| ) | |
| return NotificationResult( | |
| notification_attempted=False, | |
| notification_successful=None, | |
| explanation="preexisting_commit_status", | |
| data_sent=None, | |
| data_received=None, | |
| ) | |
| payload = None | |
| try: | |
| with nullcontext(): | |
| if not force_notify: | |
| validation_result = self._validate_notification_conditions(comparison) | |
| if validation_result: | |
| return validation_result | |
| # Then implement: | |
| def _validate_notification_conditions(self, comparison) -> NotificationResult | None: | |
| # Extract all validation logic here | |
| pass |
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+318
to
+345
| force_notify = getattr(comparison.context, "force_notify", False) | ||
|
|
||
| repository_service = self.repository_service | ||
| title = self.get_status_external_name() | ||
| head_commit_sha = comparison.head.commit.commitid | ||
| head_report = comparison.head.report | ||
| state = payload["state"] | ||
| message = payload["message"] | ||
| url = payload["url"] | ||
| if self.status_already_exists(comparison, title, state, message): | ||
|
|
Contributor
There was a problem hiding this comment.
The logic for handling force_notify in send_notification duplicates the check from maybe_send_notification. This creates a potential inconsistency where the behavior could differ between the two methods.
Suggested change
| force_notify = getattr(comparison.context, "force_notify", False) | |
| repository_service = self.repository_service | |
| title = self.get_status_external_name() | |
| head_commit_sha = comparison.head.commit.commitid | |
| head_report = comparison.head.report | |
| state = payload["state"] | |
| message = payload["message"] | |
| url = payload["url"] | |
| if self.status_already_exists(comparison, title, state, message): | |
| # Remove the duplicate force_notify check here since maybe_send_notification already handles it appropriately |
Did we get this right? 👍 / 👎 to inform future reviews.
Comment on lines
+304
to
+306
| } | ||
| else: | ||
| ci_results = None |
Contributor
There was a problem hiding this comment.
When force_notify=True and CI results cannot be fetched, setting ci_results = None could lead to unexpected behavior downstream. Consider logging this decision more prominently or adding explicit handling for this case.
Suggested change
| } | |
| else: | |
| ci_results = None | |
| else: | |
| log.warning( | |
| "Force notify enabled: proceeding without CI results due to client error", | |
| extra={"repoid": repoid, "commit": commitid} | |
| ) | |
| ci_results = None |
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
- Coverage 94.30% 94.29% -0.01%
==========================================
Files 1247 1247
Lines 45835 45874 +39
Branches 1455 1455
==========================================
+ Hits 43223 43258 +35
- Misses 2307 2311 +4
Partials 305 305
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
joseph-sentry
force-pushed
the
joseph/force-notify
branch
from
07ca5c6 to
e0c23ba
Compare
joseph-sentry
force-pushed
the
joseph/force-notify
branch
from
e0c23ba to
c5854b4
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.