Skip to content

fix(api): expand=updated_by returns the user, and null relations stay null #9717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sriramveeraghanta wants to merge 1 commit into
base: preview
Choose a base branch
from
Open

fix(api): expand=updated_by returns the user, and null relations stay null #9717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 28 additions & 5 deletions apps/api/plane/api/serializers/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -106,11 +113,27 @@ def to_representation(self, instance):
}
# 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)
Expand Down
214 changes: 102 additions & 112 deletions apps/api/plane/app/serializers/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -52,70 +131,12 @@ def _filter_fields(self, fields):
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

Expand All @@ -124,58 +145,26 @@ def to_representation(self, instance):

# 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)
Expand All @@ -184,6 +173,7 @@ def to_representation(self, instance):
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)

Expand Down
32 changes: 32 additions & 0 deletions apps/api/plane/tests/contract/api/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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()
Loading
Loading