Skip to content
Open
Changes from all commits
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
64 changes: 64 additions & 0 deletions backend/aci/common/schemas/undefined_aware_base_model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
from enum import Enum
from typing import Any

from pydantic import BaseModel, model_validator

from aci.common.logging_setup import get_logger

logger = get_logger(__name__)


class BehaviorOnDumpWithoutExcludeUnset(Enum):
WARN = "warn"
ERROR = "error"


class UndefinedAwareBaseModel(BaseModel):
"""
A base model that allows all fields to be nullable and use a custom validator to check for
non-nullable fields.
"""

_non_nullable_fields: list[str] = []
_dump_without_exclude_unset_behavior: BehaviorOnDumpWithoutExcludeUnset = (
BehaviorOnDumpWithoutExcludeUnset.WARN
)
Comment on lines +22 to +25
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Use PrivateAttr() for internal configuration fields.

The fields _non_nullable_fields and _dump_without_exclude_unset_behavior are currently treated as regular Pydantic model fields, which means they:

  • Will be included in model_dump() output
  • Will be validated during instantiation
  • Will appear in the JSON schema

Since these are internal configuration attributes, they should use PrivateAttr() from Pydantic v2.

Additionally, the mutable default [] for _non_nullable_fields is a Python anti-pattern that could cause modifications in one instance to affect others.

Apply this diff to fix the issue:

+from pydantic import BaseModel, model_validator, PrivateAttr
-from pydantic import BaseModel, model_validator

 class UndefinedAwareBaseModel(BaseModel):
     """
     A base model that allows all fields to be nullable and use a custom validator to check for
     non-nullable fields.
     """

-    _non_nullable_fields: list[str] = []
+    _non_nullable_fields: list[str] = PrivateAttr(default_factory=list)
-    _dump_without_exclude_unset_behavior: BehaviorOnDumpWithoutExcludeUnset = (
-        BehaviorOnDumpWithoutExcludeUnset.WARN
-    )
+    _dump_without_exclude_unset_behavior: BehaviorOnDumpWithoutExcludeUnset = PrivateAttr(
+        default=BehaviorOnDumpWithoutExcludeUnset.WARN
+    )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
_non_nullable_fields: list[str] = []
_dump_without_exclude_unset_behavior: BehaviorOnDumpWithoutExcludeUnset = (
BehaviorOnDumpWithoutExcludeUnset.WARN
)
from pydantic import BaseModel, model_validator, PrivateAttr
class UndefinedAwareBaseModel(BaseModel):
"""
A base model that allows all fields to be nullable and use a custom validator to check for
non-nullable fields.
"""
_non_nullable_fields: list[str] = PrivateAttr(default_factory=list)
_dump_without_exclude_unset_behavior: BehaviorOnDumpWithoutExcludeUnset = PrivateAttr(
default=BehaviorOnDumpWithoutExcludeUnset.WARN
)
# …rest of class…
🤖 Prompt for AI Agents
In backend/aci/common/schemas/undefined_aware_base_model.py around lines 22-25,
the internal config attributes _non_nullable_fields and
_dump_without_exclude_unset_behavior are currently regular Pydantic fields;
change them to Pydantic PrivateAttr()s so they are not validated, serialized, or
exposed in JSON schema. Specifically, replace the list default with
PrivateAttr(default_factory=list) to avoid a shared mutable default, and set the
enum config with PrivateAttr(default=BehaviorOnDumpWithoutExcludeUnset.WARN) so
it remains an internal attribute.


@model_validator(mode="after")
def validate_non_nullable_fields(self) -> "UndefinedAwareBaseModel":
"""
As there is no easy way to differentiate between "None" and "Undefined" with Pydantic.
We don't know whether caller do not provide a value for a field or want to explicitly set
it to None. We use a workaround as follow:
- We allow all fields to be nullable and default to None ewhen defining the pydantic model.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix typo in docstring.

"ewhen" should be "when".

-    - We allow all fields to be nullable and default to None ewhen defining the pydantic model.
+    - We allow all fields to be nullable and default to None when defining the pydantic model.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- We allow all fields to be nullable and default to None ewhen defining the pydantic model.
- We allow all fields to be nullable and default to None when defining the pydantic model.
🤖 Prompt for AI Agents
In backend/aci/common/schemas/undefined_aware_base_model.py around line 33, the
docstring contains a typo: "ewhen" should be "when"; update the docstring to
replace "ewhen" with "when" so the sentence reads correctly.

- We use a custom validator to check for non-nullable fields.
- Only when caller provided a value, field name will be in `model.model_fields_set`.
- When updating to database, we either check `model.model_fields_set` or use
`model_dump(exclude_unset=True)` to exclude unset fields.
- If model_dump is called without exclude_unset, we console warn (default) or raise error.
"""

non_nullable_fields = self._non_nullable_fields
for field in self.model_fields_set:
if field in non_nullable_fields and getattr(self, field) is None:
raise ValueError(f"{field} cannot be None if it is provided.")
return self
Comment on lines +22 to +45
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Unclear how subclasses populate _non_nullable_fields.

The validation logic relies on _non_nullable_fields, but there's no documented or type-safe mechanism for subclasses to declare which fields should be non-nullable. Users may not understand how to properly configure this base model.

Consider one of these approaches:

  1. Class-level configuration using ConfigDict (if feasible with your design):
class ConfigDict:
    non_nullable_fields: list[str] = []
  1. Document the expected usage pattern in the docstring:
class MyModel(UndefinedAwareBaseModel):
    field1: str | None = None
    field2: int | None = None
    _non_nullable_fields: list[str] = PrivateAttr(default_factory=lambda: ["field1"])
  1. Consider leveraging Pydantic 2.12's new MISSING sentinel (based on learnings), which provides a native way to distinguish undefined from None without requiring this workaround.


def model_dump(self, **kwargs: Any) -> dict:
# Warn if model_dump is called without exclude_unset
if "exclude_unset" not in kwargs:
match self._dump_without_exclude_unset_behavior:
case BehaviorOnDumpWithoutExcludeUnset.WARN:
logger.warning(
"model_dump is called without providing `exclude_unset` args. This may "
"accidentally include unset fields."
)
case BehaviorOnDumpWithoutExcludeUnset.ERROR:
raise SyntaxError(
"model_dump is called without providing `exclude_unset` args. This may "
"accidentally include unset fields."
)
Comment on lines +57 to +60
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Use ValueError instead of SyntaxError.

SyntaxError is semantically incorrect for runtime validation errors—it's reserved for syntax parsing failures. Use ValueError or define a custom exception.

Apply this diff:

-                    raise SyntaxError(
+                    raise ValueError(
                         "model_dump is called without providing `exclude_unset` args. This may "
                         "accidentally include unset fields."
                     )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
raise SyntaxError(
"model_dump is called without providing `exclude_unset` args. This may "
"accidentally include unset fields."
)
raise ValueError(
"model_dump is called without providing `exclude_unset` args. This may "
"accidentally include unset fields."
)
🤖 Prompt for AI Agents
In backend/aci/common/schemas/undefined_aware_base_model.py around lines 57 to
60, the code currently raises SyntaxError when model_dump is called without
exclude_unset; replace this with ValueError (or a project-specific custom
exception) to correctly represent a runtime validation error, updating the raise
statement message to use ValueError(...) and keeping the existing message text
unchanged.

return super().model_dump(**kwargs)

# TODO: Do we need to intercept __getattribute__() to protect when directly accessing the field
# via `model.field` ?