Fixed #21454 -- Added readonly attribute to Model Fields #7515

ewjoachim · 2016-11-06T10:05:35Z

DutH 2016 sprints : Pair coded with @wengole.
Made SQLCompiler remove readonly fields from INSERT and UPDATE queries.
Made model instances refresh themselves after save when appropriate.

https://groups.google.com/forum/#!topic/django-developers/BDAlTyJwQeY

adamchainz · 2016-11-06T10:19:03Z

tests/model_fields/tests.py

+        with CaptureQueriesContext(connection=connection) as capture:
+            inst.save()
+
+        query = capture.captured_queries[0]


no assertion on the number of queries?

I'm not sur one is needed here. It's present in the other tests when we need tu make sur we're checking the right query. That being said, I can add a check if you think it's better with than without.

adamchainz · 2016-11-06T10:20:46Z

tests/model_fields/models.py

@@ -387,3 +387,26 @@ class UUIDChild(PrimaryKeyUUIDModel):

 class UUIDGrandchild(UUIDChild):
    pass
+
+###############################################################################
+# This model is used for Readonly test cases


I feel like this comment is redundant given the model names. None of the other models in the file have such comments 😄

adamchainz · 2016-11-06T10:21:21Z

tests/model_fields/tests.py

+
+    @classmethod
+    def populate_database_fields(cls):
+        connection.cursor().execute(


a single multiline string would be clearer?

adamchainz · 2016-11-06T10:21:34Z

tests/model_fields/tests.py

+    def populate_database_fields(cls):
+        connection.cursor().execute(
+            '''ALTER TABLE "model_fields_modelwithreadonlyfield" '''
+            '''ALTER COLUMN "readonly_int1" SET DEFAULT 1 ''')


I'm not so comfortable with this given that Django should soon have database level defaults that work across databases. It should also be not much work to make this SQL compatible with MySQL and sqlite too, and put it in a migration

SQLite cannot add a default after the table is created (to my knowledge).

Mysql is clearly something we forgot ^^

adamchainz · 2016-11-06T10:21:56Z

tests/model_fields/tests.py

+class PostgreSQLReadonlyTests(ReadonlyTestsMixin, TestCase):
+
+    @classmethod
+    def populate_database_fields(cls):


wouldn't these be neater as a migration file?

I was wondering, but there's no migrations in this app yet, and I feel it's easier to understand if the code is nearby. But I could completely do it with a migration, yes.

(Except I'd have to manage different DB in the migrations file...)

adamchainz · 2016-11-06T10:25:13Z

django/db/models/base.py

@@ -834,6 +834,13 @@ def save_base(self, raw=False, force_insert=False,
        # Once saved, this is no longer a to-be-added instance.
        self._state.adding = False

+        # Readonly fields are computed by the Database. If there are any,
+        # we should fetch their value.
+        if self._meta.has_readonly_fields:


I think it would be good if there was a way to disable this extra query, e.g. in an argument to save. For example, if a model has a generated/computed field used purely for indexing on the database level, there's no need to fetch that field back upon save()

A whole new argument to save looks a little overkill to me. The query is a simple one that should not cost much...

Plus, at some point in the future, we might use RETURNING so there won't be any additional query for supported backends...

adamchainz

what about testing the cases where I try to save/update only readonly fields? I feel like Django should throw some kind of error there, as it's a nonsensical operation. That said, save() is a no-op if update_fields is an empty list.

adamchainz · 2016-11-06T10:27:47Z

django/db/models/fields/__init__.py

@@ -703,6 +704,9 @@ def contribute_to_class(self, cls, name, private_only=False, virtual_only=NOT_PR
            setattr(cls, 'get_%s_display' % self.name,
                    curry(cls._get_FIELD_display, field=self))

+        if self.readonly:
+            cls._meta.has_readonly_fields = True


I think this would be better as a cached_property on Options that iterates all the fields and returns True if any are readonly - it would be a bit less surprising to me, as then it's defined entirely within the Options class

Agreed, this is exactly what I proposed 4 minutes after you comment 😄. This code can be removed now as it seems unused.

Oh, yes, sorry !

charettes · 2016-11-06T10:32:12Z

django/db/models/fields/__init__.py

@@ -703,6 +704,9 @@ def contribute_to_class(self, cls, name, private_only=False, virtual_only=NOT_PR
            setattr(cls, 'get_%s_display' % self.name,
                    curry(cls._get_FIELD_display, field=self))

+        if self.readonly:
+            cls._meta.has_readonly_fields = True
+


It might be worth having a cached readonly_fields property instead.

charettes · 2016-11-06T10:34:58Z

django/db/models/options.py

@@ -115,6 +115,7 @@ def __init__(self, meta, app_label=None):
        self.pk = None
        self.has_auto_field = False
        self.auto_field = None
+        self.has_readonly_fields = False


Having a @cached_property for read-only fields would be more appropriate in this case. You'd just have to make sure you add 'readonly_fields' to FORWARD_PROPERTIES to make sure the cache is busted on field addition through add_field().

charettes · 2016-11-06T10:36:50Z

Makes me wonder if we'd like to name these fields writable=False instead of using the readonly terminology.

ewjoachim · 2016-11-06T10:50:26Z

So quick update / todo list :

Add mysql checks
Add oracle checks (we might need calling buildbot a few times for this)
~~make sure we agree on the terminology (what's the process ? Should I ask on the mailing list ? The ticket ?)~~ > Let's agree to change it later if it needs to be changed.
use a cached_property and add the appropriate tests, I guess ?
decide on whether we want to use migrations or not, and do the appropriate work... > We're not using migrations
write documentation

@wengole

DutH 2016 sprints : Pair coded with @wengole. Made SQLCompiler remove readonly fields from INSERT and UPDATE queries. Made model instances refresh themselves after save when appropriate.

@wengole

DutH 2016 sprints : Pair coded with @wengole. Made SQLCompiler remove readonly fields from INSERT and UPDATE queries. Made model instances refresh themselves after save when appropriate.

ewjoachim · 2016-11-06T15:39:05Z

@charette, @adamchainz : we've made corrections with @wengole, we haven't squashed the commits yet so that you can have a look at the corrections, let us know when we can squash.

And thank you everyone for those wonderful sprints :)

ewjoachim · 2016-11-07T09:47:24Z

BTW, @wengole added himself to the AUTHORS in this PR. I did in #7474 (and I guess it will be merged before this PR)

adamchainz

Idea: shouldn't AutoField and BigAutoField technically now be readonly?

adamchainz · 2016-11-07T11:10:29Z

django/db/models/base.py

+        # Readonly fields are computed by the Database. If there are any,
+        # we should fetch their value. (refresh_from_db does nothing if the
+        # field list is empty)
+        self.refresh_from_db(fields=[field.name for field in self._meta.readonly_fields])


I've thought about this more and think this should be disabled by default (or even removed, users can manually call refresh_from_db after save). I think most use cases for readonly fields, python code will not require the new value from the database immediately. Most Django workflows typically take user data and update the database, and return - not requiring the new data from the database. In addition, virtual columns will be a key use case for readonly fields, but they are mostly added just for allowing more selectivity on queries. By adding an extra query to every single save() on any model with a readonly field, we could be introducing a major performance regression for some users, for the off chance use case that after save()ing a model, the readonly columns' values are required. Even if it is kept, it should be flagged, so that users can avoid the performance hit.

I'm not sure it counts as a performance regression : it would only affect users who have defined readonly fields (except if you're talking about the cost of the sole function call to refresh_from_db that returns immediately, but I guess not). But I get your point. And even if the select should not be costly, you're right in that it's not our call to make (it might be a minor point for some and a major point for others).

In this case, we could at least provide a refresh_readonly_fields method (or something like this). Note : this could be interesting independently, because readonly fields (or computed fields) have a higher chance to see their value change independently from what happens on the Python side.

I'm thinking this be an important design decisions here, that could be brought to the dev mailing list.

An alternative approach would be to return a lazy object for that field which runs the query when accessed.

Hmm, a whole object seems overkill because it would mean one does not get the same kind of object when calling create or save() in a MySQL and PGSQL environment, but maybe we could just mark those fields as deferred, so there would be a query when one tries to access the fields (maybe with a way to load them all at once ?)

k4nar · 2016-11-07T11:41:14Z

django/db/models/sql/compiler.py

-        has_fields = bool(self.query.fields)
-        fields = self.query.fields if has_fields else [opts.pk]
+        # Filter out readonly fields
+        fields = [field for field in self.query.fields if not field.readonly]


Is there any case where self.query.fields can be None? The previous code was preventing from iterating over it in this case.

I think you're right.

k4nar · 2016-11-07T11:42:25Z

django/db/models/sql/compiler.py

+
+        writable_values = [
+            (field, model, val)
+            for field, model, val in self.query.values


Same remark about an eventual None here.

(same as above, you're right)

ewjoachim · 2016-11-07T12:11:20Z

@adamchainz

Idea: shouldn't AutoField and BigAutoField technically now be readonly?

AutoField and BigAutoField can be written by Django. You can decide you want to set the value of the AutoField to 42. But for readonly fields, the only way to bypass it is to write your own SQL. These are close but not identical concept and while one can inspire the other, they're both different.

I was also thinking about the "readonly" vs "writable" debate : readonly has the advantage of being defined as a positive attribute (the default is False and the non-default is True). The equivalent naming of Model._meta.readonly_fields using "writable" would be Model._meta.non_writable_fields or unwritable_fields which, to me, feels unnatural.

ewjoachim · 2016-11-08T15:46:35Z

I've raise the different issues on the Mailing list and for now, I'm enclined to follow @mjtamlyn 's suggestion of creating a DEP to make sure our solution fit all the common usecases.

This means that it's likely that this PR will not be merged as is, even if it can be an inspiration for a future attempt. I'm personnally convinced that the readonly part is good and when the dust settles, and if the plan says so, I may resubmit this branch with just the readonly part.

@wengole , this is your code too. Do you agree ?

charettes · 2016-11-10T14:27:30Z

I think using deferred would makes sense here, that's exactly what these
fields are.

It wouldn't be too hard for a user requiring direct access to these fields
on a backend not supporting RETURNING to issue
refresh_from_db('readony_field'). We already deal with such backend
differently in regard to primary key assignment in bulk_create.

2016-11-10 7:55 GMT-05:00 Joachim Jablon notifications@github.com:

@ewjoachim commented on this pull request.

In django/db/models/base.py #7515:

@@ -834,6 +834,11 @@ def save_base(self, raw=False, force_insert=False,
# Once saved, this is no longer a to-be-added instance.
self._state.adding = False
   # Readonly fields are computed by the Database. If there are any,
   # we should fetch their value. (refresh_from_db does nothing if the
   # field list is empty)
   self.refresh_from_db(fields=[field.name for field in self._meta.readonly_fields])
Hmm, a whole object seems overkill because it would mean one does not get
the same kind of object when calling create in a MySQL and PGSQL
environment, but maybe we could just mark those fields as deferred, so
there would be a query when one tries to access the fields (maybe with a
way to load them all at once ?)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#7515, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAkTUHEuHdwSfUxTnj2vJ3UEgxVnLlEks5q8xQlgaJpZM4Kqg0I
.

ewjoachim · 2016-11-10T15:44:31Z

( @charettes was replying to #7515 (comment) but of course, GitHub doesn't show this properly)

codingjoe · 2016-11-14T14:17:00Z

@ewjoachim thanks, I can review the DEP but I won't have the time to write anything myself. I have already another big feature pending for the next LTS. It eats up all the time I can spend on Django right now.

ewjoachim · 2016-11-16T08:29:24Z

I'll reopen a PR when it's ready.

adamchainz reviewed Nov 6, 2016

View reviewed changes

charettes changed the title ~~Fixes #21454 -- Added readonly attribute to Model Fields~~ Fixed #21454 -- Added readonly attribute to Model Fields Nov 6, 2016

adamchainz reviewed Nov 6, 2016

View reviewed changes

charettes reviewed Nov 6, 2016

View reviewed changes

ewjoachim and others added 5 commits November 6, 2016 12:26

Fixes #21454 -- Added readonly attribute to Model Fields

8b69597

DutH 2016 sprints : Pair coded with @wengole. Made SQLCompiler remove readonly fields from INSERT and UPDATE queries. Made model instances refresh themselves after save when appropriate.

Code review remarks

38a1154

Second wave of code review fixes

be37758

Fixes #21454 -- Added readonly attribute to Model Fields

e868a37

DutH 2016 sprints : Pair coded with @wengole. Made SQLCompiler remove readonly fields from INSERT and UPDATE queries. Made model instances refresh themselves after save when appropriate.

Added readonly field option documentation

56482b7

Added Ben Cole to AUTHORS

d98dac1

wengole mentioned this pull request Nov 7, 2016

Fixed #27452 -- Added SerialField and BigSerialField to contrib.postgres. #7525

Closed

adamchainz reviewed Nov 7, 2016

View reviewed changes

k4nar reviewed Nov 7, 2016

View reviewed changes

ewjoachim closed this Nov 16, 2016

ewjoachim mentioned this pull request Nov 16, 2016

Readonly and Auto-Refresh Fields django/deps#36

Closed

charettes mentioned this pull request Nov 17, 2016

Fixed #27222 -- Refreshed fields that are expressions after Model.save(). #7244

Closed

Fixed #21454 -- Added readonly attribute to Model Fields #7515

Fixed #21454 -- Added readonly attribute to Model Fields #7515

Conversation

ewjoachim commented Nov 6, 2016 • edited by timgraham Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewjoachim Nov 6, 2016 • edited Loading

Choose a reason for hiding this comment

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charettes commented Nov 6, 2016

ewjoachim commented Nov 6, 2016 • edited Loading

ewjoachim commented Nov 6, 2016

ewjoachim commented Nov 7, 2016

adamchainz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewjoachim Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewjoachim Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewjoachim commented Nov 7, 2016

ewjoachim commented Nov 8, 2016 • edited Loading

charettes commented Nov 10, 2016

@ewjoachim commented on this pull request.

ewjoachim commented Nov 10, 2016

codingjoe commented Nov 14, 2016 • edited Loading

ewjoachim commented Nov 16, 2016

ewjoachim commented Nov 6, 2016 •

edited by timgraham

Loading

ewjoachim Nov 6, 2016 •

edited

Loading

ewjoachim commented Nov 6, 2016 •

edited

Loading

ewjoachim Nov 7, 2016 •

edited

Loading

ewjoachim Nov 10, 2016 •

edited

Loading

ewjoachim commented Nov 8, 2016 •

edited

Loading

codingjoe commented Nov 14, 2016 •

edited

Loading