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
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
39 changes: 37 additions & 2 deletions src/website/shared/listeners/cache_suggestions.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,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 +109,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 +122,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 +301,32 @@ 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 of all the
affected packages linked to this suggestion, modified by potential
user-supplied edits.
"""

to_remove = {
m.maintainer for m in edits if m.type == MaintainersEdit.EditType.REMOVE
}
to_add = [m.maintainer for m in edits if m.type == MaintainersEdit.EditType.ADD]
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.

Why is this a list and to_remove a set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use them differently: to_remove is basically a hashset that we query to know if we should add a maintainer to the working list. I realize, by the way, that it shoud be only github ids and not the whole maintainers. Maybe a better name is to_skip and it should be merged with seen below; they have the same role.

to_add is a list of maintainer to process and add at the end, but we deduplicate already when adding (and we need to do it by github_id, and not on the whole maintainer object as I mentioned below in another comment). We could make to_add a set but it's useless. Deduplication happen later.


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

for m in all_maintainers:
if m["github_id"] not in seen_ids and m["github_id"] not in to_remove:
seen_ids.add(m["github_id"])
maintainers.append(m)

for m in to_add:
if m.github_id not in seen_ids and m.github_id not in to_remove:
seen_ids.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')),
],
),
]
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
25 changes: 23 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,26 @@ 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 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}