Fixed #31300 -- Added GeneratedField model field. #16860

LilyFoote · 2023-05-15T22:30:54Z

Builds on top of #16417.

LilyFoote · 2023-05-15T22:32:17Z

@jpnauta Here's the first draft of Oracle support.

django/db/backends/base/features.py

django/db/models/fields/generated.py

tests/migrations/test_operations.py

tests/model_fields/models.py

LilyFoote · 2023-05-16T16:26:49Z

I'm not sure why the jenkins builds aren't running properly. @felixxm any ideas?

sarahboyce · 2023-05-19T06:39:02Z

I'm not sure why the jenkins builds aren't running properly. @felixxm any ideas?

This might have coincided with the degraded performance/incident of GitHub actions on the 16th May (see here). Push up a small change (can reword a commit) and see if this fixes it? I can't see this PR in the list of stuff picked up by jenkins

Lancelot03 · 2023-05-20T17:08:01Z

I'm new here. I think by pushing a small change and monitoring Jenkins, you can assess if the issue persists or if the new commit triggers the build successfully. This can help determine if the previous problem was due to the degraded performance of GitHub Actions or if there might be other underlying issues with Jenkins configuration or integration.
This might can help.

LilyFoote · 2023-05-20T20:13:51Z

That doesn't appear to have fixed things.

sarahboyce · 2023-05-21T10:07:32Z

Found an error log: https://djangoci.com/view/PR%20Builders/job/pr-mariadb/lastFailedBuild/console
(don't understand what is needed to be done 🤔)

felixxm · 2023-05-26T08:18:42Z

I'd start by fixing conflicts and rebasing from the main branch.

LilyFoote · 2023-05-26T10:31:32Z

I'd start by fixing conflicts and rebasing from the main branch.

I was hoping to avoid that because I didn't want to rebase @jpnauta's commits so this PR could cleanly land back on his.

LilyFoote · 2023-06-19T17:23:48Z

@felixxm At DjangoCon Europe sprints we discussed the Oracle error we're getting here and I think we settled on updating the Jenkins install to set MAX_STRING_SIZE=EXTENDED. Do I need to open a ticket somewhere for that or is leaving this comment enough?

django/db/backends/base/features.py

LilyFoote · 2023-08-26T17:20:29Z

@felixxm I've removed the extra feature flag. The main blocker I think is #16860 (comment), followed by fixing the merge conflicts.

felixxm

@felixxm I've removed the extra feature flag. The main blocker I think is #16860 (comment), followed by fixing the merge conflicts.

Tests should pass when a database doesn't support collations. I've added comments with required feature flags checks.

django/db/backends/sqlite3/features.py

tests/model_fields/models.py

tests/model_fields/test_generatedfield.py

tests/model_fields/models.py

tests/model_fields/test_generatedfield.py

tests/invalid_models_tests/test_ordinary_fields.py

django/db/models/fields/generated.py

LilyFoote · 2023-09-05T12:24:36Z

Can we make the error nicer for now? This feels like something we can try to find a solution for in Django 5.1 to me.

Does explicitly registering the lookup with GeneratedField resolve the problem? In that case we might be able to document that as a workaround for now.

felixxm · 2023-09-05T12:34:37Z

Can we make the error nicer for now?

IMO, no, I don't know how we would detect that.

Does explicitly registering the lookup with GeneratedField resolve the problem?

Not really as registering all class-related lookups on GeneratedField will create many conflicts 😞 Since Django 4.2 we can register lookups on field instances. We could (theoretically) register lookups from the output_field class in GeneratedField.contribute_to_class(), however this will force users to always define GeneratedField.output_field and this is just a theory 😅 .

felixxm · 2023-09-05T12:41:46Z

Can we make the error nicer for now?

IMO, no, I don't know how we would detect that.

Does explicitly registering the lookup with GeneratedField resolve the problem?

Not really as registering all class-related lookups on GeneratedField will create many conflicts 😞 Since Django 4.2 we can register lookups on field instances. We could (theoretically) register lookups from the output_field class in GeneratedField.contribute_to_class(), however ~~this will force users to always define GeneratedField.output_field and~~ this is just a theory 😅 .

felixxm · 2023-09-05T12:55:14Z

@pauloxnet Can you check the following patch?

diff --git a/django/db/models/fields/generated.py b/django/db/models/fields/generated.py
index 56a2854d3c..8bf6969f9d 100644
--- a/django/db/models/fields/generated.py
+++ b/django/db/models/fields/generated.py
@@ -44,6 +44,8 @@ class GeneratedField(Field):
             if self._output_field is not None
             else self._resolved_expression.output_field
         )
+        for lookup_name, lookup in self.output_field.get_class_lookups().items():
+            self.register_lookup(lookup, lookup_name=lookup_name)
 
     def generated_sql(self, connection):
         return self._resolved_expression.as_sql(

pauloxnet · 2023-09-05T13:03:40Z

@pauloxnet Can you check the following patch?

I'll try it ASAP

pauloxnet · 2023-09-05T13:12:02Z

@pauloxnet Can you check the following patch?

diff --git a/django/db/models/fields/generated.py b/django/db/models/fields/generated.py
index 56a2854d3c..8bf6969f9d 100644
--- a/django/db/models/fields/generated.py
+++ b/django/db/models/fields/generated.py
@@ -44,6 +44,8 @@ class GeneratedField(Field):
             if self._output_field is not None
             else self._resolved_expression.output_field
         )
+        for lookup_name, lookup in self.output_field.get_class_lookups().items():
+            self.register_lookup(lookup, lookup_name=lookup_name)
 
     def generated_sql(self, connection):
         return self._resolved_expression.as_sql(

It worked 👍🏻

>>> from geometricfigures.models import Document
>>> from django.contrib.postgres import search
>>> str(Document.objects.filter(search_vector=search.SearchQuery("deadline", config="english")).only("id").query)
'SELECT "geometricfigures_document"."id" FROM "geometricfigures_document" WHERE "geometricfigures_document"."search_vector" @@ (plainto_tsquery(english::regconfig, deadline))'
>>> str(Document.objects.filter(vector=search.SearchQuery("deadline", config="english")).only("id").query)
'SELECT "geometricfigures_document"."id" FROM "geometricfigures_document" WHERE "geometricfigures_document"."vector" @@ (plainto_tsquery(english::regconfig, deadline))'
>>> Document.objects.filter(vector=search.SearchQuery("deadline", config="english")).only("id")
<QuerySet [<Document: Document object (2)>]>
>>> Document.objects.filter(search_vector=search.SearchQuery("deadline", config="english")).only("id")
<QuerySet [<Document: Document object (2)>]>

felixxm · 2023-09-05T13:17:49Z

It worked 👍🏻

Thanks for checking 👍

docs/ref/models/fields.txt

pauloxnet · 2023-09-06T10:40:05Z

I did further tests locally only finding problems for non-immutable functions in PostgreSQL. Apart from the documentation suggestions I've already sent, I have nothing more to add. Great job to all contributors, I think this is another great feature added to Django.

felixxm · 2023-09-06T12:15:44Z

We're almost there 😪 only docs left 🚀 I'll continue later today/tomorrow.

felixxm · 2023-09-06T19:34:50Z

Squashed commits and pushed edits to release notes.

Thanks Adam Johnson and Paolo Melchiorre for reviews. Co-Authored-By: Lily Foote <code@lilyf.org> Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@gmail.com>

felixxm · 2023-09-07T08:07:09Z

@jpnauta Thanks 👍 Welcome aboard 👍

@LilyFoote Thanks for pushing this to the finish line 🥇

LilyFoote · 2023-09-07T09:49:17Z

docs/ref/models/fields.txt

+    Since the database always computed the value, the object must be reloaded
+    to access the new value after :meth:`~Model.save()`, for example, by using
+    :meth:`~Model.refresh_from_db()`.


Is this always true? I'm wondering if for databases supporting RETURNING we can/do update the instance to make a refresh unnecessary?

Theoretically, we could use UPDATE ... RETURNING but let's leave some new features/cleanups for Django 5.1 😉

jpnauta · 2023-09-11T14:33:50Z

Thanks @LilyFoote @felixxm for pushing this through the finish line! It was a pleasure collaborating with you both 🥳

LilyFoote mentioned this pull request May 15, 2023

Refs #31300 -- Add function-based virtual fields #16417

Closed

LilyFoote commented May 15, 2023

View reviewed changes

LilyFoote force-pushed the ticket_31300 branch 2 times, most recently from f540194 to 8b0c9fa Compare May 26, 2023 20:37

LilyFoote force-pushed the ticket_31300 branch from 8b0c9fa to aa58318 Compare June 19, 2023 16:05

LilyFoote force-pushed the ticket_31300 branch from 1f001cc to 28644a9 Compare July 8, 2023 19:32

LilyFoote changed the title ~~Add Oracle support for generated fields.~~ Refs #31300 -- Add support for function-based virtual fields on Oracle Jul 27, 2023

felixxm reviewed Aug 25, 2023

View reviewed changes

django/db/backends/base/features.py Show resolved Hide resolved