Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
8 changes: 4 additions & 4 deletions src/website/shared/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,12 @@ def cvss_details() -> str:
return ""

def maintainers() -> str:
# We need to query for the latest username of each maintainers, because
# those might have changed since they were written out in Nixpkgs; since we
# have the user id (which is stable), we can ask the GitHub API
# We need to query for the latest username of each maintainer, because
# those might have changed since they were written out in Nixpkgs; since
# we have the user id (which is stable), we can ask the GitHub API
maintainers_list = [
get_maintainer_username(maintainer, github)
for maintainer in cached_suggestion.all_maintainers
for maintainer in cached_suggestion.payload["maintainers"]
if "github_id" in maintainer and "github" in maintainer
]

Expand Down
41 changes: 39 additions & 2 deletions src/website/shared/listeners/cache_suggestions.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import itertools
import logging
import re
import urllib.parse
Expand All @@ -11,7 +12,7 @@
from shared.models import NixDerivation, NixMaintainer
from shared.models.cached import CachedSuggestions
from shared.models.cve import AffectedProduct, Metric, Version
from shared.models.linkage import CVEDerivationClusterProposal
from shared.models.linkage import CVEDerivationClusterProposal, MaintainersEdit
from shared.models.nix_evaluation import get_major_channel

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -109,6 +110,11 @@ def cache_new_suggestions(suggestion: CVEDerivationClusterProposal) -> None:
)

prefetched_metrics = Metric.objects.filter(container__cve=suggestion.cve)
packages = channel_structure(all_versions, derivations)
maintainers_edits = list(
suggestion.maintainers_edits.select_related("maintainer").all()
)
maintainers = maintainers_list(packages, maintainers_edits)

only_relevant_data = {
"pk": suggestion.pk,
Expand All @@ -117,8 +123,9 @@ def cache_new_suggestions(suggestion: CVEDerivationClusterProposal) -> None:
"title": relevant_piece["title"],
"description": relevant_piece["descriptions__value"],
"affected_products": affected_products,
"packages": channel_structure(all_versions, derivations),
"packages": packages,
"metrics": [to_dict(m) for m in prefetched_metrics],
"maintainers": maintainers,
}

# TODO: add format checking to avoid disasters in the frontend.
Expand Down Expand Up @@ -295,3 +302,33 @@ def parse_drv_name(name: str) -> tuple[str, str]:
return match.group(1), match.group(2)
else:
return name, ""


def maintainers_list(packages: dict, edits: list[MaintainersEdit]) -> list[dict]:
"""
Returns a deduplicated list (by GitHub ID) of all the maintainers, as dicts,
of all the affected packages linked to this suggestion, modified by
potential user-supplied edits.
"""

# Set of maintainers manually removed by the user. We use it to store
# maintainers that have already been added as well, for deduplication. If a
# maintainer's id is in this set at some point, it'll be ignored from there.
to_skip_or_seen: set[int] = {
m.maintainer.github_id
for m in edits
if m.type == MaintainersEdit.EditType.REMOVE
}
to_add: list[dict] = [
to_dict(m.maintainer) for m in edits if m.type == MaintainersEdit.EditType.ADD
]

maintainers: list[dict] = list()
all_maintainers = [m for pkg in packages.values() for m in pkg["maintainers"]]

for m in itertools.chain(all_maintainers, to_add):
if m["github_id"] not in to_skip_or_seen:
to_skip_or_seen.add(m["github_id"])
maintainers.append(m)

return maintainers
23 changes: 23 additions & 0 deletions src/website/shared/migrations/0049_maintainersedit.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Generated by Django 4.2.16 on 2025-05-14 13:58

from django.db import migrations, models
import django.db.models.deletion


class Migration(migrations.Migration):

dependencies = [
('shared', '0048_remove_attribute_suffix'),
]

operations = [
migrations.CreateModel(
name='MaintainersEdit',
fields=[
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('edit_type', models.CharField(choices=[('add', 'add'), ('remove', 'remove')], max_length=6)),
('maintainer', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='shared.nixmaintainer')),
('suggestion', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='maintainers_edits', to='shared.cvederivationclusterproposal')),
],
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 4.2.16 on 2025-05-16 08:30

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('shared', '0049_maintainersedit'),
]

operations = [
migrations.AddConstraint(
model_name='maintainersedit',
constraint=models.UniqueConstraint(fields=('suggestion', 'maintainer'), name='unique_maintainer_edit_per_suggestion'),
),
]
26 changes: 1 addition & 25 deletions src/website/shared/models/cached.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from django.core.serializers.json import DjangoJSONEncoder
from django.db import models

from shared.models.linkage import CVEDerivationClusterProposal
from shared.models.nix_evaluation import TimeStampMixin

Expand All @@ -18,28 +19,3 @@ class CachedSuggestions(TimeStampMixin):

# The exact format of this payload will change until it's properly defined.
payload = models.JSONField(encoder=DjangoJSONEncoder)

@property
# TODO: for now we blindly list all maintainers of all affected packages,
# but in the future we might want to be able to edit this list before
# creating the GitHub issue. When that happens, this function will need to
# be updated (or the GitHub creation code should use a distinct
# `maintainers()` property, for example).
def all_maintainers(self) -> list[dict]:
"""
Returns a deduplicated list (by GitHub ID) of all the maintainers of all
the affected packages linked to this suggestion.
"""

seen = set()
result = []
all_maintainers = [
m for pkg in self.payload["packages"].values() for m in pkg["maintainers"]
]

for m in all_maintainers:
if m["github_id"] not in seen:
seen.add(m["github_id"])
result.append(m)

return result
35 changes: 33 additions & 2 deletions src/website/shared/models/linkage.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
from enum import STRICT, IntFlag, auto

import pghistory
import shared.models.cached
from django.db import models
from django.utils.translation import gettext_lazy as _

import shared.models.cached
from shared.models.cve import CveRecord, Description, IssueStatus, NixpkgsIssue
from shared.models.nix_evaluation import NixDerivation, TimeStampMixin
from shared.models.nix_evaluation import NixDerivation, NixMaintainer, TimeStampMixin


def text_length(choices: type[models.TextChoices]) -> int:
Expand Down Expand Up @@ -62,6 +63,36 @@ def create_nixpkgs_issue(self) -> NixpkgsIssue:
return issue


class MaintainersEdit(models.Model):
"""
A single manual edit of the list of maintainers of a suggestion.
"""

class EditType(models.TextChoices):
ADD = "add", _("add")
REMOVE = "remove", _("remove")

edit_type = models.CharField(
max_length=text_length(EditType), choices=EditType.choices
)
maintainer = models.ForeignKey(NixMaintainer, on_delete=models.CASCADE)
suggestion = models.ForeignKey(
CVEDerivationClusterProposal,
related_name="maintainers_edits",
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk May 15, 2025

Choose a reason for hiding this comment

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

Can we enforce in a constraint that a maintainer edit only appears once on a suggestion? We'd still need to ensure our UI logic won't run into the constraint, but at least the data model would make invalid state impossible to represent and we won't need to do post-checks in the display logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that for example we don't have two edits that add the same maintainer to the same suggestion? Otherwise, reading things literally, I believe ForeignKey does enforce that an edit only belongs to one suggestion (it's a one-to-many relation).

Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk May 15, 2025

Choose a reason for hiding this comment

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

Do you mean that for example we don't have two edits that add the same maintainer to the same suggestion?

Yes. Using sets we wouldn't see the problem, but there'd still be potential for a small pile-up in the DB.

ForeignKey does enforce that an edit only belongs to one suggestion

Well, it's a one-to-many for edits, not for maintainers in those edits. You could have multiple edits with the same maintainer, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's a one-to-many for edits, not for maintainers in those edits. You could have multiple edits with the same maintainer, right?

Yes, but this is ok... ? You might want to independently add maintainer @tartempion to suggestion A and remove it from suggestion B. You initially wrote

Can we enforce in a constraint that a maintainer edit only appears once on a suggestion

which I believe is indeed enforced by the foreign key. So, if I understand correctly, the only remaining constraint that isn't enforced and that we want to enforce is the one you said, that several edits to the same package with the same maintainer shouldn't be allowed. Is that correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that correct?

Yes, that's what I meant.

on_delete=models.CASCADE,
)

class Meta: # type: ignore[override]
constraints = [
# Ensures that a maintainer can only be added or removed once per
# suggestion.
models.UniqueConstraint(
fields=["suggestion", "maintainer"],
name="unique_maintainer_edit_per_suggestion",
)
]


class ProvenanceFlags(IntFlag, boundary=STRICT):
PACKAGE_NAME_MATCH = auto()
VERSION_CONSTRAINT_INRANGE = auto()
Expand Down
2 changes: 1 addition & 1 deletion src/website/webview/templates/components/suggestion.html
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
{% nixpkgs_package_list cached_suggestion.packages %}
{% endif %}

{% maintainers_list cached_suggestion.packages %}
{% maintainers_list cached_suggestion.maintainers %}

{% if user|is_maintainer_or_admin %}
<div class="change-issue-state">
Expand Down
19 changes: 4 additions & 15 deletions src/website/webview/templatetags/viewutils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import datetime
import json
from typing import Any, TypedDict, cast
from typing import Any, TypedDict

from django import template
from django.template.context import Context
Expand Down Expand Up @@ -196,17 +196,6 @@ def suggestion_activity_log(

@register.inclusion_tag("components/maintainers_list.html")
def maintainers_list(
packages: PackageList,
) -> dict[str, list[Maintainer]]:
maintainers = [
maintainer
for _, package in packages.items()
for maintainer in package["maintainers"]
]
github_ids = set()
unique_maintainers = [
cast(Maintainer, m)
for m in maintainers
if m["github_id"] not in github_ids and not github_ids.add(m["github_id"])
]
return {"maintainers": unique_maintainers}
maintainers: list[dict],
) -> dict[str, list[dict]]:
return {"maintainers": maintainers}