Better compat with Model and Django Migrations #31

jleclanche · 2022-12-14T04:31:50Z

This branch is an experiment, and it's currently broken.

I mentioned this in #8:

My theory is that a composite type is similar to a model; so, it should be possible to make Django think that it is a Model, and hack into Django so that it generates the types and selects them correctly.

Success 1: By subclassing CreateModel migration operation into a CreateType operation, I've been able to create composite types without issues. However, the migration generated by makemigrations needs hand editing as there is no way to tell it to use that particular operation instead of the CreateModel operation, which is hardcoded. This is easily fixed in Django.

Success 2: I've renamed db_type to db_table, making the composite type compatible with Model. This has so far removed quite a bit of hacky code.

Fail 1: Models absolutely require a primary key in Django. I have however worked around this by generating a dummy __id column, which I ignore during field generation. I believe this will be a huge blocker for this approach if the code is upstreamed into Django itself.

So this is where I'm at. The two main changes are db_type is now db_table (and this is still required; breaking change), and _meta.fields is now the same structure as for normal fields, which is a list of field instances; their name comes from field.name.

I've had to use a custom manager that always returns empty querysets, as the tests surfaced that some django code (such as the db serialization code) will query all models. Types should always be empty for this.

This doesn't solve #8 because something like AlterType hasn't been implemented, but it makes it a lot easier to tackle.

Also, because I replaced the migration operation with one that doesn't rely on the class itself, I have had to drop support for the composite_type_created signal. IMO this is not a useful signal anyway - any push back?

Currently, I'm fighting with the following test error:

>       self.assertIn(expected_deconstruction, text)
E       AssertionError: 'base_field=tests.models.Card.Field()' not found in 'django.contrib.postgres.fields.ArrayField(base_field=postgres_composite_types.composite_type.Card.Field(), size=None)'

I'm still investigating what's happening here.

jleclanche · 2022-12-14T06:43:35Z

@danni I've fixed all test errors.

The following issues remain in makemigrations:

Migrations generate invalid imports. This is not fixable in current Django due to the migration autodetector not having a concept of non-top-level attributes of a module.
Migrations generate CreateModel instead of CreateType instruction. This is not fixable in current Django, but the CreateModel can be swapped out 1:1 with a CreateType
Migrations include an _id_not_used dummy field upon generation. This can safely be removed or left in, it will be ignored regardless.
Migration options include base_manager_name and default_manager_name. This is fixable, but harmless, and I've spent enough time on getting this hack to work :P

This makes the type more compatible with the Model class

This does not need a db connection, nor does it need migrations to already have run. So we can do this early.

Double underscore causes errors in lookups

danni

This is very clever, but I keep wondering about the approach. Perhaps before merging this it would be worth raising a ticket in Django to see if they're interested in the feature at all, and if they are, how they would approach making migrations work.

If we do go forward with this approach we'll need to test what it's like to move existing code onto this approach, i.e. will we make a huge mess of people's migration graphs.

danni · 2023-01-02T22:15:03Z

postgres_composite_types/fields.py

+    def deconstruct(self):
+        name, path, args, kwargs = super().deconstruct()
+
+        name = path.replace("postgres_composite_types.composite_type.", "")


This feels a bit fragile. Also the shadowing makes it hard to appreciate what will happen.

I agree. I am not sure how else to do it though. Postgres' migration generation system is poor in terms of customizability.

danni · 2023-01-02T22:15:35Z

postgres_composite_types/fields.py

+            {
+                field.name: field.value_to_string(value)
+                for field in self._composite_type_model._meta.fields
+                if field.name != DummyField.name


isinstance ?

danni · 2023-01-02T22:15:55Z

postgres_composite_types/forms.py

+        fields = {
+            field.name: field.formfield()
+            for field in fields or model._meta.fields
+            if field.name != DummyField.name


isinstance ?

I don't remember now, but IIRC that didn't work because of the way Django does all this.

danni · 2023-01-02T22:19:26Z

postgres_composite_types/operations.py

-            sql_create_type(self.Meta.db_type, self.Meta.fields, schema_editor)
-        )
-        self.Meta.model.register_composite(schema_editor.connection)
-        composite_type_created.send(


Just because?

I culdn't get this to work at all. I am not sure what benefit it actually brings to the table, though.

jleclanche force-pushed the dev/compat-with-model branch 3 times, most recently from 5a74a4e to f227d54 Compare December 14, 2022 06:40

jleclanche changed the title ~~WIP: Better compat with Model and Django Migrations~~ Better compat with Model and Django Migrations Dec 14, 2022

jleclanche added 16 commits December 14, 2022 07:44

Use attribute db_table instead of db_type

7f86f5c

This makes the type more compatible with the Model class

Fix bug with fields on type that don't have associated widget

e1c1c40

Tweak field name to generate valid migrations

b1e3af7

Make CompositeType compatible with Model

b6df537

Replace migration operation with one based on CreateModel

cf3a6c7

Add a dummy primary key to each composite field (user-invisible)

9663050

Regenerate test migrations

c53d147

Use an EmptyManager for composite types

ea88858

Register the adapter once, before registering the composite

3b87d8c

This does not need a db connection, nor does it need migrations to already have run. So we can do this early.

Register post_migrate signal as well

06d5499

Fix Field without requiring a Meta attribute to be set on it

69d4923

Replace Caster attribute with a dynamically-created class

44b43d1

Fix Field deconstruction

b04b900

Use a name that doesn't have double underscore for DummyField

4530085

Double underscore causes errors in lookups

Fix various typings

667ed9b

Disable invalid pylint errors

635fbe8

jleclanche force-pushed the dev/compat-with-model branch from f227d54 to 635fbe8 Compare December 14, 2022 06:44

danni reviewed Jan 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better compat with Model and Django Migrations #31

Better compat with Model and Django Migrations #31

jleclanche commented Dec 14, 2022 •

edited

Loading

jleclanche commented Dec 14, 2022

danni left a comment

danni Jan 2, 2023

jleclanche Jan 26, 2023

danni Jan 2, 2023

danni Jan 2, 2023

jleclanche Jan 26, 2023

danni Jan 2, 2023

jleclanche Jan 26, 2023

Better compat with Model and Django Migrations #31

Are you sure you want to change the base?

Better compat with Model and Django Migrations #31

Conversation

jleclanche commented Dec 14, 2022 • edited Loading

jleclanche commented Dec 14, 2022

danni left a comment

Choose a reason for hiding this comment

danni Jan 2, 2023

Choose a reason for hiding this comment

jleclanche Jan 26, 2023

Choose a reason for hiding this comment

danni Jan 2, 2023

Choose a reason for hiding this comment

danni Jan 2, 2023

Choose a reason for hiding this comment

jleclanche Jan 26, 2023

Choose a reason for hiding this comment

danni Jan 2, 2023

Choose a reason for hiding this comment

jleclanche Jan 26, 2023

Choose a reason for hiding this comment

jleclanche commented Dec 14, 2022 •

edited

Loading