Refactor Treeherder's branch model

docs/submitting_data.md

@@ -16,8 +16,10 @@ this exists in the GitHub group `mozilla`. (For example, `github.com/mozilla/tre
 
 The following steps are required:
 
-1. Create a PR with the new repository information added to the fixtures file:
-   `treeherder/model/fixtures/repository.json`
+1. Create a PR with the new repository information added to the fixture files:
+   - `treeherder/model/fixtures/repository.json`
+   - `treeherder/model/fixtures/repository_branch.json`
+   - `treeherder/model/fixtures/repository_group.json`
 
 2. Open a bug request to enable the webhook that will trigger pulse messages for
    every push from your repo. Use the following information:

tests/etl/test_push_loader.py

@@ -13,7 +13,7 @@
     PulsePushError,
     PushLoader,
 )
-from treeherder.model.models import Push
+from treeherder.model.models import Push, RepositoryBranch
 
 
 @pytest.fixture
@@ -214,6 +214,27 @@ def test_ingest_hg_push_bad_repo(hg_push):
     assert Push.objects.count() == 0
 
 
+@pytest.mark.django_db
+def test_ingest_hg_push_ignores_wildcard_repo(hg_push, test_repository, mock_hg_push_commits):
+    """Repos with wildcard branches are not matched for Hg pushes"""
+    from treeherder.model.models import Repository
+
+    hg_push["payload"]["repo_url"] = test_repository.url
+    wildcard_repo = Repository.objects.create(
+        name="wildcard-hg",
+        repository_group=test_repository.repository_group,
+        dvcs_type="hg",
+        url=test_repository.url,
+        tc_root_url=test_repository.tc_root_url,
+    )
+    RepositoryBranch.objects.create(repository=wildcard_repo, branch="*")
+    PushLoader().process(
+        hg_push, "exchange/hgpushes/v1", "https://firefox-ci-tc.services.mozilla.com"
+    )
+    assert Push.objects.count() == 1
+    assert Push.objects.first().repository == test_repository
+
+
 @pytest.mark.django_db
 def test_ingest_github_push_bad_repo(github_push):
     """Test graceful handling of an unknown GH repo"""
@@ -232,8 +253,11 @@ def test_ingest_github_push_merge_commit(github_push, test_repository, mock_gith
     test_repository.url = github_push[1]["payload"]["details"]["event.head.repo.url"].replace(
         ".git", ""
     )
-    test_repository.branch = github_push[1]["payload"]["details"]["event.base.repo.branch"]
     test_repository.save()
+    RepositoryBranch.objects.create(
+        repository=test_repository,
+        branch=github_push[1]["payload"]["details"]["event.base.repo.branch"],
+    )
     PushLoader().process(
         github_push[1]["payload"],
         github_push[1]["exchange"],
@@ -255,12 +279,13 @@ def test_ingest_github_push_merge_commit(github_push, test_repository, mock_gith
 def test_ingest_github_push_comma_separated_branches(
     branch, expected_pushes, github_push, test_repository, mock_github_push_compare
 ):
-    """Test a repository accepting pushes for multiple branches"""
+    """Test a repository accepting pushes for multiple explicitly-listed branches"""
     test_repository.url = github_push[0]["payload"]["details"]["event.head.repo.url"].replace(
         ".git", ""
     )
-    test_repository.branch = "master,foo,bar"
     test_repository.save()
+    for b in ["master", "foo", "bar"]:
+        RepositoryBranch.objects.create(repository=test_repository, branch=b)
     github_push[0]["payload"]["details"]["event.base.repo.branch"] = branch
     assert Push.objects.count() == 0
     PushLoader().process(
@@ -271,6 +296,149 @@ def test_ingest_github_push_comma_separated_branches(
     assert Push.objects.count() == expected_pushes
 
 
+@pytest.mark.django_db
+def test_ingest_github_push_wildcard_repo(github_push, test_repository, mock_github_push_compare):
+    """Repo with branch='*' accepts a push on any branch"""
+    test_repository.url = github_push[0]["payload"]["details"]["event.head.repo.url"].replace(
+        ".git", ""
+    )
+    test_repository.save()
+    RepositoryBranch.objects.create(repository=test_repository, branch="*")
+    github_push[0]["payload"]["details"]["event.base.repo.branch"] = "my-feature-branch"
+    assert Push.objects.count() == 0
+    PushLoader().process(
+        github_push[0]["payload"],
+        github_push[0]["exchange"],
+        "https://firefox-ci-tc.services.mozilla.com",
+    )
+    assert Push.objects.count() == 1
+
+
+@pytest.mark.django_db
+def test_ingest_github_push_explicit_beats_wildcard(
+    github_push, test_repository, mock_github_push_compare
+):
+    """Explicit branch match takes precedence over wildcard for the same URL"""
+    from treeherder.model.models import Repository
+
+    url = github_push[0]["payload"]["details"]["event.head.repo.url"].replace(".git", "")
+    branch = github_push[0]["payload"]["details"]["event.base.repo.branch"]
+
+    test_repository.url = url
+    test_repository.save()
+    RepositoryBranch.objects.create(repository=test_repository, branch=branch)
+
+    wildcard_repo = Repository.objects.create(
+        name="wildcard-repo",
+        repository_group=test_repository.repository_group,
+        dvcs_type="git",
+        url=url,
+        tc_root_url=test_repository.tc_root_url,
+    )
+    RepositoryBranch.objects.create(repository=wildcard_repo, branch="*")
+
+    PushLoader().process(
+        github_push[0]["payload"],
+        github_push[0]["exchange"],
+        "https://firefox-ci-tc.services.mozilla.com",
+    )
+    assert Push.objects.count() == 1
+    push = Push.objects.first()
+    assert push.repository == test_repository
+    assert push.repository != wildcard_repo
+
+
+@pytest.mark.django_db
+def test_ingest_github_push_special_char_branch(
+    github_push, test_repository, mock_github_push_compare
+):
+    """Branch names with special chars are handled safely via wildcard branch record"""
+    test_repository.url = github_push[0]["payload"]["details"]["event.head.repo.url"].replace(
+        ".git", ""
+    )
+    test_repository.save()
+    RepositoryBranch.objects.create(repository=test_repository, branch="*")
+    github_push[0]["payload"]["details"]["event.base.repo.branch"] = "release/v1.2+hotfix"
+    PushLoader().process(
+        github_push[0]["payload"],
+        github_push[0]["exchange"],
+        "https://firefox-ci-tc.services.mozilla.com",
+    )
+    assert Push.objects.count() == 1
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "branch, expected_pushes",
+    [
+        ("releases/v1.2", 1),
+        ("master", 0),
+    ],
+)
+def test_ingest_github_push_prefix_wildcard(
+    branch, expected_pushes, github_push, test_repository, mock_github_push_compare
+):
+    """A prefix wildcard like 'releases/*' matches branches under that prefix only"""
+    test_repository.url = github_push[0]["payload"]["details"]["event.head.repo.url"].replace(
+        ".git", ""
+    )
+    test_repository.save()
+    RepositoryBranch.objects.create(repository=test_repository, branch="releases/*")
+    github_push[0]["payload"]["details"]["event.base.repo.branch"] = branch
+    PushLoader().process(
+        github_push[0]["payload"],
+        github_push[0]["exchange"],
+        "https://firefox-ci-tc.services.mozilla.com",
+    )
+    assert Push.objects.count() == expected_pushes
+
+
+@pytest.mark.django_db
+def test_ingest_github_push_ambiguous_wildcards_skipped(
+    github_push, test_repository, mock_github_push_compare
+):
+    """When two wildcard patterns both match, the push is skipped"""
+    from treeherder.model.models import Repository
+
+    url = github_push[0]["payload"]["details"]["event.head.repo.url"].replace(".git", "")
+    test_repository.url = url
+    test_repository.save()
+    RepositoryBranch.objects.create(repository=test_repository, branch="releases/*")
+
+    catchall_repo = Repository.objects.create(
+        name="catchall-repo",
+        repository_group=test_repository.repository_group,
+        dvcs_type="git",
+        url=url,
+        tc_root_url=test_repository.tc_root_url,
+    )
+    RepositoryBranch.objects.create(repository=catchall_repo, branch="*")
+
+    github_push[0]["payload"]["details"]["event.base.repo.branch"] = "releases/v1"
+    PushLoader().process(
+        github_push[0]["payload"],
+        github_push[0]["exchange"],
+        "https://firefox-ci-tc.services.mozilla.com",
+    )
+    assert Push.objects.count() == 0
+
+
+@pytest.mark.django_db
+def test_ingest_github_pull_request_routing(github_pr, test_repository, mock_github_pr_commits):
+    """PR events route to repos with accepts_pull_requests=True and store the real base branch"""
+    test_repository.url = github_pr["details"]["event.base.repo.url"].replace(".git", "")
+    test_repository.accepts_pull_requests = True
+    test_repository.save()
+    assert Push.objects.count() == 0
+    PushLoader().process(
+        github_pr,
+        "exchange/taskcluster-github/v1/pull-request",
+        "https://firefox-ci-tc.services.mozilla.com",
+    )
+    assert Push.objects.count() == 1
+    assert Push.objects.first().branch == github_pr["details"]["event.base.repo.branch"]
+
+
 def test_fetch_push_raises_on_empty_pushes(monkeypatch):
     """Test that a HgPushFetchError is raised when fetch_json returns a dict without 'pushes'"""
     monkeypatch.setattr("treeherder.etl.push_loader.fetch_json", lambda url: {})

tests/model/test_repository_branch.py

@@ -0,0 +1,35 @@
+import pytest
+from django.core.exceptions import ValidationError
+
+from treeherder.model.models import RepositoryBranch
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "branch",
+    [
+        "*",
+        "master",
+        "releases/*",
+        "foo/bar/*",
+    ],
+)
+def test_repository_branch_valid_patterns(branch, test_repository):
+    rb = RepositoryBranch(repository=test_repository, branch=branch)
+    rb.clean()  # should not raise
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "branch",
+    [
+        "foo*bar",
+        "*release",
+        "re*ases",
+        "a*b*",
+    ],
+)
+def test_repository_branch_invalid_patterns(branch, test_repository):
+    rb = RepositoryBranch(repository=test_repository, branch=branch)
+    with pytest.raises(ValidationError):
+        rb.clean()

treeherder/etl/management/commands/ingest.py

@@ -255,7 +255,7 @@ def repo_meta(project):
     split_url = _repo.url.split("/")
     return {
         "url": _repo.url,
-        "branch": _repo.branch,
+        "branch": _repo.branches.exclude(branch__endswith="*").values_list("branch", flat=True).first() or "",
         "owner": split_url[3],
         "repo": split_url[4],
         "tc_root_url": _repo.tc_root_url,

treeherder/etl/push_loader.py

@@ -2,7 +2,7 @@
 
 import environ
 import newrelic.agent
-from django.core.exceptions import ObjectDoesNotExist
+from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist
 
 from treeherder.etl.push import store_push_data
 from treeherder.model.models import Repository
@@ -21,13 +21,15 @@ def process(self, message_body, exchange, root_url):
         try:
             newrelic.agent.add_custom_attribute("url", transformer.repo_url)
             newrelic.agent.add_custom_attribute("branch", transformer.branch)
-            repos = Repository.objects
-            if transformer.branch:
-                repos = repos.filter(branch__regex=f"(^|,){transformer.branch}($|,)")
-            else:
-                repos = repos.filter(branch=None)
-            repo = repos.get(url=transformer.repo_url, active_status="active")
+            repo = transformer.resolve_repo()
             newrelic.agent.add_custom_attribute("repository", repo.name)
+        except MultipleObjectsReturned:
+            logger.error(
+                "Multiple repositories matched url=%s branch=%s; skipping push",
+                transformer.repo_url,
+                transformer.branch,
+            )
+            return
         except ObjectDoesNotExist:
             repo_info = transformer.get_info()
             repo_info.update(
@@ -65,10 +67,14 @@ def __init__(self, message_body):
         self.message_body = message_body
         self.repo_url = self.get_repo()
         self.branch = self.get_branch()
+        self.repos = Repository.objects.filter(url=self.repo_url, active_status="active")
 
     def get_branch(self):
         return self.message_body["details"]["event.base.repo.branch"]
 
+    def resolve_repo(self):
+        return Repository.resolve_branch(self.repo_url, self.branch)
+
     def get_info(self):
         # flatten the data a bit so it will show in new relic as fields
         info = self.message_body["details"].copy()
@@ -188,10 +194,10 @@ class GithubPullRequestTransformer(GithubTransformer):
     # }
 
     def get_branch(self):
-        """
-        Pull requests don't use the actual branch, just the string "pull request"
-        """
-        return "pull request"
+        return None
+
+    def resolve_repo(self):
+        return self.repos.get(accepts_pull_requests=True)
 
     def get_repo(self):
         return self.message_body["details"]["event.base.repo.url"].replace(".git", "")
@@ -237,6 +243,10 @@ def __init__(self, message_body):
         self.message_body = message_body
         self.repo_url = message_body["payload"]["repo_url"]
         self.branch = None
+        self.repos = Repository.objects.filter(url=self.repo_url, active_status="active")
+
+    def resolve_repo(self):
+        return self.repos.exclude(branches__branch__endswith="*").get()
 
     def get_info(self):
         return self.message_body["payload"]