Skip to content

PolymorphicBaseModel's custom model_serializer ignores exclude/include (and other model_dump kwargs) #855

Description

@thusser

Symptom

PolymorphicBaseModel.inject_class_on_serialization() (pyobs/utils/serialization.py:44-49) is declared @model_serializer(mode="wrap") but never calls the handler it's given — it builds the output dict itself via raw getattr() over type(self).model_fields:

@model_serializer(mode="wrap")
def inject_class_on_serialization(self, handler: ValidatorFunctionWrapHandler) -> dict[str, Any]:
    result = {field_name: getattr(self, field_name) for field_name in type(self).model_fields}
    result["class"] = f"{self.__module__}.{self.__class__.__name__}"
    return result

Because it never delegates to handler(self), every model_dump()/model_dump_json() call on any PolymorphicBaseModel subclass silently ignores exclude, include, by_alias, exclude_none, exclude_unset, exclude_defaults, etc. Verified directly:

from pyobs.robotic.task import Task
t = Task(id=1, name="t1", duration=100, updated_at="2026-01-01T00:00:00Z")
t.model_dump(exclude={"updated_at"})
# {'id': 1, ..., 'updated_at': '2026-01-01T00:00:00Z', 'class': 'pyobs.robotic.task.Task'}
# 'updated_at' is still there -- exclude had no effect.

PolymorphicBaseModel subclasses include at least Task, Script, Constraint, Merit, and Target (and their concrete leaf types) -- anywhere a caller passes exclude=/include= to model_dump() on one of these gets silently wrong output instead of an error.

Where this bit us

pyobs-core PR #854 (issue #848) added Scheduler._changed_task_ids(), which needed to compare two Task dumps while excluding the updated_at timestamp field (a portal-side save marker, not scheduling content). task.model_dump(exclude={"updated_at"}) compiled and ran without error but silently included updated_at anyway, which would have made a no-op re-save look like a "changed" task and force spurious reschedules. Caught by a regression test (test_changed_task_ids_ignores_updated_at) before merge; worked around locally in that PR via Scheduler._content_dump(), which calls model_dump() with no exclude and pops the key from the resulting dict afterward instead.

Why not fixed in place

The handler-bypass isn't an accident -- the existing comment says it's there "to avoid Pydantic v2 resolving field schemas against the abstract base type when nested in a parent model" (i.e. calling handler(self) directly can serialize a subclass instance using the parent's abstract-typed field schema, dropping subclass-specific fields). So the fix isn't simply "call handler" -- it needs the hand-rolled dict-building logic to itself honor info.exclude/info.include (via the SerializationInfo parameter model_serializer wrap-mode functions can accept), and exclude/include support pydantic's nested dict-style specs (e.g. {"field": {"nested_field"}}), not just flat sets, so doing this generally and correctly needs real thought and its own test coverage across the affected subclasses. That's a bigger, riskier change than #848 called for, so it wasn't bundled into that PR.

Proposed fix

Extend inject_class_on_serialization to accept info: SerializationInfo and apply info.exclude/info.include (and ideally by_alias, exclude_none, etc.) to the manually-built field dict before returning it, matching what handler(self) would have done for a non-polymorphic model. Add tests exercising model_dump(exclude=...)/include=... on at least Task and one other PolymorphicBaseModel subclass, both standalone and nested under an abstract-typed parent field (to confirm the original abstract-type-resolution fix this serializer exists for still holds).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions