Fix ContentReplicaRouter to allow relations across default/replica - #1503
Conversation
ContentReplicaRouter didn't implement allow_relation, so Django fell back to its default same-database check. Domains read during content app requests come from "replica", but pulp_maven's path_index builds a new, unsaved Artifact (db=None) and assigns that domain to it. Since "replica" != None, Django raised: ValueError: Cannot assign "<Domain: ...>": the current database router prevents this relation. "replica" mirrors "default", so relations between objects loaded from either database are always safe. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe router now explicitly permits all model relations, avoiding Django’s same-database rejection when replica-loaded domains are assigned to new unsaved objects, with parameterized tests covering replica, default, and unset database states. Sequence diagram for replica-read relation assignmentsequenceDiagram
participant ContentApp
participant Router as ContentReplicaRouter
participant Replica
participant Artifact
participant Domain
ContentApp->>Replica: db_for_read(Domain)
Replica-->>ContentApp: Domain(db=replica)
ContentApp->>Artifact: Create unsaved Artifact(db=None)
ContentApp->>Router: allow_relation(Artifact, Domain)
Router-->>ContentApp: True
ContentApp->>Artifact: Assign Domain relation
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pulp_service/pulp_service/app/database_router.py" line_range="38" />
<code_context>
def db_for_write(self, model, **hints):
return "default"
+ def allow_relation(self, obj1, obj2, **hints):
+ # "replica" mirrors "default", so relations between objects loaded from
+ # either database (or not yet assigned to one) are always safe. Without
+ # this, Django's default same-db check rejects assigning a replica-read
+ # object (e.g. a Domain) to a new, unsaved instance (db=None), such as
+ # when pulp_maven builds an Artifact during a content-app read.
+ return True
+
def allow_migrate(self, db, app_label, model_name=None, **hints):
</code_context>
<issue_to_address>
**issue (broader_impact):** `allow_relation` returns `True` for every pair of objects, including objects loaded from an unrelated database alias such as `other`. When such an object is assigned to an object saved on `default`, Django accepts the relation and the resulting foreign key references the other database's row ID, raising an integrity error when absent or linking to the wrong row when IDs collide.
**Triggers:** When callers explicitly load or construct an object whose `_state.db` is an alias other than `default`, `replica`, or `None`.
**Suggested fix:** Return `True` only when both database states are in the supported `default`/`replica`/`None` set, and return `None` otherwise so Django's normal same-database check applies.
```suggestion
if {obj1._state.db, obj2._state.db} <= {"default", "replica", None}:
return True
return None
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the router permits an object from an unintended database, Django could persist an incorrect foreign-key or many-to-many association because cross-database constraints are not enforced. Reverting stops the new behavior, but any bad associations already written would need to be identified and repaired or recomputed.
Blocking findings: pulp_service/pulp_service/app/database_router.py:38
| # this, Django's default same-db check rejects assigning a replica-read | ||
| # object (e.g. a Domain) to a new, unsaved instance (db=None), such as | ||
| # when pulp_maven builds an Artifact during a content-app read. | ||
| return True |
There was a problem hiding this comment.
issue (broader_impact): allow_relation returns True for every pair of objects, including objects loaded from an unrelated database alias such as other. When such an object is assigned to an object saved on default, Django accepts the relation and the resulting foreign key references the other database's row ID, raising an integrity error when absent or linking to the wrong row when IDs collide.
Triggers: When callers explicitly load or construct an object whose _state.db is an alias other than default, replica, or None.
Suggested fix: Return True only when both database states are in the supported default/replica/None set, and return None otherwise so Django's normal same-database check applies.
| return True | |
| if {obj1._state.db, obj2._state.db} <= {"default", "replica", None}: | |
| return True | |
| return None |
ContentReplicaRouter didn't implement allow_relation, so Django fell back to its default same-database check. Domains read during content app requests come from "replica", but pulp_maven's path_index builds a new, unsaved Artifact (db=None) and assigns that domain to it. Since "replica" != None, Django raised:
ValueError: Cannot assign "<Domain: ...>": the current database
router prevents this relation.
"replica" mirrors "default", so relations between objects loaded from either database are always safe.
Summary by Sourcery
Permit safe relations across mirrored default and replica databases in the content database router.
Bug Fixes:
Tests: