Skip to content
3 changes: 2 additions & 1 deletion src/backend/core/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class ServiceFactory(factory.django.DjangoModelFactory):
A factory for generating service instances for testing and development purposes.
"""

name = factory.Sequence(lambda n: f"test-index-{n!s}")
slug = factory.Sequence(lambda n: f"testidx{n!s}")
name = factory.LazyAttribute(lambda o: f"Test Service {o.slug}")
Comment thread
StephanMeijer marked this conversation as resolved.
created_at = factory.Faker("date_time_this_year", tzinfo=None)
is_active = True
client_id = "some_client_id"
Expand Down
73 changes: 73 additions & 0 deletions src/backend/core/migrations/0003_service_slug_and_editable_name.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import re

import django.core.validators
from django.db import migrations, models


def backfill_slug_from_name(apps, schema_editor):
Comment thread
StephanMeijer marked this conversation as resolved.
Outdated
Service = apps.get_model("core", "Service") # noqa: N806
seen = set()
for service in Service.objects.all():
derived = re.sub(r"[^a-zA-Z0-9]", "", service.name or "").lower()
if not derived:
raise RuntimeError(
f"Cannot derive slug for Service id={service.pk!r} "
f"name={service.name!r}: name contains no alphanumeric characters."
)
if derived in seen:
raise RuntimeError(
f"Slug collision while backfilling: name={service.name!r} -> "
f"slug={derived!r} already used by another service."
)
seen.add(derived)
service.slug = derived
service.save(update_fields=["slug"])


class Migration(migrations.Migration):
dependencies = [
("core", "0002_service_client_id_service_services"),
]

operations = [
migrations.AddField(
model_name="service",
name="slug",
field=models.CharField(max_length=20, null=True),
),
migrations.RunPython(
backfill_slug_from_name,
reverse_code=migrations.RunPython.noop,
),
migrations.AlterField(
model_name="service",
name="slug",
field=models.SlugField(
editable=False,
help_text=(
"Stable identifier used in the OpenSearch index name. "
"Lowercase alphanumeric only. Set on creation, immutable thereafter."
),
max_length=20,
unique=True,
validators=[
django.core.validators.RegexValidator(
message="Slug must contain only lowercase letters and digits.",
regex="^[a-z0-9]+$",
)
],
),
),
migrations.AlterField(
model_name="service",
name="name",
field=models.CharField(max_length=255),
),
migrations.AddConstraint(
model_name="service",
constraint=models.CheckConstraint(
condition=models.Q(("slug__regex", "^[a-z0-9]+$")),
name="slug_alphanumeric_only",
),
),
]
50 changes: 43 additions & 7 deletions src/backend/core/models.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
"""Models for find's core app"""

import re
import secrets
import string

from django.contrib.auth.models import AbstractUser
from django.core.exceptions import ValidationError
from django.core.validators import RegexValidator
from django.db import models
from django.db.models.functions import Length
from django.utils.text import slugify
from django.utils.translation import gettext_lazy as _

models.CharField.register_lookup(Length)
TOKEN_LENGTH = 50
SLUG_REGEX = r"^[a-z0-9]+$"
SLUG_VALIDATOR = RegexValidator(
regex=SLUG_REGEX,
message=_("Slug must contain only lowercase letters and digits."),
)


class User(AbstractUser):
Expand All @@ -20,15 +27,23 @@ class User(AbstractUser):
class Service(models.Model):
"""Service registered to index its documents to our find"""

name = models.SlugField(max_length=20, unique=True)
name = models.CharField(max_length=255)
slug = models.SlugField(
max_length=20,
unique=True,
editable=False,
validators=[SLUG_VALIDATOR],
Comment thread
StephanMeijer marked this conversation as resolved.
help_text=_(
"Stable identifier used in the OpenSearch index name. "
"Lowercase alphanumeric only. Set on creation, immutable thereafter."
),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also you may want to modify the admin model and prepopulate this field using dedicated utility (see Django docs).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point on the admin flow. I think we should make slug editable only at creation, but not prepopulate it from name: slug is the stable OpenSearch index identifier, while name is an editable display label. Prepopulating from name could suggest the slug should track the display name, which is exactly what this PR is decoupling.

So I propose to expose slug on the admin add form for an explicit, intentional value, make it read-only on existing services via ServiceAdmin.get_readonly_fields(), and keep the model-level immutability check as a backstop outside the admin.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perfect.

token = models.CharField(max_length=TOKEN_LENGTH)
created_at = models.DateTimeField(auto_now_add=True)
is_active = models.BooleanField(default=True)
client_id = models.CharField(blank=True, null=True)
services = models.ManyToManyField(
"self",
verbose_name=_("Allowed services for search"),
blank=True,
"self", blank=True, verbose_name=_("Allowed services for search")
)

class Meta:
Expand All @@ -41,14 +56,35 @@ class Meta:
condition=models.Q(token__length=TOKEN_LENGTH),
name="token_length_exact_50",
),
models.CheckConstraint(
condition=models.Q(slug__regex=SLUG_REGEX),
name="slug_alphanumeric_only",
),
]

def __str__(self):
return self.name

def save(self, *args, **kwargs):
"""Automatically slugify the service name and generate a token on creation"""
self.name = slugify(self.name)
"""Generate token, auto-derive slug on creation, enforce slug immutability.

- ``slug`` is auto-derived from ``name`` if not provided (strip
non-alphanumeric, lowercase). It is immutable after creation.
- ``name`` is a free-form display field and can be edited.
- ``token`` is generated once on creation if missing.
"""
if not self.slug:
self.slug = re.sub(r"[^a-zA-Z0-9]", "", self.name or "").lower()
Comment thread
StephanMeijer marked this conversation as resolved.
Outdated
if self.pk is not None:
Comment thread
StephanMeijer marked this conversation as resolved.
Outdated
stored_slug = (
Service.objects.filter(pk=self.pk)
.values_list("slug", flat=True)
.first()
)
if self.slug != stored_slug:
raise ValidationError(
{"slug": _("Service.slug is immutable after creation.")}
)
if not self.token:
self.token = self.generate_secure_token()
super().save(*args, **kwargs)
Expand Down
52 changes: 45 additions & 7 deletions src/backend/core/tests/test_models_services.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests Service model for find's core app."""

from django.core.exceptions import ValidationError
from django.db import DataError, IntegrityError

import pytest
Expand All @@ -9,18 +10,37 @@
pytestmark = pytest.mark.django_db


def test_models_services_name_unique():
"""The name field should be unique across services."""
def test_models_services_slug_unique():
"""The slug field must be unique across services."""
service = factories.ServiceFactory()

with pytest.raises(IntegrityError):
factories.ServiceFactory(name=service.name)
factories.ServiceFactory(slug=service.slug)


def test_models_services_name_slugified():
"""The name field should be slugified."""
service = factories.ServiceFactory(name="My service name")
assert service.name == "my-service-name"
def test_models_services_name_not_required_unique():
"""Two services may share the same display name."""
factories.ServiceFactory(slug="aa", name="Same Name")
factories.ServiceFactory(slug="bb", name="Same Name")
Comment thread
StephanMeijer marked this conversation as resolved.
Outdated


def test_models_services_slug_auto_derived_from_name():
"""When no slug is provided, it is auto-derived from name (alphanumeric, lowercase)."""
service = factories.ServiceFactory(slug=None, name="My Service Name")
assert service.slug == "myservicename"
assert service.name == "My Service Name"


def test_models_services_slug_auto_derivation_strips_special_chars():
"""Slug derivation strips hyphens, underscores, spaces and punctuation."""
service = factories.ServiceFactory(slug=None, name="docs-service_v2!")
assert service.slug == "docsservicev2"


def test_models_services_slug_rejects_non_alphanumeric():
"""Explicit non-alphanumeric slugs are rejected by the DB check constraint."""
with pytest.raises(IntegrityError):
factories.ServiceFactory(slug="has-hyphen")


def test_models_services_token_50_characters_exact():
Expand All @@ -39,3 +59,21 @@ def test_models_services_token_50_characters_more():
"""The token field should be 50 characters long."""
with pytest.raises(DataError):
factories.ServiceFactory(token="a" * 51)


def test_service_slug_immutable_after_creation():
"""The slug field must be immutable after creation."""
service = factories.ServiceFactory(slug="originalname")
service.slug = "differentname"
with pytest.raises(ValidationError):
service.save()


def test_service_name_editable_after_creation():
"""The name field is freely editable after creation."""
service = factories.ServiceFactory(slug="myslug", name="Original")
service.name = "New Display Name"
service.save()
service.refresh_from_db()
assert service.name == "New Display Name"
assert service.slug == "myslug"
Comment thread
StephanMeijer marked this conversation as resolved.
Outdated
9 changes: 6 additions & 3 deletions src/backend/demo/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@

DEV_SERVICES = (
{
"name": "docs",
"slug": "docs",
"name": "Docs",
"client_id": "impress",
"token": "find-api-key-for-docs-with-exactly-50-chars-length",
},
{
"name": "drive",
"slug": "drive",
"name": "Drive",
"client_id": "drive",
"token": "find-api-key-for-driv-with-exactly-50-chars-length",
},
{
"name": "conversations",
"slug": "conversations",
"name": "Conversations",
"client_id": "conversations",
"token": "find-api-key-for-conv-with-exactly-50-chars-length",
},
Expand Down
6 changes: 3 additions & 3 deletions src/backend/demo/tests/test_commands_create_demo.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ def test_commands_create_demo(settings):
"""The create_demo management command should create objects as expected."""
call_command("create_demo")

assert models.Service.objects.exclude(name="docs").count() == 4
assert models.Service.objects.exclude(slug="docs").count() == 4
assert opensearch_client().count(index=settings.OPENSEARCH_INDEX)["count"] == 4

docs = models.Service.objects.get(name="docs")
docs = models.Service.objects.get(slug="docs")
assert docs.client_id == "impress"

drive = models.Service.objects.get(name="drive")
drive = models.Service.objects.get(slug="drive")
assert drive.client_id == "drive"
Loading