#5461 - Refactoring of database backends for creation, etc

[Russell Keith-Magee]

Hi all,

I've just uploaded an updated patch for ticket #5461 [1].

This is really a follow on from two older tickets - The database
backend refactoring (#5106 [2]) and the recent database type
refactoring (#7560 [3]). The patch isn't strictly required, but given
we are aiming for API stability, it makes sense to me that we should
finalize the database API so that it uses a consistent approach for
all operations.

r6192 [4] added an empty skeleton for the creation part of this work;
this patch finishes the job.

I've tested on sqlite3, mysql and postgres_psycopg2. This leaves the
oracle backend and base postgres.

At this point, I'm looking for:

* Testing and validation for Oracle and psyco1. Psycopg1 is mostly
tested by virtue of the postgres_psycopg2 testing, but there is one
subtle difference, so I'd like some independent validation

* Any other feedback on the approach taken by the patch.

Yours,
Russ Magee %-)

[1] http://code.djangoproject.com/ticket/5461
[2] http://code.djangoproject.com/ticket/5106
[3] http://code.djangoproject.com/ticket/7560
[4] http://code.djangoproject.com/changeset/6192

[Ivan Illarionov]

Looks great to me.

I have further proposals for database backends refactoring:
* Move all the functions from django.core.management.sql to the
BaseCreation class and let backends change its behavior.
* Create django/db/backends/validation.py with BaseValidation class
* Move all the functions from django.core.management.validation to the
BaseValidation class.
* Add new validation attribute to connection and move all backend-
specific validation (like 255 max_length MySQL limit) to these
backends.

This would finally remove all scary `if settings.DATABASE_ENGINE`
checks and replace many `if connection.features.*` with nice
consistent OO design. External developers can't add new database
features to support various RDBMS quirks (and there are many) so this
will make writing external backends possible without patching Django.

If this (or part of it) looks interesting I can write a patch.

Regards,
Ivan Illarionov

[Leo Soto M.]

On Thu, Jul 31, 2008 at 9:14 AM, Russell Keith-Magee
<freakb...@gmail.com> wrote:
>
> Hi all,
>
> I've just uploaded an updated patch for ticket #5461 [1].

Great!

I just adapted the code of Jython backends to this patch and getting
rid of all the duplication on the introspection modules feels good.
--
Leo Soto M.
http://blog.leosoto.com

[alex....@gmail.com]

I like the suggestion of moving the db table creation stuff to also be
in django.db.backends, this is, as Ivan says, more OO and cohesive,
plus it seems like the better place to go. That would also make sense
in the context of schema evolution, if that were to ever make it's way
into django(as opposed to via 3rd party projects).

[Russell Keith-Magee]

On Fri, Aug 1, 2008 at 2:12 AM, Ivan Illarionov
<ivan.il...@gmail.com> wrote:
>
> Looks great to me.
>
> I have further proposals for database backends refactoring:

Good suggestions, all. I've just uploaded a patch that implements the
changes you suggest. Feedback welcome.

Yours,
Russ Magee %-)

[Adam V.]

A couple of notes:
* Lines 18-19 of db/__init__.py shouldn't be there, I think.
Since using external backends is supported, there's no need to print
the exception if the current backend isn't found in Django itself.
(Probably just debugging code that got left in?)

* Also, it looks like db/__init__.py isn't using the "...import curry"
import anymore.

[Ramiro Morales]

On Sat, Aug 2, 2008 at 3:08 PM, Adam V. <fla...@gmail.com> wrote:
>
> A couple of notes:
> * Lines 18-19 of db/__init__.py shouldn't be there, I think.
> Since using external backends is supported, there's no need to print
> the exception if the current backend isn't found in Django itself.
> (Probably just debugging code that got left in?)
>

Had noted the same detail and I agree. A small obvious correction though: With
Russell's latest patch (5461-r8194.diff) the problem is in lines 19 and 20.

Another minor related modification I'd suggest is to show the error message
when importing the external DB backend has failed simply by printing e_user.
This has proven to be helpful when coding and testing a backend and could be of
help to people when setting up their PYTHONPATH to use it.

I'm attaching now to the ticket an updated patch (5461-r8194.2.diff) implemen-
ting these two suggestions.

Regards,

--
Ramiro Morales

[Adam V.]

On further inspection, the traceback should be printed in the inner
except block (instead of removed entirely.)

[Ivan Illarionov]

On Aug 2, 12:39 pm, "Russell Keith-Magee" wrote:
> Good suggestions, all. I've just uploaded a patch that implements the
> changes you suggest. Feedback welcome.

> Good suggestions, all. I've just uploaded a patch that implements the
> changes you suggest. Feedback welcome.

Really nice patch. Thank you for improving this part of Django.

I suggest going further:

* remove `management/core/sql.py` altogether and move all the
functions to BaseDatabaseCreation class to remove any ambiguity
between `connection.creation` object and `core.management.sql` module.

* make parent connection an attribute of BaseDatabaseCreation,
importing connection inside methods of connection.creation makes
little sense.

* remove `support_constraints` feature and instead just override
`sql_for_pending_references` and `sql_destroy_model` methods inside
sqlite backend (the only one that does not support constraints).


I am not so sure about the following:

* `supports_tablespaces` could be replaced by overriding
`sql_create_model`, `sql_for_many_to_many_field` and
`sql_indexes_for_field` methods in Oracle backend (the only one that
supports it)

* `inline_fk_references` could be replaced by overriding
`sql_create_model` and `sql_for_many_to_many_field` methods inside
MySQL backend (all other backends have this feature). Thus `features`
attribute can be removed from BaseDatabaseCreation class.

* make `style` another property of creation object instead of passing
it as argument.

Regards,
Ivan Illarionov

[Adam V.]

> * `supports_tablespaces` could be replaced by overriding
> `sql_create_model`, `sql_for_many_to_many_field` and
> `sql_indexes_for_field` methods in Oracle backend (the only one that
> supports it)

SQL Server also supports tablespaces (calls them something else, I
think, like "file groups").

[Russell Keith-Magee]

On Sun, Aug 3, 2008 at 5:42 PM, Ivan Illarionov
<ivan.il...@gmail.com> wrote:
>
> On Aug 2, 12:39 pm, "Russell Keith-Magee" wrote:
>> Good suggestions, all. I've just uploaded a patch that implements the
>> changes you suggest. Feedback welcome.
>
>> Good suggestions, all. I've just uploaded a patch that implements the
>> changes you suggest. Feedback welcome.
>
> Really nice patch. Thank you for improving this part of Django.
>
> I suggest going further:
>
> * remove `management/core/sql.py` altogether and move all the
> functions to BaseDatabaseCreation class to remove any ambiguity
> between `connection.creation` object and `core.management.sql` module.

I originally considered this, but decided against it.
core.management.sql contains some logic that is pretty closely tied to
the way models are interpreted, and there is almost nothing backend
specific about that logic. I settled on keeping the SQL specific calls
in the backend, and everything else where it is currently.

> * make parent connection an attribute of BaseDatabaseCreation,
> importing connection inside methods of connection.creation makes
> little sense.

A nice idea, but not as easy as it sounds. The backend modules are
constructed as class members, not instance members.

> * remove `support_constraints` feature and instead just override

> * `supports_tablespaces` could be replaced by overriding

> * `inline_fk_references` could be replaced by overriding

One of the biggest unresolved issues with this patch is the overlap
between a fully OO database backend and the 'features' class on the
backend. Most of what is currently stored as 'features' could be
implemented as function overloading in an object-based interface.

Going down this path will require a fair bit of refactoring, so I'm
waiting on some feedback from the other core developers before I do
too much more work on this particular area.

> * make `style` another property of creation object instead of passing
> it as argument.

I'm not sure this is the right thing to do. The output style isn't a
database specific thing, and it isn't consistent across all uses.

Russ %-)