Skip to content
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
48 changes: 31 additions & 17 deletions src/sentry/issues/ownership/grammar.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class OwnershipRule(TypedDict):

event_tag = ~r"tags.[^:]+"

owners = _ owner+
owners = _ owner*
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One or more owners —> zero or more owners

Comment thread
shashjar marked this conversation as resolved.
owner = _ team_prefix identifier
team_prefix = "#"?

Expand Down Expand Up @@ -96,6 +96,8 @@ class Rule(namedtuple("Rule", "matcher owners")):
"""

def __str__(self) -> str:
if not self.owners:
return str(self.matcher)
owners = [o.dump() for o in self.owners]
owners_str = " ".join(
f"#{owner['identifier']}" if owner["type"] == "team" else owner["identifier"]
Expand Down Expand Up @@ -347,7 +349,13 @@ def owner_prefix(type: str) -> str:
return ""

for rule in rules:
text += f"{rule.matcher.type}:{rule.matcher.pattern} {' '.join([f'{owner_prefix(owner.type)}{owner.identifier}' for owner in rule.owners])}\n"
owners_str = " ".join(
f"{owner_prefix(owner.type)}{owner.identifier}" for owner in rule.owners
)
if owners_str:
text += f"{rule.matcher.type}:{rule.matcher.pattern} {owners_str}\n"
else:
text += f"{rule.matcher.type}:{rule.matcher.pattern}\n"

return text

Expand Down Expand Up @@ -439,21 +447,25 @@ def convert_codeowners_syntax(
# we skip associations
continue

if sentry_assignees:
# Replace source_root with stack_root for anchored paths
# /foo/dir -> anchored
# foo/dir -> anchored
# foo/dir/ -> anchored
# foo/ -> not anchored
if re.search(r"[\/].{1}", path):
path_with_stack_root = path.replace(
code_mapping.source_root, code_mapping.stack_root, 1
)
# flatten multiple '/' if not protocol
formatted_path = re.sub(r"(?<!:)\/{2,}", "/", path_with_stack_root)
result += f"codeowners:{formatted_path} {' '.join(sentry_assignees)}\n"
else:
result += f"codeowners:{path} {' '.join(sentry_assignees)}\n"
# Replace source_root with stack_root for anchored paths
# /foo/dir -> anchored
# foo/dir -> anchored
# foo/dir/ -> anchored
# foo/ -> not anchored
if re.search(r"[\/].{1}", path):
path_with_stack_root = path.replace(
code_mapping.source_root, code_mapping.stack_root, 1
)
# flatten multiple '/' if not protocol
formatted_path = re.sub(r"(?<!:)\/{2,}", "/", path_with_stack_root)
else:
formatted_path = path

if not code_owners:
# Exclusion rule: path with no owners means "no ownership" for this path
result += f"codeowners:{formatted_path}\n"
elif sentry_assignees:
result += f"codeowners:{formatted_path} {' '.join(sentry_assignees)}\n"

return result

Expand Down Expand Up @@ -578,6 +590,8 @@ def remove_deleted_owners_from_schema(
valid_rules = rules

for rule in rules:
if not rule["owners"]:
continue
valid_owners = rule["owners"]
for rule_owner in rule["owners"]:
if rule_owner["identifier"] not in owners_id.keys():
Expand Down
10 changes: 10 additions & 0 deletions src/sentry/models/projectownership.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from sentry.db.models import Model, cell_silo_model, sane_repr
from sentry.db.models.fields import FlexibleForeignKey
from sentry.issues.ownership.grammar import (
CODEOWNERS,
Matcher,
OwnershipSchema,
Rule,
Expand Down Expand Up @@ -135,6 +136,12 @@ def get_owners(

rules = cls._matching_ownership_rules(ownership, data)

# CODEOWNERS exclusion: if the last matching codeowners rule has no owners,
# it means "no ownership" — remove all codeowners rules from the match set
codeowners_matches = [r for r in rules if r.matcher.type == CODEOWNERS]
if codeowners_matches and not codeowners_matches[-1].owners:
rules = [r for r in rules if r.matcher.type != CODEOWNERS]

if not rules:
return [], None

Expand Down Expand Up @@ -217,6 +224,9 @@ def get_issue_owners(

with metrics.timer("projectownership.get_issue_owners_codeowners_rules"):
codeowners_rules = list(reversed(cls._matching_ownership_rules(codeowners, data)))
# CODEOWNERS exclusion: last matching rule (first after reversal) has no owners
if codeowners_rules and not codeowners_rules[0].owners:
return rules_with_owners
hydrated_codeowners_rules = cls._hydrate_rules(
project_id, codeowners_rules, OwnerRuleType.CODEOWNERS.value
)
Expand Down
129 changes: 129 additions & 0 deletions tests/sentry/issues/ownership/test_grammar.py
Original file line number Diff line number Diff line change
Expand Up @@ -1299,3 +1299,132 @@ def test_empty_bad_owners(self) -> None:
project = self.create_project()
messages = get_invalid_owner_details([], project.id)
assert messages == []


def test_parse_rules_with_exclusion_rule() -> None:
rules = parse_rules("codeowners:/apps/\ncodeowners:/apps/github\n")
assert rules == [
Rule(Matcher("codeowners", "/apps/"), []),
Rule(Matcher("codeowners", "/apps/github"), []),
]


def test_parse_rules_exclusion_rule_mixed() -> None:
rules = parse_rules("codeowners:/apps/ [email protected]\ncodeowners:/apps/github\n")
assert rules == [
Rule(Matcher("codeowners", "/apps/"), [Owner("user", "[email protected]")]),
Rule(Matcher("codeowners", "/apps/github"), []),
]


def test_dump_load_schema_exclusion_rule() -> None:
rule_with_owner = Rule(Matcher("codeowners", "/apps/"), [Owner("user", "[email protected]")])
rule_exclusion = Rule(Matcher("codeowners", "/apps/github"), [])

schema = dump_schema([rule_with_owner, rule_exclusion])
assert schema == {
"$version": 1,
"rules": [
{
"matcher": {"type": "codeowners", "pattern": "/apps/"},
"owners": [{"type": "user", "identifier": "[email protected]"}],
},
{
"matcher": {"type": "codeowners", "pattern": "/apps/github"},
"owners": [],
},
],
}
assert load_schema(schema) == [rule_with_owner, rule_exclusion]


def test_str_exclusion_rule() -> None:
assert str(Rule(Matcher("codeowners", "/apps/github"), [])) == "codeowners:/apps/github"


def test_convert_schema_to_rules_text_exclusion_rule() -> None:
assert (
convert_schema_to_rules_text(
{
"$version": 1,
"rules": [
{
"matcher": {"type": "codeowners", "pattern": "/apps/"},
"owners": [{"type": "user", "identifier": "[email protected]"}],
},
{
"matcher": {"type": "codeowners", "pattern": "/apps/github"},
"owners": [],
},
],
}
)
== "codeowners:/apps/ [email protected]\ncodeowners:/apps/github\n"
)


def test_convert_codeowners_syntax_exclusion_rule() -> None:
code_mapping = type("", (), {})()
code_mapping.stack_root = ""
code_mapping.source_root = ""

codeowners = "/apps/ @octocat\n/apps/github\n"
result = convert_codeowners_syntax(
codeowners,
{"@octocat": "[email protected]"},
code_mapping,
)
assert "codeowners:/apps/ [email protected]\n" in result
assert "codeowners:/apps/github\n" in result


def test_convert_codeowners_syntax_exclusion_with_comment() -> None:
code_mapping = type("", (), {})()
code_mapping.stack_root = ""
code_mapping.source_root = ""

codeowners = "/apps/ @octocat\n/apps/github # Skip\n"
result = convert_codeowners_syntax(
codeowners,
{"@octocat": "[email protected]"},
code_mapping,
)
assert "codeowners:/apps/ [email protected]\n" in result
assert "codeowners:/apps/github\n" in result


def test_convert_codeowners_syntax_unmapped_owner_not_exclusion() -> None:
code_mapping = type("", (), {})()
code_mapping.stack_root = ""
code_mapping.source_root = ""

codeowners = "/apps/ @unknown-user\n"
result = convert_codeowners_syntax(
codeowners,
{},
code_mapping,
)
assert "codeowners:/apps/" not in result


def test_convert_codeowners_syntax_exclusion_with_stack_root() -> None:
code_mapping = type("", (), {})()
code_mapping.stack_root = "webpack://static/"
code_mapping.source_root = ""

codeowners = "/apps/ @octocat\n/apps/github\n"
result = convert_codeowners_syntax(
codeowners,
{"@octocat": "[email protected]"},
code_mapping,
)
assert "codeowners:webpack://static/apps/ [email protected]\n" in result
assert "codeowners:webpack://static/apps/github\n" in result


def test_parse_code_owners_exclusion_rule() -> None:
codeowners = "/apps/ @getsentry/frontend\n/apps/github\n"
teams, usernames, emails = parse_code_owners(codeowners)
assert teams == ["@getsentry/frontend"]
assert usernames == []
assert emails == []
141 changes: 141 additions & 0 deletions tests/sentry/models/test_projectownership.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,147 @@ def test_get_owners_when_codeowners_and_issueowners_exists(self) -> None:
),
)

def test_get_owners_codeowners_exclusion_rule(self) -> None:
self.code_mapping = self.create_code_mapping(project=self.project)

rule_a = Rule(Matcher("codeowners", "/apps/"), [Owner("team", self.team.slug)])
rule_exclusion = Rule(Matcher("codeowners", "/apps/github"), [])

self.create_codeowners(
self.project,
self.code_mapping,
raw="/apps/ @tiger-team\n/apps/github\n",
schema=dump_schema([rule_a, rule_exclusion]),
)

# File in /apps/github — last matching codeowners rule is exclusion → no owners
assert ProjectOwnership.get_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/github/foo.py"}]}},
) == ([], None)

# File in /apps/bar — only matches rule_a → owned
self.assert_ownership_equals(
ProjectOwnership.get_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/bar/foo.py"}]}},
),
([Actor(id=self.team.id, actor_type=ActorType.TEAM)], [rule_a]),
)

def test_get_owners_codeowners_exclusion_overridden_by_later_rule(self) -> None:
self.code_mapping = self.create_code_mapping(project=self.project)

rule_a = Rule(Matcher("codeowners", "/apps/"), [Owner("team", self.team.slug)])
rule_exclusion = Rule(Matcher("codeowners", "/apps/github"), [])
rule_c = Rule(
Matcher("codeowners", "/apps/github/special"), [Owner("team", self.team.slug)]
Comment thread
shashjar marked this conversation as resolved.
Outdated
)

self.create_codeowners(
self.project,
self.code_mapping,
raw="/apps/ @tiger-team\n/apps/github\n/apps/github/special @tiger-team\n",
schema=dump_schema([rule_a, rule_exclusion, rule_c]),
)

# File in /apps/github/special — last matching codeowners rule has owners,
# so exclusion logic doesn't activate. All matching rules are returned.
self.assert_ownership_equals(
ProjectOwnership.get_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/github/special/foo.py"}]}},
),
(
[Actor(id=self.team.id, actor_type=ActorType.TEAM)],
[rule_a, rule_exclusion, rule_c],
),
)

def test_get_owners_codeowners_exclusion_issueowners_still_apply(self) -> None:
self.code_mapping = self.create_code_mapping(project=self.project)

codeowner_rule = Rule(Matcher("codeowners", "/apps/"), [Owner("team", self.team.slug)])
codeowner_exclusion = Rule(Matcher("codeowners", "/apps/github"), [])

self.create_codeowners(
self.project,
self.code_mapping,
raw="/apps/ @tiger-team\n/apps/github\n",
schema=dump_schema([codeowner_rule, codeowner_exclusion]),
)

issueowner_rule = Rule(Matcher("path", "*.py"), [Owner("user", self.user.email)])
ProjectOwnership.objects.create(
project_id=self.project.id,
schema=dump_schema([issueowner_rule]),
)

# File in /apps/github — codeowners excluded, but issueowner rule still matches
self.assert_ownership_equals(
ProjectOwnership.get_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/github/foo.py"}]}},
),
([Actor(id=self.user.id, actor_type=ActorType.USER)], [issueowner_rule]),
)

def test_get_issue_owners_codeowners_exclusion_rule(self) -> None:
Comment thread
shashjar marked this conversation as resolved.
self.code_mapping = self.create_code_mapping(project=self.project)

rule_a = Rule(Matcher("codeowners", "/apps/"), [Owner("team", self.team.slug)])
rule_exclusion = Rule(Matcher("codeowners", "/apps/github"), [])

ProjectOwnership.objects.create(project_id=self.project.id)

self.create_codeowners(
self.project,
self.code_mapping,
raw="/apps/ @tiger-team\n/apps/github\n",
schema=dump_schema([rule_a, rule_exclusion]),
)

# File in /apps/github — exclusion prevents codeowners ownership
assert (
ProjectOwnership.get_issue_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/github/foo.py"}]}},
)
== []
)

# File in /apps/bar — only matches rule_a → owned
assert ProjectOwnership.get_issue_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/bar/foo.py"}]}},
) == [(rule_a, [self.team], OwnerRuleType.CODEOWNERS.value)]

def test_get_issue_owners_codeowners_exclusion_issueowners_still_apply(self) -> None:
self.code_mapping = self.create_code_mapping(project=self.project)

codeowner_rule = Rule(Matcher("codeowners", "/apps/"), [Owner("team", self.team.slug)])
codeowner_exclusion = Rule(Matcher("codeowners", "/apps/github"), [])

issueowner_rule = Rule(Matcher("path", "*.py"), [Owner("team", self.team.slug)])

ProjectOwnership.objects.create(
project_id=self.project.id,
schema=dump_schema([issueowner_rule]),
)

self.create_codeowners(
self.project,
self.code_mapping,
raw="/apps/ @tiger-team\n/apps/github\n",
schema=dump_schema([codeowner_rule, codeowner_exclusion]),
)

# File in /apps/github — codeowners excluded, but issueowner rule matches
assert ProjectOwnership.get_issue_owners(
self.project.id,
{"stacktrace": {"frames": [{"filename": "apps/github/foo.py"}]}},
) == [(issueowner_rule, [self.team], OwnerRuleType.OWNERSHIP_RULE.value)]

def test_get_issue_owners_no_codeowners_or_issueowners(self) -> None:
assert ProjectOwnership.get_issue_owners(self.project.id, {}) == []

Expand Down
Loading