Mirrors/plane
1
0
Fork 0
mirror of https://github.com/makeplane/plane.git synced 2026-09-02 03:59:00 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(api): expand null relations to null instead of an empty object

Browse Source 
`?expand=updated_by` was reported in #4639 as returning a bare UUID
because `updated_by` was missing from the expansion mapper. #7667 added
the key, but that alone did not fix the reported request: `BaseModel.save`
leaves `updated_by` NULL until a record is first updated, and expanding a
null relation called `UserLiteSerializer(None)`, which DRF resolves via
`get_initial()` to `{}`. The app layer never got the key at all, and its
null case produced a ghost user with blank names.

Resolve the relation off the instance instead of guessing arity from the
already-serialized value, and keep a null relation null so the expanded
and unexpanded responses agree. Add `updated_by` to the app mapper, and
hoist its two duplicated copies into one `get_expansion_mapper()` -- the
duplication is why the original fix reached only one of them.

`issue_attachment` is deliberately left out of the unified mapper: it is
the reverse manager of the legacy `IssueAttachment` model, which
`IssueAttachmentLiteSerializer` (`model = FileAsset`) cannot serialize.

Also document the supported `expand` values, which were previously
undiscoverable, and add the first test coverage for `expand`.

Claude-Session: https://claude.ai/code/session_0142ihfx57JhqVnvs5w73aX7
This commit is contained in:
Sriram Veeraghanta
2026-08-30 19:06:18 +05:30
parent 23cfe3cc42
commit 2be57e2467
6 changed files with 444 additions and 119 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
apps/api/plane/api/serializers/base.py
View File

@@ -2,10 +2,17 @@
# SPDX-License-Identifier: AGPL-3.0-only
# See the LICENSE file for details.
# Django imports
from django.db import models
# Third party imports
from rest_framework import serializers
# Distinguishes "the instance has no such attribute" from "the attribute is None".
_MISSING = object()
class BaseSerializer(serializers.ModelSerializer):
"""
Base serializer providing common functionality for all model serializers.
@@ -106,11 +113,27 @@ class BaseSerializer(serializers.ModelSerializer):
}
# Check if field in expansion then expand the field
if expand in expansion:
if isinstance(response.get(expand), list):
exp_serializer = expansion[expand](getattr(instance, expand), many=True)
else:
exp_serializer = expansion[expand](getattr(instance, expand))
response[expand] = exp_serializer.data
# Resolve against the instance rather than guessing arity from the
# already-serialized value: a to-many relation the serializer does
# not render is not a list in `response`. `_MISSING` separates "no
# such relation" from "the relation is null" -- a missing reverse
# relation raises RelatedObjectDoesNotExist, an AttributeError.
related = getattr(instance, expand, _MISSING)
if isinstance(related, models.Manager):
response[expand] = expansion[expand](related, many=True).data
elif isinstance(related, models.Model):
response[expand] = expansion[expand](related).data
elif related is None:
# A null relation stays null, matching the unexpanded
# response. Serializing None emits an object built from the
# nested serializer's defaults instead, which is what made
# `expand` unusable for `updated_by`: it is null until the
# first update.
response[expand] = None
# Anything else means `expand` names something that is not a
# relation on this instance -- a queryset annotation or a
# SerializerMethodField of the same name. Leave the value the
# serializer already produced rather than clobbering it.
else:
# You might need to handle this case differently
response[expand] = getattr(instance, f"{expand}_id", None)

214
apps/api/plane/app/serializers/base.py
View File

@@ -2,9 +2,88 @@
# SPDX-License-Identifier: AGPL-3.0-only
# See the LICENSE file for details.
from django.db import models
from rest_framework import serializers
# Distinguishes "the instance has no such attribute" from "the attribute is None".
_MISSING = object()
# Relations in the expansion mapper that are to-many.
MANY_EXPANSION_FIELDS = frozenset(
{
"members",
"assignees",
"labels",
"issue_cycle",
"issue_relation",
"issue_intake",
"issue_reactions",
"issue_link",
"sub_issues",
"issue_related",
}
)
def get_expansion_mapper():
"""Return the ``expand`` key -> serializer mapping.
Shared by ``_filter_fields`` and ``to_representation`` so the two cannot drift
apart -- keeping two copies is how ``updated_by`` came to be added to the
``/api/v1/`` mapper and to neither of these (makeplane/plane#4639).
``issue_attachment`` is deliberately absent: ``Issue.issue_attachment`` is the
reverse manager of the legacy ``IssueAttachment`` model, which
``IssueAttachmentLiteSerializer`` (``model = FileAsset``) cannot serialize.
Attachments are served by the ``issue_attachments`` block in
``to_representation`` below.
Imports stay inside the function because the serializers import this module.
"""
from . import (
WorkspaceLiteSerializer,
ProjectLiteSerializer,
UserLiteSerializer,
StateLiteSerializer,
IssueSerializer,
LabelSerializer,
CycleIssueSerializer,
IssueLiteSerializer,
IssueRelationSerializer,
IntakeIssueLiteSerializer,
IssueReactionLiteSerializer,
IssueLinkLiteSerializer,
RelatedIssueSerializer,
)
return {
"user": UserLiteSerializer,
"workspace": WorkspaceLiteSerializer,
"project": ProjectLiteSerializer,
"default_assignee": UserLiteSerializer,
"project_lead": UserLiteSerializer,
"state": StateLiteSerializer,
"created_by": UserLiteSerializer,
"updated_by": UserLiteSerializer,
"issue": IssueSerializer,
"actor": UserLiteSerializer,
"owned_by": UserLiteSerializer,
"members": UserLiteSerializer,
"assignees": UserLiteSerializer,
"labels": LabelSerializer,
"issue_cycle": CycleIssueSerializer,
"parent": IssueLiteSerializer,
"issue_relation": IssueRelationSerializer,
"issue_intake": IntakeIssueLiteSerializer,
"issue_related": RelatedIssueSerializer,
"issue_reactions": IssueReactionLiteSerializer,
"issue_link": IssueLinkLiteSerializer,
"sub_issues": IssueLiteSerializer,
}
class BaseSerializer(serializers.ModelSerializer):
id = serializers.PrimaryKeyRelatedField(read_only=True)
@@ -52,70 +131,12 @@ class DynamicBaseSerializer(BaseSerializer):
elif isinstance(item, dict):
allowed.append(list(item.keys())[0])
for field in allowed:
if field not in self.fields:
from . import (
WorkspaceLiteSerializer,
ProjectLiteSerializer,
UserLiteSerializer,
StateLiteSerializer,
IssueSerializer,
LabelSerializer,
CycleIssueSerializer,
IssueLiteSerializer,
IssueRelationSerializer,
IntakeIssueLiteSerializer,
IssueReactionLiteSerializer,
IssueLinkLiteSerializer,
RelatedIssueSerializer,
)
# Expansion mapper
expansion = {
"user": UserLiteSerializer,
"workspace": WorkspaceLiteSerializer,
"project": ProjectLiteSerializer,
"default_assignee": UserLiteSerializer,
"project_lead": UserLiteSerializer,
"state": StateLiteSerializer,
"created_by": UserLiteSerializer,
"issue": IssueSerializer,
"actor": UserLiteSerializer,
"owned_by": UserLiteSerializer,
"members": UserLiteSerializer,
"assignees": UserLiteSerializer,
"labels": LabelSerializer,
"issue_cycle": CycleIssueSerializer,
"parent": IssueLiteSerializer,
"issue_relation": IssueRelationSerializer,
"issue_intake": IntakeIssueLiteSerializer,
"issue_related": RelatedIssueSerializer,
"issue_reactions": IssueReactionLiteSerializer,
"issue_link": IssueLinkLiteSerializer,
"sub_issues": IssueLiteSerializer,
}
if field not in self.fields and field in expansion:
self.fields[field] = expansion[field](
many=(
True
if field
in [
"members",
"assignees",
"labels",
"issue_cycle",
"issue_relation",
"issue_intake",
"issue_reactions",
"issue_attachment",
"issue_link",
"sub_issues",
"issue_related",
]
else False
)
)
missing = [field for field in allowed if field not in self.fields]
if missing:
expansion = get_expansion_mapper()
for field in missing:
if field in expansion:
self.fields[field] = expansion[field](many=field in MANY_EXPANSION_FIELDS)
return self.fields
@@ -124,58 +145,26 @@ class DynamicBaseSerializer(BaseSerializer):
# Ensure 'expand' is iterable before processing
if self.expand:
expansion = get_expansion_mapper()
for expand in self.expand:
if expand in self.fields:
# Import all the expandable serializers
from . import (
WorkspaceLiteSerializer,
ProjectLiteSerializer,
UserLiteSerializer,
StateLiteSerializer,
IssueSerializer,
LabelSerializer,
CycleIssueSerializer,
IssueRelationSerializer,
IntakeIssueLiteSerializer,
IssueLiteSerializer,
IssueReactionLiteSerializer,
IssueAttachmentLiteSerializer,
IssueLinkLiteSerializer,
RelatedIssueSerializer,
)
# Expansion mapper
expansion = {
"user": UserLiteSerializer,
"workspace": WorkspaceLiteSerializer,
"project": ProjectLiteSerializer,
"default_assignee": UserLiteSerializer,
"project_lead": UserLiteSerializer,
"state": StateLiteSerializer,
"created_by": UserLiteSerializer,
"issue": IssueSerializer,
"actor": UserLiteSerializer,
"owned_by": UserLiteSerializer,
"members": UserLiteSerializer,
"assignees": UserLiteSerializer,
"labels": LabelSerializer,
"issue_cycle": CycleIssueSerializer,
"parent": IssueLiteSerializer,
"issue_relation": IssueRelationSerializer,
"issue_intake": IntakeIssueLiteSerializer,
"issue_related": RelatedIssueSerializer,
"issue_reactions": IssueReactionLiteSerializer,
"issue_attachment": IssueAttachmentLiteSerializer,
"issue_link": IssueLinkLiteSerializer,
"sub_issues": IssueLiteSerializer,
}
# Check if field in expansion then expand the field
if expand in expansion:
if isinstance(response.get(expand), list):
exp_serializer = expansion[expand](getattr(instance, expand), many=True)
else:
exp_serializer = expansion[expand](getattr(instance, expand))
response[expand] = exp_serializer.data
# Resolve against the instance rather than guessing arity from the
# already-serialized value. `_MISSING` separates "no such relation"
# from "the relation is null": `members` and `sub_issues` are
# SerializerMethodField/annotation names on some serializers here,
# and those must keep the value the serializer already produced.
related = getattr(instance, expand, _MISSING)
if isinstance(related, models.Manager):
response[expand] = expansion[expand](related, many=True).data
elif isinstance(related, models.Model):
response[expand] = expansion[expand](related).data
elif related is None:
# A null relation stays null, matching the unexpanded response.
# Serializing None emits an object built from the nested
# serializer's field defaults instead.
response[expand] = None
else:
# You might need to handle this case differently
response[expand] = getattr(instance, f"{expand}_id", None)
@@ -184,6 +173,7 @@ class DynamicBaseSerializer(BaseSerializer):
if "issue_attachments" in self.fields or "issue_attachments" in self.expand:
# Import the model here to avoid circular imports
from plane.db.models import FileAsset
from . import IssueAttachmentLiteSerializer
issue_id = getattr(instance, "id", None)

32
apps/api/plane/tests/contract/api/conftest.py Normal file
View File

@@ -0,0 +1,32 @@
# Copyright (c) 2023-present Plane Software, Inc. and contributors
# SPDX-License-Identifier: AGPL-3.0-only
# See the LICENSE file for details.
import pytest
from django.core.cache import cache
def _clear_api_key_throttle_keys():
"""Delete only the ApiKeyRateThrottle history keys from the shared cache.
``ApiKeyRateThrottle.get_cache_key`` returns ``api_key:<token>``, so scoping
the pattern to ``api_key:`` removes just this throttle's entries instead of
wiping unrelated cache state.
"""
cache.delete_pattern("api_key:*")
@pytest.fixture(autouse=True)
def _reset_api_key_throttle_cache():
"""Clear the API-key throttle state around every test in this package.
Every test here authenticates with the same token from the ``api_token``
fixture, and the request history behind ``ApiKeyRateThrottle`` lives in a
cache the whole session shares. Without this the count leaks across tests
until the suite trips its own rate limit and later tests fail with 429
regardless of the code under test. Mirrors ``_reset_auth_throttle_cache``
in ``plane/tests/contract/app/test_authentication.py``.
"""
_clear_api_key_throttle_keys()
yield
_clear_api_key_throttle_keys()

85
apps/api/plane/tests/contract/api/test_projects.py
View File

@@ -259,3 +259,88 @@ class TestProjectListCreateAPIEndpoint:
# The dispatch was attempted but its failure was swallowed by
# transaction.on_commit(robust=True).
mocked_activity.delay.assert_called_once()
@pytest.mark.contract
class TestProjectListExpand:
"""Contract tests for ``?expand=`` on GET /api/v1/workspaces/{slug}/projects/.
Regression for https://github.com/makeplane/plane/issues/4639. ``updated_by``
originally came back as a bare UUID because it was missing from the expansion
mapper. Adding it was not enough: ``BaseModel.save`` leaves ``updated_by`` NULL
until a record is first updated, and expanding a null relation emitted ``{}``
rather than ``null``.
"""
def get_url(self, workspace_slug):
return f"/api/v1/workspaces/{workspace_slug}/projects/"
def make_project(self, workspace, user, identifier, updated_by=None):
"""Create a project with the audit columns set explicitly.
``BaseModel.save()`` overwrites ``created_by``/``updated_by`` from the
current request user (there is none here), so the values have to be
written with a queryset update that bypasses ``save()``.
"""
project = Project.objects.create(
name=f"Project {identifier}",
identifier=identifier,
workspace=workspace,
project_lead=user,
)
ProjectMember.objects.create(project=project, member=user, role=20)
Project.objects.filter(pk=project.pk).update(created_by=user, updated_by=updated_by)
project.refresh_from_db()
return project
def get_project(self, response, identifier):
results = response.json()["results"]
match = next((p for p in results if p["identifier"] == identifier), None)
assert match is not None, f"{identifier} missing from results: {[p['identifier'] for p in results]}"
return match
@pytest.mark.django_db
def test_expand_updated_by_returns_a_user_object(self, api_key_client, workspace, create_user):
"""The exact request from the issue: expand=created_by,updated_by,project_lead."""
self.make_project(workspace, create_user, "EXP", updated_by=create_user)
response = api_key_client.get(
self.get_url(workspace.slug),
{"expand": "created_by,updated_by,project_lead"},
)
assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}"
payload = self.get_project(response, "EXP")
for field in ("created_by", "updated_by", "project_lead"):
assert isinstance(payload[field], dict), f"{field} was not expanded: {payload[field]!r}"
assert payload[field]["id"] == str(create_user.id)
assert "display_name" in payload[field]
@pytest.mark.django_db
def test_expand_null_relation_returns_null(self, api_key_client, workspace, create_user):
"""A project that has never been edited has updated_by = NULL.
Expanding it must stay ``null`` rather than becoming an empty object.
"""
project = self.make_project(workspace, create_user, "NEW")
assert project.updated_by_id is None, "fixture precondition: updated_by is null"
response = api_key_client.get(
self.get_url(workspace.slug),
{"expand": "created_by,updated_by"},
)
assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}"
payload = self.get_project(response, "NEW")
assert payload["updated_by"] is None
assert isinstance(payload["created_by"], dict)
@pytest.mark.django_db
def test_without_expand_updated_by_stays_a_plain_id(self, api_key_client, workspace, create_user):
self.make_project(workspace, create_user, "RAW", updated_by=create_user)
response = api_key_client.get(self.get_url(workspace.slug))
assert response.status_code == status.HTTP_200_OK, f"Got {response.status_code}: {response.data!r}"
payload = self.get_project(response, "RAW")
assert payload["updated_by"] == str(create_user.id)

180
apps/api/plane/tests/unit/serializers/test_expand.py Normal file
View File

@@ -0,0 +1,180 @@
# Copyright (c) 2023-present Plane Software, Inc. and contributors
# SPDX-License-Identifier: AGPL-3.0-only
# See the LICENSE file for details.
"""Regression tests for the ``expand`` query parameter.
See https://github.com/makeplane/plane/issues/4639 -- ``?expand=updated_by``
returned the bare UUID instead of a user object, and once the key was added to
the mapper it returned an empty object for the (very common) case of a record
that has never been updated.
"""
from uuid import uuid4
import pytest
from plane.api.serializers import ModuleLiteSerializer, ProjectSerializer
from plane.app.serializers import IssueSerializer as AppIssueSerializer
from plane.app.serializers.base import get_expansion_mapper
from plane.app.serializers.project import ProjectListSerializer
from plane.db.models import Issue, Module, ModuleMember, Project, User, Workspace
# The three fields from the request in issue #4639.
AUDIT_EXPANDS = ["created_by", "updated_by", "project_lead"]
def build_user():
return User(
id=uuid4(),
email="ada@plane.so",
first_name="Ada",
last_name="Lovelace",
display_name="ada",
)
def build_project(**kwargs):
workspace = Workspace(id=uuid4(), name="Test Workspace", slug="test-workspace")
return Project(
id=uuid4(),
name="Test Project",
identifier="TEST",
workspace=workspace,
**kwargs,
)
@pytest.mark.unit
class TestPublicAPIExpand:
"""``/api/v1/`` -- plane.api.serializers.base.BaseSerializer."""
def test_updated_by_expands_to_a_user_object(self):
"""The original report: updated_by came back as a bare UUID."""
user = build_user()
project = build_project(created_by=user, updated_by=user, project_lead=user)
data = ProjectSerializer(project, expand=AUDIT_EXPANDS).data
for field in AUDIT_EXPANDS:
assert isinstance(data[field], dict), f"{field} was not expanded: {data[field]!r}"
assert data[field]["id"] == user.id
assert data[field]["display_name"] == "ada"
def test_null_relation_expands_to_null_not_an_empty_object(self):
"""updated_by is null until a record is first updated (BaseModel.save).
Expanding it used to emit ``{}`` -- the nested serializer built from its
own defaults -- which does not match the unexpanded response.
"""
user = build_user()
project = build_project(created_by=user, updated_by=None, project_lead=None)
data = ProjectSerializer(project, expand=AUDIT_EXPANDS).data
assert data["updated_by"] is None
assert data["project_lead"] is None
assert isinstance(data["created_by"], dict)
def test_expand_does_not_change_the_unexpanded_contract(self):
user = build_user()
project = build_project(created_by=user, updated_by=None, project_lead=None)
data = ProjectSerializer(project).data
assert data["created_by"] == user.id
assert data["updated_by"] is None
assert data["project_lead"] is None
def test_expand_name_that_is_not_a_relation_is_not_a_field(self):
project = build_project(created_by=build_user())
data = ProjectSerializer(project, expand=["not_a_field"]).data
assert "not_a_field" not in data
def test_to_many_relation_expands_to_a_list_of_objects(self, db):
"""Guards the to-many arm of the dispatch.
Arity is now read off the ORM object (a related manager) rather than
guessed from the already-serialized value, so this pins that to-many
expansion keeps working.
"""
user = User.objects.create(email="member@plane.so", display_name="member")
workspace = Workspace.objects.create(name="Test Workspace", slug="test-workspace", owner=user)
project = Project.objects.create(name="Test Project", identifier="TEST", workspace=workspace)
module = Module.objects.create(name="Test Module", project=project, workspace=workspace)
ModuleMember.objects.create(module=module, member=user, project=project, workspace=workspace)
data = ModuleLiteSerializer(module, expand=["members"]).data
assert isinstance(data["members"], list)
assert [m["id"] for m in data["members"]] == [user.id]
def test_audit_fields_expand_identically(self):
"""created_by and updated_by must stay in lockstep.
#4639 happened because only created_by was in the mapper.
"""
user = build_user()
project = build_project(created_by=user, updated_by=user)
data = ProjectSerializer(project, expand=["created_by", "updated_by"]).data
assert data["created_by"] == data["updated_by"]
assert data["created_by"]["id"] == user.id
@pytest.mark.unit
class TestAppAPIExpand:
"""``/api/`` (internal) -- plane.app.serializers.base.DynamicBaseSerializer."""
def build_issue(self, created_by, updated_by):
workspace = Workspace(id=uuid4(), name="Test Workspace", slug="test-workspace")
project = Project(id=uuid4(), name="Test Project", identifier="TEST", workspace=workspace)
return Issue(
id=uuid4(),
name="Test work item",
project=project,
workspace=workspace,
created_by=created_by,
updated_by=updated_by,
)
def test_updated_by_expands_to_a_user_object(self):
user = build_user()
issue = self.build_issue(created_by=user, updated_by=user)
data = AppIssueSerializer(issue, expand=["created_by", "updated_by"]).data
assert isinstance(data["updated_by"], dict), f"not expanded: {data['updated_by']!r}"
assert data["updated_by"]["id"] == user.id
def test_null_relation_expands_to_null(self):
"""The app UserLiteSerializer leaves most fields writable, so a null
relation used to serialize into a fabricated user with blank names."""
user = build_user()
issue = self.build_issue(created_by=user, updated_by=None)
data = AppIssueSerializer(issue, expand=["created_by", "updated_by"]).data
assert data["updated_by"] is None
def test_expand_naming_a_method_field_keeps_the_serializer_value(self, db):
"""`members` is a SerializerMethodField on ProjectListSerializer and not a
relation on Project at all. Resolving it used to raise AttributeError and
return a 500; the value the serializer produced must survive instead."""
workspace = Workspace(id=uuid4(), name="Test Workspace", slug="test-workspace")
project = Project(id=uuid4(), name="Test Project", identifier="TEST", workspace=workspace)
data = ProjectListSerializer(project, expand=["members"]).data
assert data["members"] == []
def test_mapper_is_shared_by_both_call_sites(self):
"""_filter_fields and to_representation had drifted apart, which is how
updated_by ended up in one mapper and not the other."""
mapper = get_expansion_mapper()
assert "created_by" in mapper
assert "updated_by" in mapper

19
apps/api/plane/utils/openapi/parameters.py
View File

@@ -480,13 +480,28 @@ EXPAND_PARAMETER = OpenApiParameter(
name="expand",
type=OpenApiTypes.STR,
location=OpenApiParameter.QUERY,
description="Comma-separated list of related fields to expand in response",
description=(
"Comma-separated list of related fields to expand in response. An expanded "
"single-valued relation is returned as a nested object instead of an id, and "
"stays `null` when it is not set. Which values apply depends on the resource: "
"`created_by` and `updated_by` on most resources; `project`, `workspace`, "
"`project_lead`, `default_assignee`, `owned_by`, `actor`, `state`, "
"`estimate_point`, `issue`, `user`; and on work items `parent`, `assignees` "
"and `labels`. Only pass names from this list -- a value that is a plain "
"field on the resource rather than a relation is not expandable, and passing "
"it clears that field in the response."
),
required=False,
examples=[
OpenApiExample(
name="Expand audit users",
value="created_by,updated_by",
description="Include full user details for who created and last updated the record",
),
OpenApiExample(
name="Expand assignees",
value="assignees",
description="Include full assignee details",
description="Include full assignee details (work items)",
),
OpenApiExample(
name="Multiple expansions",