[FIX] Keep DRF 3.15.2 safe for org-scoped serializers (drop server-set uniqueness)#2091
[FIX] Keep DRF 3.15.2 safe for org-scoped serializers (drop server-set uniqueness)#2091chandrasekharan-zipstack wants to merge 1 commit intomainZipstack/unstract:mainfrom fix/drf-315-org-uniquenessZipstack/unstract:fix/drf-315-org-uniquenessCopy head branch name to clipboard
Conversation
…t uniqueness)
DRF 3.15 extended ModelSerializer.get_uniqueness_extra_kwargs() and
get_unique_together_validators() to derive uniqueness from a model's
multi-field UniqueConstraints, not just Meta.unique_together (3.14
behaviour). Models using DefaultOrganizationMixin carry org-scoped
constraints (e.g. AdapterInstance's unique_organization_adapter over
adapter_name/adapter_type/organization). `organization` is a nullable FK
set server-side in DefaultOrganizationMixin.save() via UserContext and is
never sent by clients.
Under 3.15 this made serializers (notably fields="__all__") mark
`organization` required=True AND build a UniqueTogetherValidator whose
enforce_required_fields also demands it, so create/update endpoints 400
with {"attr":"organization","code":"required"} across adapters,
connectors, workflows, api-deployments, prompt-studio, etc.
Add DropServerSetUniquenessMixin (utils/serializer/org_uniqueness.py) that
strips organization/created_by/modified_by from BOTH the uniqueness
extra_kwargs and the unique-together validators, restoring 3.14 behaviour.
DB UniqueConstraints + IntegrityErrorMixin still enforce and cleanly
surface real duplicates. Apply it once on AuditSerializer (covers all OSS
create serializers) and on IntegrityErrorMixin (so cloud serializers that
import it inherit the fix for free). This supersedes the DRF 3.14 revert,
keeping 3.15.2 and its CVE-2024-21520 XSS patch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DoRxNHky8RoutBCFrSzZQ7
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughA new ChangesOrg-scoped uniqueness validation fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| backend/utils/serializer/org_uniqueness.py | New mixin that strips server-set fields from both DRF 3.15 uniqueness code paths; logic is sound but drops entire validators on any overlap, leaving non-server-set field pairs in the same constraint without DRF-layer pre-validation. |
| backend/backend/serializers.py | Adds DropServerSetUniquenessMixin to AuditSerializer via correct MRO ordering (mixin before ModelSerializer); straightforward and safe. |
| backend/utils/serializer/integrity_error_mixin.py | IntegrityErrorMixin now inherits DropServerSetUniquenessMixin; cloud serializers that import it inherit the fix automatically with no functional change to IntegrityError handling. |
| backend/adapter_processor_v2/tests/test_org_uniqueness.py | Regression guard tests for DRF 3.15 org-uniqueness; accesses private _declared_fields API and uses a module-level Django setup pattern that conflicts with standard pytest-django lifecycle. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DRF 3.15 ModelSerializer.get_fields] --> B[get_uniqueness_extra_kwargs]
A --> C[get_unique_together_validators]
B --> D{DropServerSetUniquenessMixin}
C --> D
D -->|strip organization / created_by / modified_by from extra_kwargs & hidden_fields| E[Safe extra_kwargs returned]
D -->|drop any validator whose fields overlap SERVER_SET_UNIQUENESS_FIELDS| F[Filtered validators returned]
E --> G[Serializer fields: organization not required=True]
F --> H[No UniqueTogetherValidator demands organization]
G --> I[create/update succeeds]
H --> I
I --> J{Duplicate record?}
J -->|DB IntegrityError raised| K{IntegrityErrorMixin present?}
K -->|Yes| L[Clean 400 ValidationError returned]
K -->|No| M[500 unhandled IntegrityError]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[DRF 3.15 ModelSerializer.get_fields] --> B[get_uniqueness_extra_kwargs]
A --> C[get_unique_together_validators]
B --> D{DropServerSetUniquenessMixin}
C --> D
D -->|strip organization / created_by / modified_by from extra_kwargs & hidden_fields| E[Safe extra_kwargs returned]
D -->|drop any validator whose fields overlap SERVER_SET_UNIQUENESS_FIELDS| F[Filtered validators returned]
E --> G[Serializer fields: organization not required=True]
F --> H[No UniqueTogetherValidator demands organization]
G --> I[create/update succeeds]
H --> I
I --> J{Duplicate record?}
J -->|DB IntegrityError raised| K{IntegrityErrorMixin present?}
K -->|Yes| L[Clean 400 ValidationError returned]
K -->|No| M[500 unhandled IntegrityError]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
backend/utils/serializer/org_uniqueness.py:34-39
**Entire validator dropped for partial server-set overlap**
When a `UniqueConstraint` includes both client-supplied and server-set fields (e.g. `(adapter_name, adapter_type, organization)`), the entire `UniqueTogetherValidator` is removed. This means DRF no longer pre-validates uniqueness of the client-supplied subset (`adapter_name + adapter_type` within an org) before hitting the database. For serializers that mix in `IntegrityErrorMixin` this is caught at the DB layer and re-raised cleanly; for `AuditSerializer`-only subclasses (e.g. `ConnectorInstanceSerializer`) a real duplicate will raise an unhandled `IntegrityError` and return a 500. The PR description acknowledges this trade-off and notes it matches pre-3.15 behavior — worth a quick audit that every `AuditSerializer`-only descendant over an org-scoped constraint also inherits `IntegrityErrorMixin`, or has its own `save()` guard.
### Issue 2 of 3
backend/adapter_processor_v2/tests/test_org_uniqueness.py:37-38
**Test relies on private DRF attribute `_declared_fields`**
`serializer._declared_fields` is an internal implementation detail not part of DRF's public API. If DRF renames or restructures it in a future minor release the test would break with an `AttributeError` rather than a meaningful assertion failure. The public `serializer.fields` mapping is the stable surface; `_declared_fields` only differs from it in that it excludes dynamically added fields, which matters only if the test is specifically checking for that distinction.
### Issue 3 of 3
backend/adapter_processor_v2/tests/test_org_uniqueness.py:19-21
**Module-level Django setup bypasses pytest-django lifecycle**
Calling `django.setup()` at import time with `if not apps.ready` is a workaround for running the file directly (`python test_org_uniqueness.py`). When pytest-django is the test runner it configures Django before collecting tests, so `apps.ready` is already `True` and the guard is a no-op — correct, but the `os.environ.setdefault` line above it is also a no-op if pytest-django already set `DJANGO_SETTINGS_MODULE`. This makes the intent ambiguous. The idiomatic pytest-django approach is a `conftest.py` fixture; if the test genuinely needs no DB, the module-level block can simply be removed.
Reviews (1): Last reviewed commit: "[FIX] Keep DRF 3.15.2 safe for org-scope..." | Re-trigger Greptile
| def get_unique_together_validators(self) -> list[Any]: | ||
| return [ | ||
| validator | ||
| for validator in super().get_unique_together_validators() | ||
| if not (set(getattr(validator, "fields", ())) & SERVER_SET_UNIQUENESS_FIELDS) | ||
| ] |
There was a problem hiding this comment.
Entire validator dropped for partial server-set overlap
When a UniqueConstraint includes both client-supplied and server-set fields (e.g. (adapter_name, adapter_type, organization)), the entire UniqueTogetherValidator is removed. This means DRF no longer pre-validates uniqueness of the client-supplied subset (adapter_name + adapter_type within an org) before hitting the database. For serializers that mix in IntegrityErrorMixin this is caught at the DB layer and re-raised cleanly; for AuditSerializer-only subclasses (e.g. ConnectorInstanceSerializer) a real duplicate will raise an unhandled IntegrityError and return a 500. The PR description acknowledges this trade-off and notes it matches pre-3.15 behavior — worth a quick audit that every AuditSerializer-only descendant over an org-scoped constraint also inherits IntegrityErrorMixin, or has its own save() guard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/utils/serializer/org_uniqueness.py
Line: 34-39
Comment:
**Entire validator dropped for partial server-set overlap**
When a `UniqueConstraint` includes both client-supplied and server-set fields (e.g. `(adapter_name, adapter_type, organization)`), the entire `UniqueTogetherValidator` is removed. This means DRF no longer pre-validates uniqueness of the client-supplied subset (`adapter_name + adapter_type` within an org) before hitting the database. For serializers that mix in `IntegrityErrorMixin` this is caught at the DB layer and re-raised cleanly; for `AuditSerializer`-only subclasses (e.g. `ConnectorInstanceSerializer`) a real duplicate will raise an unhandled `IntegrityError` and return a 500. The PR description acknowledges this trade-off and notes it matches pre-3.15 behavior — worth a quick audit that every `AuditSerializer`-only descendant over an org-scoped constraint also inherits `IntegrityErrorMixin`, or has its own `save()` guard.
How can I resolve this? If you propose a fix, please make it concise.| declared_fields = serializer._declared_fields | ||
| extra_kwargs, _hidden = serializer.get_uniqueness_extra_kwargs( |
There was a problem hiding this comment.
Test relies on private DRF attribute
_declared_fields
serializer._declared_fields is an internal implementation detail not part of DRF's public API. If DRF renames or restructures it in a future minor release the test would break with an AttributeError rather than a meaningful assertion failure. The public serializer.fields mapping is the stable surface; _declared_fields only differs from it in that it excludes dynamically added fields, which matters only if the test is specifically checking for that distinction.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/adapter_processor_v2/tests/test_org_uniqueness.py
Line: 37-38
Comment:
**Test relies on private DRF attribute `_declared_fields`**
`serializer._declared_fields` is an internal implementation detail not part of DRF's public API. If DRF renames or restructures it in a future minor release the test would break with an `AttributeError` rather than a meaningful assertion failure. The public `serializer.fields` mapping is the stable surface; `_declared_fields` only differs from it in that it excludes dynamically added fields, which matters only if the test is specifically checking for that distinction.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| os.environ.setdefault("DJANGO_SETTINGS_MODULE", "backend.settings.test") | ||
| if not apps.ready: | ||
| django.setup() |
There was a problem hiding this comment.
Module-level Django setup bypasses pytest-django lifecycle
Calling django.setup() at import time with if not apps.ready is a workaround for running the file directly (python test_org_uniqueness.py). When pytest-django is the test runner it configures Django before collecting tests, so apps.ready is already True and the guard is a no-op — correct, but the os.environ.setdefault line above it is also a no-op if pytest-django already set DJANGO_SETTINGS_MODULE. This makes the intent ambiguous. The idiomatic pytest-django approach is a conftest.py fixture; if the test genuinely needs no DB, the module-level block can simply be removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/adapter_processor_v2/tests/test_org_uniqueness.py
Line: 19-21
Comment:
**Module-level Django setup bypasses pytest-django lifecycle**
Calling `django.setup()` at import time with `if not apps.ready` is a workaround for running the file directly (`python test_org_uniqueness.py`). When pytest-django is the test runner it configures Django before collecting tests, so `apps.ready` is already `True` and the guard is a no-op — correct, but the `os.environ.setdefault` line above it is also a no-op if pytest-django already set `DJANGO_SETTINGS_MODULE`. This makes the intent ambiguous. The idiomatic pytest-django approach is a `conftest.py` fixture; if the test genuinely needs no DB, the module-level block can simply be removed.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Unstract test resultsPer-group results
Critical paths
|
|
Closing in favour of a durable model-level fix (mark server-managed org/audit fields editable=False so DRF auto-skips the org-scoped UniqueTogetherValidator everywhere, not just on two base classes). |
|
What
Add
DropServerSetUniquenessMixin(backend/utils/serializer/org_uniqueness.py) and apply it toAuditSerializerandIntegrityErrorMixinso create/update endpoints stop 400ing underdjangorestframework==3.15.2.Supersedes the DRF 3.14 revert #2090 — this keeps 3.15.2 (and its CVE-2024-21520 XSS patch) while removing the regression that revert was working around.
Why
DRF 3.15 extended
ModelSerializer.get_uniqueness_extra_kwargs()andget_unique_together_validators()to derive uniqueness from a model's multi-fieldUniqueConstraints — not justMeta.unique_togetheras in 3.14. Models usingDefaultOrganizationMixincarry org-scoped constraints (e.g.AdapterInstance.unique_organization_adapteroveradapter_name, adapter_type, organization).organizationis a nullable FK set server-side inDefaultOrganizationMixin.save()viaUserContextand is never sent by clients.Under 3.15 this made serializers (notably
fields="__all__") markorganizationrequired=Trueand build aUniqueTogetherValidatorwhoseenforce_required_fieldsalso demands it. Result: create/update 400 with{"attr":"organization","code":"required"}across adapters, connectors, workflows, api-deployments, prompt-studio, etc. (this is what broke ~146 staging tests).Both DRF methods had to be neutralized: for
AdapterInstancetherequiredkwarg test passes but theUniqueTogetherValidatoris the active 400 path — a validator-only or kwargs-only fix would be insufficient.How
DropServerSetUniquenessMixinstripsorganization/created_by/modified_byfrom both the uniquenessextra_kwargs/hidden-fields and the unique-together validators, restoring 3.14 behaviour. DBUniqueConstraints +IntegrityErrorMixinstill enforce and cleanly surface real duplicate errors.Applied with minimal churn:
AuditSerializergains the mixin — covers all OSS create serializers (adapter/connector/workflow/api/prompt-studio/platform-api, allfields="__all__").IntegrityErrorMixingains the mixin — cloud serializers that import it (e.g.AgenticProjectSerializer,LookupDefinitionSerializer) inherit the fix for free. Serializers inheriting both (WorkflowSerializer,PipelineSerializer,APIDeploymentSerializer,CustomToolSerializer) get it once via MRO (verified — harmless).Coverage audit
__all__)__all__)__all__)__all__)__all__)__all__)organizationnot in serializer fields, so DRF skips the constraint (issuperset check fails)No OSS writable serializer over an org-scoped multi-field constraint was left uncovered.
Can this PR break any existing features
No. The mixin only removes uniqueness machinery for server-set fields that clients never supply anyway; DB
UniqueConstraints remain authoritative andIntegrityErrorMixinstill returns clean validation errors on real duplicates. This restores the exact pre-3.15 (3.14) serializer behaviour for these fields.Database Migrations
None.
Env Config
None.
Related Issues or PRs
Supersedes #2090 (DRF 3.14 revert). Cloud counterpart:
Zipstack/unstract-cloud#fix/drf-315-org-uniqueness.Dependencies Versions
No change — keeps
djangorestframework==3.15.2.Notes on Testing
Added
backend/adapter_processor_v2/tests/test_org_uniqueness.py(3 tests): assertsAdapterInstanceSerializer().fields["organization"].required is False, that no server-set field appears in the uniquenessrequiredextra_kwargs, and that noUniqueTogetherValidatorcovers a server-set field. Verified the tests FAIL without the mixin (the(adapter_name, adapter_type, organization)validator reappears) and PASS with it, under DRF 3.15.2.🤖 Generated with Claude Code