This change has been marked as completed.

Describe the completed change (optional):

Pushed to release-5.0.x (21e2147)

Summary

Fix manual Status Update runs with multiple candidate integration configs.

Review Request #12835 — Created Feb. 21, 2023 and submitted 2 years, 4 months ago — Latest diff uploaded 2 years, 5 months ago

Information

Owner

chipx86*

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description*

When performing a manual run of a StatusUpdate, the code responding to
the manual run signal had no choice but to try to find a matching
IntegrationConfig for the review request. If multiple matches were
found, it had to pick one, and this could easily be the wrong one.

To solve this, we now store the configuration ID in
StatusUpdate.extra_data, using a private key. This is stored either
when setting the new StatusUpdate.integration_config property, or when
creating the StatusUpdate using
StatusUpdate.objects.create_for_integration() (which is the preferred
way going forward).

When either setting or fetching the configuration, the configuration is
validated to make sure it matches the same LocalSite, to avoid any
cross-site contamination issues.

The status_update_request_run signal is now given a config argument
mapping to this stored configuration (if one is stored), which
integrations can use. This will require a change for integrations
handling that signal to prioritize the new argument.

Old Status Updates in manual run mode will result in a None
configuration, which will need to be handled through legacy means, but
all future Status Updates created after this change will have a suitable
configuration.

Testing Done

All unit tests pass.

Tested this along with changes in rbintegrations to make use of the
new config state.

This will be tested in production with an affected customer.

Diff Revision 2 (Latest)

orig

Commits

First	Last	Summary	ID	Author
		Fix manual Status Update runs with multiple candidate integration configs. When performing a manual run of a `StatusUpdate`, the code responding to the manual run signal had no choice but to try to find a matching `IntegrationConfig` for the review request. If multiple matches were found, it had to pick one, and this could easily be the wrong one. To solve this, we now store the configuration ID in `StatusUpdate.extra_data`, using a private key. This is stored either when setting the new `StatusUpdate.integration_config` property, or when creating the `StatusUpdate` using `StatusUpdate.objects.create_for_integration()` (which is the preferred way going forward). When either setting or fetching the configuration, the configuration is validated to make sure it matches the same `LocalSite`, to avoid any cross-site contamination issues. The `status_update_request_run` signal is now given a `config` argument mapping to this stored configuration (if one is stored), which integrations can use. This will require a change for integrations handling that signal to prioritize the new argument. Old Status Updates in manual run mode will result in a `None` configuration, which will need to be handled through legacy means, but all future Status Updates created after this change will have a suitable configuration.	0e6fcb14583551c29b9a546448c6c8b678369200	Christian Hammond

Files

	reviewboard/reviews/managers.py
	reviewboard/reviews/signals.py
	reviewboard/reviews/models/status_update.py
	reviewboard/reviews/tests/test_status_update.py
	reviewboard/reviews/tests/test_status_update_manager.py

-    Revision b0bc89dfac9ee6ab49dfe3cd3eb37fdc0a521f3c
+    New Change
     """Managers for reviewboard.reviews.models."""
     from __future__ import annotations
     import logging
     from typing import Dict, Optional, TYPE_CHECKING
     from django.contrib.auth.models import AnonymousUser, User
     from django.core.exceptions import ObjectDoesNotExist
     from django.db import connections, router, transaction, IntegrityError
     from django.db.models import Manager, Q
     from django.db.models.query import QuerySet
     from django.utils.text import slugify
     from djblets.db.managers import ConcurrencyManager
     from reviewboard.deprecation import RemovedInReviewBoard60Warning
     from reviewboard.diffviewer.models import DiffSetHistory
     from reviewboard.scmtools.errors import ChangeNumberInUseError
     from reviewboard.scmtools.models import Repository
     from reviewboard.site.models import LocalSite
     if TYPE_CHECKING:
         from reviewboard.changedescs.models import ChangeDescription
         from reviewboard.integrations.base import Integration
         from reviewboard.integrations.models import IntegrationConfig
         from reviewboard.reviews.models import ReviewRequest, StatusUpdate
     logger = logging.getLogger(__name__)
     class CommentManager(ConcurrencyManager):
             if distinct:
                 queryset = queryset.distinct()
             return queryset
     class StatusUpdateManager(Manager):
         """A manager for StatusUpdate models.
         This offers conveniences around creating
         :py:class:`~reviewboard.reviews.models.status_update.StatusUpdate` models
         for custom integrations.
         Version Added:
 .0.3
         """
         def create_for_integration(
             self,
             integration: Integration,
             *,
             config: IntegrationConfig,
             user: User,
             review_request: ReviewRequest,
             change_description: Optional[ChangeDescription] = None,
             service_id: Optional[str] = None,
             summary: Optional[str] = None,
             description: Optional[str] = None,
             state: Optional[str] = None,
             can_retry: bool = False,
             extra_data: Dict = {},
             starting_description: str = 'starting...',
             waiting_description: str = 'waiting to run.',
             **kwargs,
         ) -> StatusUpdate:
             """Return a new status update for a given integration.
             This helps with generating defaults for a status update, and putting
             it in the correct initial state when running manually.
             The integration configuration will be associated with the status
             update, which is important for manually running integrations when
             multiple integrations are present on a review request.
             Args:
                 integration (reviewboard.integrations.base.Integration):
                     The integration that this status update will be associated
                     with.
                 config (reviewboard.integrations.models.IntegrationConfig):
                     The configuration for the integration, used to provide
                     defaults and used for later manual runs.
                 user (django.contrib.auth.models.User):
                     The user that the status update will be associated with.
                 review_request (reviewboard.reviews.models.review_request.
                                 ReviewRequest):
                     The review request that the status update will be associated
                     with.
                 change_description (reviewboard.changedescs.models.
                                     ChangeDescription, optional):
                     The optional change description that the status update will
                     be associated with.
                 service_id (str, optional):
                     An explicit service ID for the status update.
                     If not provided (or if ``None``), a slugified version of
                     the integration's name will be used.
                 summary (str, optional):
                     An explicit summary for the status update.
                     If not provided (or if ``None``), the integration name will
                     be used.
                 description (str, optional):
                     An explicit description for the status update.
                     If not provided (or if ``None``), a standardized description
                     will be used depending on whether the status update will be
                     created in manual run mode.
                     See ``starting_description`` and ``waiting_description`` to
                     customize these strings.
                 state (str, optional):
                     An explicit state for the status update.
                     If not provided (or if ``None``), the state will be in Not
                     Yet Run if creating in manual run mode, or Pending otherwise.
                 can_retry (bool, optional):
                     Whether the status update can be retried after being run.
                 extra_data (dict, optional):
                     Extra data to store in the status update.
                 starting_description (str, optional):
                     The description to use if creating a status update that is
                     immediately starting.
                 waiting_description (str, optional):
                     The description to use if creating a status update that is
                     in manual run mode.
                 **kwargs (dict, optional):
                     Additional keyword arguments for the model.
             Returns:
                 reviewboard.reviews.status_update.StatusUpdate:
                 The new status update.
             """
             run_manually = config.get('run_manually')
             if description is None:
                 if run_manually:
                     description = waiting_description
                 else:
                     description = starting_description
             if state is None:
                 if run_manually:
                     state = self.model.NOT_YET_RUN
                 else:
                     state = self.model.PENDING
             if not service_id:
                 assert integration.name
                 service_id = slugify(integration.name)
             status_update: StatusUpdate = self.model(
                 user=user,
                 review_request=review_request,
                 change_description=change_description,
                 service_id=service_id,
                 summary=summary or integration.name,
                 description=description,
                 state=state,
                 extra_data=dict(extra_data, **{
                     'can_retry': can_retry,
                 }),
                 **kwargs)
             status_update.integration_config = config
             status_update.save()
             return status_update

reviewboard/reviews/signals.py

reviewboard/reviews/models/status_update.py

reviewboard/reviews/tests/test_status_update.py

reviewboard/reviews/tests/test_status_update_manager.py