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

fix: GitHub repos unique constraint and delete #4037

Merged
merged 3 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions api/conftest.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os
import typing
from unittest.mock import MagicMock

import boto3
import pytest
Expand All @@ -11,6 +12,7 @@
from moto import mock_dynamodb
from mypy_boto3_dynamodb.service_resource import DynamoDBServiceResource, Table
from pytest_django.plugin import blocking_manager_key
from pytest_mock import MockerFixture
from rest_framework.authtoken.models import Token
from rest_framework.test import APIClient
from urllib3.connectionpool import HTTPConnectionPool
Expand Down Expand Up @@ -85,6 +87,25 @@ def pytest_sessionstart(session: pytest.Session) -> None:
fix_issue_3869()


@pytest.fixture()
def post_request_mock(mocker: MockerFixture) -> MagicMock:
def mocked_request(*args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

class MockResponse:
def __init__(self, json_data, status_code):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

self.json_data = json_data
self.status_code = status_code

def raise_for_status(self) -> None:
pass

def json(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return self.json_data

return MockResponse(json_data={"data": "data"}, status_code=200)

return mocker.patch("requests.post", side_effect=mocked_request)


@pytest.hookimpl(trylast=True)
def pytest_configure(config: pytest.Config) -> None:
if (
Expand Down Expand Up @@ -966,9 +987,16 @@ def flagsmith_environments_v2_table(dynamodb: DynamoDBServiceResource) -> Table:


@pytest.fixture()
def feature_external_resource(feature: Feature) -> FeatureExternalResource:
def feature_external_resource(
feature: Feature, post_request_mock: MagicMock, mocker: MockerFixture
) -> FeatureExternalResource:
mocker.patch(
"integrations.github.client.generate_token",
return_value="mocked_token",
)

return FeatureExternalResource.objects.create(
url="https://github.com/userexample/example-project-repo/issues/11",
url="https://github.com/repositoryownertest/repositorynametest/issues/11",
type="GITHUB_ISSUE",
feature=feature,
metadata='{"status": "open"}',
Expand All @@ -978,9 +1006,16 @@ def feature_external_resource(feature: Feature) -> FeatureExternalResource:
@pytest.fixture()
def feature_with_value_external_resource(
feature_with_value: Feature,
post_request_mock: MagicMock,
mocker: MockerFixture,
) -> FeatureExternalResource:
mocker.patch(
"integrations.github.client.generate_token",
return_value="mocked_token",
)

return FeatureExternalResource.objects.create(
url="https://github.com/userexample/example-project-repo/issues/11",
url="https://github.com/repositoryownertest/repositorynametest/issues/11",
type="GITHUB_ISSUE",
feature=feature_with_value,
)
Expand Down
11 changes: 6 additions & 5 deletions api/features/feature_external_resources/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@
logger = logging.getLogger(__name__)


class FeatureExternalResource(LifecycleModelMixin, models.Model):
class ResourceType(models.TextChoices):
# GitHub external resource types
GITHUB_ISSUE = "GITHUB_ISSUE", "GitHub Issue"
GITHUB_PR = "GITHUB_PR", "GitHub PR"
class ResourceType(models.TextChoices):
# GitHub external resource types
GITHUB_ISSUE = "GITHUB_ISSUE", "GitHub Issue"
GITHUB_PR = "GITHUB_PR", "GitHub PR"


class FeatureExternalResource(LifecycleModelMixin, models.Model):
url = models.URLField()
type = models.CharField(max_length=20, choices=ResourceType.choices)

Expand Down
16 changes: 16 additions & 0 deletions api/integrations/github/github.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging
import typing
from dataclasses import asdict
from typing import Any

from core.helpers import get_current_site_url
from django.utils.formats import get_format
Expand All @@ -25,6 +26,21 @@
logger = logging.getLogger(__name__)


def handle_installation_deleted(payload: dict[str, typing.Any]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Any has already been imported from typing so this should be like the below

Suggested change
def handle_installation_deleted(payload: dict[str, typing.Any]) -> None:
def handle_installation_deleted(payload: dict[str, Any]) -> None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

installation_id = payload.get("installation", {}).get("id")
try:
GithubConfiguration.objects.get(installation_id=installation_id).delete()
except GithubConfiguration.DoesNotExist:
Copy link
Contributor

Choose a reason for hiding this comment

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

installation_id could be None which may match the queryset if there are installation_id matching records with None in them. I would add an explicit check for installation_id is None and handle that case separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

logger.error(
f"Github Configuration with installation_id {installation_id} does not exist"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
f"Github Configuration with installation_id {installation_id} does not exist"
f"GitHub Configuration with installation_id {installation_id} does not exist"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)


def handle_github_webhook_event(event_type: str, payload: dict[str, Any]) -> None:
if event_type == "installation" and payload.get("action") == "deleted":
handle_installation_deleted(payload)


def generate_body_comment(
name: str,
event_type: str,
Expand Down
29 changes: 29 additions & 0 deletions api/integrations/github/migrations/0003_auto_20240528_0640.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Generated by Django 3.2.25 on 2024-05-28 06:40

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('github', '0002_auto_20240502_1949'),
]

operations = [
migrations.AlterModelOptions(
name='githubconfiguration',
options={'ordering': ('id',)},
),
migrations.AlterModelOptions(
name='githubrepository',
options={'ordering': ('id',)},
),
migrations.RemoveConstraint(
model_name='githubrepository',
name='unique_repository_data',
),
migrations.AddConstraint(
model_name='githubrepository',
constraint=models.UniqueConstraint(condition=models.Q(('deleted_at__isnull', True)), fields=('github_configuration', 'project', 'repository_owner', 'repository_name'), name='unique_repository_data'),
),
]
10 changes: 8 additions & 2 deletions api/integrations/github/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class Meta:
condition=models.Q(deleted_at__isnull=True),
)
]
ordering = ("id",)


class GithubRepository(LifecycleModelMixin, SoftDeleteExportableModel):
Expand All @@ -57,21 +58,26 @@ class Meta:
"repository_name",
],
name="unique_repository_data",
condition=models.Q(deleted_at__isnull=True),
)
]
ordering = ("id",)

@hook(BEFORE_DELETE)
def delete_feature_external_resources(
self,
) -> None:
from features.feature_external_resources.models import (
FeatureExternalResource,
ResourceType,
)

FeatureExternalResource.objects.filter(
feature_id__in=self.project.features.values_list("id", flat=True),
type__in=[
FeatureExternalResource.ResourceType.GITHUB_ISSUE,
FeatureExternalResource.ResourceType.GITHUB_PR,
ResourceType.GITHUB_ISSUE,
ResourceType.GITHUB_PR,
],
# Filter by url containing the repository owner and name
url__contains=f"{self.repository_owner}/{self.repository_name}",
Copy link
Contributor

Choose a reason for hiding this comment

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

The match criteria isn't good enough. If a repository owner is named foo it would also match to examplefoo. you need to bound the region of the url__contains so that you're always matching the right object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using regex instead of contains

).delete()
8 changes: 3 additions & 5 deletions api/integrations/github/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
fetch_search_github_resource,
)
from integrations.github.exceptions import DuplicateGitHubIntegration
from integrations.github.github import handle_github_webhook_event
from integrations.github.helpers import github_webhook_payload_is_valid
from integrations.github.models import GithubConfiguration, GithubRepository
from integrations.github.permissions import HasPermissionToGithubConfiguration
Expand Down Expand Up @@ -249,11 +250,8 @@ def github_webhook(request) -> Response:
payload_body=payload, secret_token=secret, signature_header=signature
):
data = json.loads(payload.decode("utf-8"))
# handle GitHub Webhook "installation" event with action type "deleted"
if github_event == "installation" and data["action"] == "deleted":
GithubConfiguration.objects.filter(
installation_id=data["installation"]["id"]
).delete()
if github_event == "installation":
handle_github_webhook_event(event_type=github_event, payload=data)
return Response({"detail": "Event processed"}, status=200)
else:
return Response({"detail": "Event bypassed"}, status=200)
Expand Down
Loading