Fixed #36580 -- Fixed constraint validation crash when condition uses a `ForeignObject`. #19798

JaeHyuckSa · 2025-08-30T11:10:12Z

Trac ticket number

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

docs/releases/5.2.6.txt

jacobtylerwalls · 2025-08-30T11:38:29Z

Thanks for the speedy work here @JaeHyuckSa, but I just triaged the ticket to needsinfo, so I think we need to close this for now.

jacobtylerwalls · 2025-08-30T11:56:23Z

My mistake, reopening! I had forgotten to change user__isnull when testing against test models copied from the docs 🤦

jacobtylerwalls · 2025-09-02T12:26:09Z

buildbot, test on oracle.

jacobtylerwalls

Thanks @JaeHyuckSa. The constraint on the test model can't be created on Oracle:

django.db.utils.DatabaseError: ORA-00904: "COMPOSITE_PK_COMMENT"."USER_ID": invalid identifier

@felixxm have you seen this Oracle error before? If there's a workaround, it would be nice to avoid creating a separate test model just to skip a test on Oracle.

tests/composite_pk/models/tenant.py

tests/composite_pk/test_models.py

felixxm · 2025-09-02T14:50:38Z

django.db.utils.DatabaseError: ORA-00904: "COMPOSITE_PK_COMMENT"."USER_ID": invalid identifier
@felixxm have you seen this Oracle error before? If there's a workaround, it would be nice to avoid creating a separate test model just to skip a test on Oracle.

It means that USER_ID column doesn't exist on this model 🤔 I can check later DDLs that we execute.

Also, I wouldn't mix this with composite PK tests but add them to the tests/foreign_object/.

jacobtylerwalls

Appreciate the help with this one @JaeHyuckSa! I have a few comments.

tests/foreign_object/models/customers.py

tests/foreign_object/models/__init__.py

tests/foreign_object/models/customers.py

tests/foreign_object/tests.py

django/db/models/base.py

tests/foreign_object/tests.py

felixxm · 2025-09-02T20:08:19Z

I can check later DDLs that we execute.

The current version works for me on Oracle 🤷

jacobtylerwalls · 2025-09-02T20:41:41Z

Thanks for checking. Might be a multi-column-only DDL issue then 😉 .

jacobtylerwalls · 2025-09-08T13:52:09Z

Hi @JaeHyuckSa 👋

Thanks for pushing this forward. This is a release blocker for 6.0, so we need to clear it by the middle of next week. Do you anticipate having time to continue it this week? Thanks.

JaeHyuckSa · 2025-09-08T16:02:08Z

Hi @JaeHyuckSa 👋

Thanks for pushing this forward. This is a release blocker for 6.0, so we need to clear it by the middle of next week. Do you anticipate having time to continue it this week? Thanks.

Hi @jacobtylerwalls Jacob!
I’ve been swamped with a lot of tasks all of a sudden, so I couldn’t get to this earlier. I’m planning to address your review comments on this PR either by tomorrow at the earliest, or within the next two days at the latest. Thanks for the reminder.

JaeHyuckSa · 2025-09-15T15:37:31Z

@jacobtylerwalls Hi Jacob

Sorry for the delay in addressing this; I wasn’t able to keep up with the agreed timeline as a lot of work suddenly came in. I’ve applied the simpler review comments, and regarding the requested test code, would this level of coverage be appropriate? Currently, the tests are failing for another reason: as you mentioned, a performance regression is occurring because both fields are being checked, which triggers an error. I think we’ll need to resolve this by making use of field.local_related_fields.

jacobtylerwalls · 2025-09-15T18:05:27Z

Thanks for the update. I'll pick it up from here later this afternoon.

JaeHyuckSa · 2025-09-15T18:25:56Z

Thanks for the update. I'll pick it up from here later this afternoon.

@jacobtylerwalls Thank you for reviewing. I've applied the changes you suggested above. :)

django/db/models/base.py

jacobtylerwalls · 2025-09-15T21:54:20Z

@charettes I took a stab at a simpler approach, how does it look?

charettes · 2025-09-15T22:12:18Z

django/db/models/base.py

+            else:
+                value = getattr(self, field.attname)


Should we continue if the field is not concrete?

Suggested change

else:

value = getattr(self, field.attname)

elif field.concrete:

value = getattr(self, field.attname)

else:

continue

Do you have an idea for a test that would cover that case?

I think you'd have to test _get_field_expression_map directly for it as we don't have a lot of non-concrete in core.

The only two I can think about are GenericForeignKey and soon ManyToManyField after #19602 lands. The idea is that for these fields getattr(self, field.attname) could return a manager which is not suitable to be passed as the right-hand-side in a lookup.

Got it. In that case let's add coverage via ManyToManyField on #19602. I'll leave a note.

tests/foreign_object/tests.py

charettes · 2025-09-15T22:14:35Z

django/db/models/base.py

+                value = tuple(
+                    getattr(self, from_field) for from_field in field.from_fields
+                )
+                if len(value) == 1:
+                    value = value[0]


This highlights were still missing proper handling for non-pk composite fields.

Exactly -- I'll open a follow-up issue.

Tried to capture this in ticket-36611, but if you meant something else, let me know! 🙏

…aint validation.

… a ForeignObject. Follow-up to e44e832. Refs #36222.

jacobtylerwalls · 2025-09-16T14:25:31Z

Thanks @JaeHyuckSa ⭐

JaeHyuckSa force-pushed the issue-36580 branch 2 times, most recently from 9f3971a to efb8415 Compare August 30, 2025 11:18

jacobtylerwalls reviewed Aug 30, 2025

View reviewed changes

docs/releases/5.2.6.txt Outdated Show resolved Hide resolved

JaeHyuckSa force-pushed the issue-36580 branch from efb8415 to e6f16d2 Compare August 30, 2025 11:25

JaeHyuckSa changed the title ~~Fixed #36580 -- Fixed constraint validation with Q() referencing ForeignObject.~~ Fixed #36580 -- Fixed constraint validation crash when condition uses a ForeignObject. Aug 30, 2025

jacobtylerwalls closed this Aug 30, 2025

jacobtylerwalls reopened this Aug 30, 2025

jacobtylerwalls reviewed Sep 2, 2025

View reviewed changes

tests/composite_pk/models/tenant.py Outdated Show resolved Hide resolved

tests/composite_pk/test_models.py Show resolved Hide resolved

JaeHyuckSa force-pushed the issue-36580 branch 2 times, most recently from 50af1d7 to 9776e8f Compare September 2, 2025 17:43

jacobtylerwalls reviewed Sep 2, 2025

View reviewed changes

jacobtylerwalls self-assigned this Sep 14, 2025

JaeHyuckSa force-pushed the issue-36580 branch 2 times, most recently from a560e34 to 64e4c91 Compare September 15, 2025 15:34

JaeHyuckSa force-pushed the issue-36580 branch from 64e4c91 to e103551 Compare September 15, 2025 16:50

JaeHyuckSa force-pushed the issue-36580 branch from e103551 to 8a58630 Compare September 15, 2025 18:23

JaeHyuckSa force-pushed the issue-36580 branch from 8a58630 to 26e7487 Compare September 15, 2025 18:30

charettes reviewed Sep 15, 2025

View reviewed changes

django/db/models/base.py Outdated Show resolved Hide resolved

charettes reviewed Sep 15, 2025

View reviewed changes

django/db/models/base.py Outdated Show resolved Hide resolved

jacobtylerwalls force-pushed the issue-36580 branch from 26e7487 to e768d91 Compare September 15, 2025 21:49

charettes reviewed Sep 15, 2025

View reviewed changes

jacobtylerwalls mentioned this pull request Sep 15, 2025

Fixed #35453 -- ManyToManyField no longer flagged as a concrete field. #19602

Merged

6 tasks

JaeHyuckSa added 2 commits September 15, 2025 20:00

Refs #36580 -- Added coverage for excluding ForeignObject from constr…

23ac37e

…aint validation.

Fixed #36580 -- Fixed constraint validation crash when condition uses…

0340c45

… a ForeignObject. Follow-up to e44e832. Refs #36222.

jacobtylerwalls force-pushed the issue-36580 branch from e768d91 to 0340c45 Compare September 16, 2025 00:06

jacobtylerwalls merged commit 8c621e9 into django:main Sep 16, 2025
42 of 44 checks passed

Uh oh!

Fixed #36580 -- Fixed constraint validation crash when condition uses a ForeignObject. #19798

Fixed #36580 -- Fixed constraint validation crash when condition uses a ForeignObject. #19798

Conversation

JaeHyuckSa commented Aug 30, 2025

Trac ticket number

Checklist

Uh oh!

Uh oh!

jacobtylerwalls commented Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobtylerwalls commented Aug 30, 2025

Uh oh!

jacobtylerwalls commented Sep 2, 2025

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixxm commented Sep 2, 2025

Uh oh!

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixxm commented Sep 2, 2025

Uh oh!

jacobtylerwalls commented Sep 2, 2025

Uh oh!

jacobtylerwalls commented Sep 8, 2025

Uh oh!

JaeHyuckSa commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaeHyuckSa commented Sep 15, 2025

Uh oh!

jacobtylerwalls commented Sep 15, 2025

Uh oh!

JaeHyuckSa commented Sep 15, 2025

Uh oh!

Uh oh!

Uh oh!

jacobtylerwalls commented Sep 15, 2025

Uh oh!

charettes Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

charettes Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

charettes Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

jacobtylerwalls Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobtylerwalls commented Sep 16, 2025

Fixed #36580 -- Fixed constraint validation crash when condition uses a `ForeignObject`. #19798

Fixed #36580 -- Fixed constraint validation crash when condition uses a `ForeignObject`. #19798

jacobtylerwalls commented Aug 30, 2025 •

edited

Loading

JaeHyuckSa commented Sep 8, 2025 •

edited

Loading

charettes Sep 15, 2025 •

edited

Loading