Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #35127 -- Made Model.full_clean() ignore GeneratedFields. #17758

Merged
merged 1 commit into from Jan 19, 2024

Conversation

felixxm
Copy link
Member

@felixxm felixxm commented Jan 19, 2024

Thanks Claude Paroz for the report.

Regression in f333e35.

ticket-35127

Thanks Claude Paroz for the report.

Regression in f333e35.
Copy link
Contributor

@shangxiao shangxiao left a comment

Choose a reason for hiding this comment

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

Nice quick fix ⭐

We've seen this pop up once before, I was almost going to suggest defining something alongside fields in a similar spirit to concrete_fields though not sure it's worth it.

Are there any more instances of accessing fields that we should check or have you checked already?

Copy link
Member

@claudep claudep left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@felixxm
Copy link
Member Author

felixxm commented Jan 19, 2024

Thanks both for checking 👍

We've seen this pop up once before, I was almost going to suggest defining something alongside fields in a similar spirit to concrete_fields though not sure it's worth it.

Yes, there are a few places, but I don't think it's worth adding concrete_non_generated_fields. Maybe later on the main as a cleanup.

Are there any more instances of accessing fields that we should check or have you checked already?

I'll check other places, but even If I find something, it will be a separate bug.

@felixxm felixxm merged commit 4879907 into django:main Jan 19, 2024
36 checks passed
@felixxm felixxm deleted the issue-35127 branch January 19, 2024 07:57
@charettes
Copy link
Member

charettes commented Jan 19, 2024

I wonder if this is a missed opportunity to branch of field.editable instead.

Per the docs these should be skipped during model validation. Is it not the case?

@felixxm
Copy link
Member Author

felixxm commented Jan 19, 2024

I wonder if this is a missed opportunity to branch of field.editable instead.

Per the docs these should be skipped during model validation. Is it not the case?

I don't think that would work 🤔, because AttributeError is raised when getting a value for deferred generated fields and clean() will still try to do this even when we early return from validate().

@charettes
Copy link
Member

@felixxm what I meant is something like that

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 61925f63ea..ec0e975837 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -1628,7 +1628,7 @@ def clean_fields(self, exclude=None):

         errors = {}
         for f in self._meta.fields:
-            if f.name in exclude or f.generated:
+            if f.name in exclude or not f.editable:
                 continue
             # Skip validation for empty fields with blank=True. The developer
             # is responsible for making sure they have a valid value.

which seems more correct based on the Field.editable documentation

They are also skipped during model validation.

And happens to resolve this issue as GeneratedField.editable must be set to False.

FWIW the full test suite passes on SQLite except for model_fields.test_binaryfield.BinaryFieldTests.test_max_length which seems to be violating the documentation.

@felixxm
Copy link
Member Author

felixxm commented Jan 20, 2024

@felixxm what I meant is something like that

Right 👍 We can change this on the main branch (as a separate cleanup) since it breaks some tests.

@charettes
Copy link
Member

Agreed likely safer, I'll create a ticket about it and target main. Thanks for the discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants