SaferRemoveIndexConcurrently #18
Conversation
Coverage Report Results
|
marcelofern
force-pushed
the
add-safer-remove-index-concurrently
branch
2 times, most recently
from
e356bbf to
d3ee1f5
Compare
In future commits, we want to have a new class called SaferRemoveIndexConcurrently, which will basically be the same as SaferAddIndexConcurrently but with opposite behaviour. These functions that are now being moved to the base class will be useful in this future SaferRemoveIndexConcurrently class and are thus extracted in this commit. The diff might look a bit strange, but essentially this commit moves the database_forwards/backwards functions out (along with the auxiliary functions that make it work) into the new BaseIndexOperation class, and calls it from the existing SaferAddIndexConcurrently class.
Given that the BaseIndexOperation class will cater for both
SaferAddIndexConcurrently and SaferRemoveIndexConcurrently, we need to
change the way the index argument is passed.
This is because SaferAddIndexConcurrently is instantiated like this:
SaferAddIndexConcurrently(
model_name="foo",
index=models.Index(
fields=["bar"],
name="foo_bar_idx",
),
)
Whereas SaferRemoveIndexConcurrently will be instantiated like this (and
thus doesn't have an index obj at instantiation time):
SaferRemoveIndexConcurrently(
model_name="foo",
name="foo_bar_idx",
)
So that they can be run with other index names later.
Not using RunSQL as a base means that I can simplify much of the code in this class. Plus, I can also make the BaseIndexOperation more generic so that it can be easily used by SaferRemoveIndexConcurrently Note that the `hint` argument isn't available on Django's original AddIndexConcurrently class, and it is not necessary any longer as we don't use the router but the model itself to check if the migration can happen. Thus, the `hint` argument has been removed from the implementation and documentation.
marcelofern
force-pushed
the
add-safer-remove-index-concurrently
branch
from
8375415 to
ae504a1
Compare
| @@ -32,7 +32,7 @@ def safer_create_index( | |||
| to_state: migrations.state.ProjectState, | |||
| index: models.Index, | |||
| model: type[models.Model], | |||
| operation: SaferAddIndexConcurrently, | |||
| operation: SaferAddIndexConcurrently | SaferRemoveIndexConcurrently, | |||
There was a problem hiding this comment.
can this instead be a Protocol class as the type you expect?
There was a problem hiding this comment.
There should be only two classes that create/drop indexes, so I'm not sure there're gains in using structural typing here if we're not aiming for flexibility. On the side, nominal typing provides stronger contracts too.
pyproject.toml
Outdated
| @@ -21,7 +21,7 @@ where = ["src"] | |||
|
|
|||
| [project] | |||
| name = "django_pg_migration_tools" | |||
| version = "0.1.2" | |||
| version = "0.1.3" | |||
There was a problem hiding this comment.
I'd encourage the previous commit updating dependencies and the part of this commit that changes the version number be a separate PR dedicated to releasing a new version rather than bundling it with this change.
There was a problem hiding this comment.
Thanks. Are there any pros/cons to either approach I wouldn't be aware of? I thought that if a release is linked to a single change it would be best to keep it in the same PR so that it's easier to link to the scoped work from the tags page. Similar to how some repos create a single release PR over multiple aggregated feature branches. Not particularly fussy about that either though.
There was a problem hiding this comment.
yeah, certainly not a hard requirement, but my own sensibility is that releases should be separate as a general habit.
There was a problem hiding this comment.
Here's a separated PR for the release. To be merged after this one is merged.
| self, app_label: str, state: migrations.state.ProjectState | ||
|
|
||
| class SaferAddIndexConcurrently( | ||
| BaseIndexOperation, psql_operations.AddIndexConcurrently |
There was a problem hiding this comment.
is there anything stopping us from not using inheritence for composition? So that the SaferAddIndexConcurrently instance creates it's own instance of AddIndexConcurrently that it delegates to rather than being a subclass of AddIndexConcurrently?
There was a problem hiding this comment.
I'd surely prefer composition if the code action was "AddIndexConcurrently, please go do something for me".
But in this case the code action is more like "AddIndexConcurrently, I need your interface so that Django's migration apparatus can understand me as an index creation/deletion operation".
This includes having methods that will register application database state alterations for example. But more importantly, I'm interested in picking up changes from upstream on that specific interface.
For example, a recent change in Django (new in 5.1) has added the "category" attribute to operations. In this case, category = OperationCategory.ADDITION (or REMOVAL), wouldn't be picked up by our classes unless we inherit from upstream. Otherwise, we'd have to keep different interfaces for different Django versions, which is less desirable.
There was a problem hiding this comment.
fair enough, inheritence sounds reasonable then.
marcelofern
force-pushed
the
add-safer-remove-index-concurrently
branch
from
ae504a1 to
ae35bef
Compare
| "charmodel", | ||
| "char_field_idx", | ||
| ) | ||
| # Proceed to try and add the index: |
docs/usage/operations.rst
Outdated
| How to use | ||
| ---------- | ||
|
|
||
| 1. Remove the index to the relevant model: |
There was a problem hiding this comment.
copy/paste: should be "… from the relevant model"
This class is the analogous of SaferAddIndexConcurrently, but does the direct oposite. I.e., it swaps the database_forwards operation by the database_backwards and vice-versa.
marcelofern
force-pushed
the
add-safer-remove-index-concurrently
branch
from
50d6377 to
68aa12a
Compare
Description
Adds a new Django migration operation class
SaferRemoveIndexConcurrently. The name is self-explanatory, but here's a screenshot of the documentation (built withmake docs) with more details (also available in the diff on the ante-penultimate commit).