[Django] #29882: UDFs are not dumped to the clone MySQL test db
Django
-----------------------------------------+------------------------
Reporter: thomazzo | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Stored procedures and functions are not dumped to the clone db.
Adding the `--routines` option to the mysqldump command would fix it (see
https://dev.mysql.com/doc/refman/8.0/en/mysqldump.html#option_mysqldump_routines)
--
Ticket URL: <https://code.djangoproject.com/ticket/29882>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: thomazzo | Owner: thomazzo
Type: New feature | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by thomazzo):
* status: new => assigned
* type: Uncategorized => New feature
* component: Uncategorized => Database layer (models, ORM)
* owner: nobody => thomazzo
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:1>
Django
Reporter: thomazzo | Owner: thomazzo
Type: New feature | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by thomazzo:
Old description:
> Stored procedures and functions are not dumped to the clone db.
>
> Adding the `--routines` option to the mysqldump command would fix it (see
> https://dev.mysql.com/doc/refman/8.0/en/mysqldump.html#option_mysqldump_routines)
New description:
Stored procedures and functions are not dumped to the clone db.
When creating the test database with mysql, the mysqldump command is only
dumping the schemas but not the stored procedures and functions, when that
should probably be the default.
Adding the `--routines` option to the mysqldump command would fix it (see
https://dev.mysql.com/doc/refman/8.0/en/mysqldump.html#option_mysqldump_routines)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:2>
Django
Reporter: thomazzo | Owner: thomazzo
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Davis):
* cc: Dan Davis (added)
* has_patch: 1 => 0
* type: New feature => Uncategorized
Comment:
thomazzo,
Can you clarify your database OPTIONS settings?
Without any options to force clone/mirror, Django should not preserve the
DDL and data in the database. In the normal mode, Django is not cloning
the database using `mysqldump` - it is dropping and recreating the
database, and then running migrations.
I've created a git repository that verifies this occurs, but it is not a
patch, but an example.
The git repo is https://github.com/danizen/django-review-29882.
If you set it up with mysql 5.7 on ubuntu and create a "djangoreview" user
and "djangoreview" and "test_djangoreview" database to which that user has
all privileges, you should be able to run `./manage.py test` and verify
you get:
{{{
django.db.utils.OperationalError: (1305, 'FUNCTION test_djangoreview.HELLO
does not exist')
----------------------------------------------------------------------
Ran 2 tests in 0.026s
FAILED (errors=1)
Destroying test database for alias 'default'...
}}}
If you want, you could try to get this test to work in isolation by
writing a custom migration as follows:
{{{
./manage.py makemigrations app --empty -n add_hello_function
}}}
Then, edit the migration file created, and use the RunSQL migration
operation to run text from the file `sql/ddl/hello.sql`. This shows how
you can use the django migrations system to manage custom defined SQL.
There are add-on packages that do similar things, but I want to emphasize
that Django core contains the functionality to read SQL files.
If you are using Python 3, I recommend you pathlib to read a file to get
the text to run.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:3>
Django
Reporter: thomazzo | Owner: thomazzo
Component: Database layer | Version: 2.1
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Davis):
* status: assigned => closed
* resolution: => invalid
Comment:
thomazzo,
I've reviewed the general and MySQL specific OPTIONS and settings for the
test database, as listed:
* here - https://docs.djangoproject.com/en/2.1/ref/settings/#databases
* and Here - https://docs.djangoproject.com/en/2.1/ref/databases/#mysql-
notes
There is no option I'm aware of that would clone the entire database -
think of how much time that would take. Also, the whole point of
Django's migration support is to allow a developer such as yourself to
manage DDL statements needed to define tables, views, functions, stored
procedures, and such.
Django is an opinionated framework, and it makes table definitions and
relationships easy, but if you need custom SQL, it provides mechanisms for
including that sort of DDL. So, I think this is not actually a bug. It
would be nice if Django's RunSQL migration operation supported filenames,
or there was some other migration operation that handled files. However,
that is a different feature and someone may already have filed it.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:4>
Django
Reporter: thomazzo | Owner: thomazzo
Type: Uncategorized | Status: closed
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution: invalid
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
Dan, I think you missed a step here... @thomazzo is talking about when
Django clones test database for parallel runs, which is done at the start
of test runs by `setup_databases`:
https://github.com/django/django/blob/master/django/test/utils.py#L176 .
This calls through to backend specific functions, which Django has for
MySQL here:
https://github.com/django/django/blob/9a88c6dd6aa84a1b35e585faa0058a0996cc7ed7/django/db/backends/mysql/creation.py#L31
I think the request is really around passing `--routines` to `mysqldump`
in `_clone_db`. I think this is a good idea and probably just needs one
line in that function.
Dan, you could make a test case from your example repo by using the
`CREATE FUNCTION` statement in a `RunSQL` in a migration.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:5>
Django
Reporter: thomazzo | Owner: thomazzo
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* status: closed => new
* type: Uncategorized => Bug
* resolution: invalid =>
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:6>
Django
Reporter: thomazzo | Owner: thomazzo
Type: Bug | Status: new
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* cc: Adam (Chainz) Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:7>
Django
Reporter: thomazzo | Owner: Dan Davis
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Davis):
* status: new => assigned
* owner: thomazzo => Dan Davis
Comment:
@adamchainz, I can add a 1-liner there and endeavor to add tests. Is
https://github.com/django/django/blob/55b0b766fbeb2f71e68331a2e14205702f681012/tests/backends/mysql/test_creation.py#L13
the right place to put that test, or it should it tested with a separate
repository because Django's tests are unit tests?
I ask because `django.test.TestCase` is so good it makes most of my tests
really Service Level tests or Database Integration Tests, not really unit
tests. Django itself may have a different philosophy about what the tests
should be and how fast they should run...
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:8>
Django
Reporter: thomazzo | Owner: Dan Davis
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
That looks the right place to add a test. Since these tests are true unit
tests and not actually calling `mysqldump`, you only need test that the
`--routines` argument is passed to a mock for `subprocess.Popen`.
But I'd even be tempted to not add a test, we often don't for features
that heavily rely on other systems - instead you can confirm the
functionality works for an example project like you made before. Since the
change is so small and pretty non-offensive, that should be fine.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:9>
Django
Reporter: thomazzo | Owner: Dan Davis
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Dan Davis):
I've enhanced https://github.com/danizen/django-review-29882 so that the
tests run fine without a parallel run, but fail when run in parallel,
reproducing the issue raised by the ticket.
The issue can be reproduced by running:
{{{
/manage.py test --parallel 2 -v 3
}}}
Fixing it is indeed a 1-liner, and I will submit a pull request shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:10>
Django
Reporter: thomazzo | Owner: Dan Davis
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Dan Davis):
* has_patch: 0 => 1
Comment:
Pull request exists as https://github.com/django/django/pull/10579
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:11>
Django
Reporter: Thomas Ormezzano | Owner: Dan Davis
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Thomas Ormezzano):
Hi,
Sorry folks I was busy this weekend and couldn't reply. Yes Adam that's
exactly what I meant.
I am not sure what the usual process is here but I had assigned this
ticket to myself and set it to `Has patch` as I already had a patch for it
and was waiting for this ticket to be accepted to submit the PR. Going to
submit it now that it has been accepted and I don't think Dan's suggestion
works.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:12>
Django
Reporter: Thomas Ormezzano | Owner: Thomas
| Ormezzano
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Thomas Ormezzano):
* owner: Dan Davis => Thomas Ormezzano
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:13>
Django
Reporter: Thomas Ormezzano | Owner: Thomas
| Ormezzano
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
Thomas, next time you have a patch please submit a PR immediately. Even if
it's wrong it can help the undesrtanding of the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:14>
Django
Reporter: Thomas Ormezzano | Owner: Thomas
| Ormezzano
Type: Bug | Status: assigned
Component: Database layer | Version: 2.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Thomas.
Yes, please do submit in future. I saw the issue, saw you had self-
assigned and assumed the PR would land soonish: I was waiting for the PR
to be able to assess the issue.
All good now. 🙂
Thanks for the input!
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:15>
Django
Reporter: Thomas Ormezzano | Owner: Thomas
| Ormezzano
Component: Database layer | Version: 2.1
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9625d13f7b76ed22c8d4c24d411531abe8502854" 9625d13]:
{{{
#!CommitTicketReference repository=""
revision="9625d13f7b76ed22c8d4c24d411531abe8502854"
Fixed #29882 -- Added events and stored routines to MySQL's cloned test
databases.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29882#comment:16>