Opened 6 years ago
Closed 6 years ago
#29882 closed Bug (fixed)
UDFs are not dumped to the clone MySQL test db
Reported by: | Thomas Ormezzano | Owned by: | Thomas Ormezzano |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 2.1 |
Severity: | Normal | Keywords: | |
Cc: | Dan Davis, Adam Johnson | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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)
Change History (16)
comment:1 by , 6 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Type: | Uncategorized → New feature |
comment:2 by , 6 years ago
Description: | modified (diff) |
---|
comment:3 by , 6 years ago
Cc: | added |
---|---|
Has patch: | unset |
Type: | New feature → Uncategorized |
comment:4 by , 6 years ago
Resolution: | → invalid |
---|---|
Status: | assigned → closed |
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.
comment:5 by , 6 years ago
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.
comment:6 by , 6 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:7 by , 6 years ago
Cc: | added |
---|
comment:8 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
@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...
comment:9 by , 6 years ago
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.
comment:10 by , 6 years ago
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.
comment:11 by , 6 years ago
Has patch: | set |
---|
Pull request exists as https://github.com/django/django/pull/10579
comment:12 by , 6 years ago
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.
comment:14 by , 6 years ago
Thomas, next time you have a patch please submit a PR immediately. Even if it's wrong it can help the understanding of the issue.
comment:15 by , 6 years ago
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!
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:If you want, you could try to get this test to work in isolation by writing a custom migration as follows:
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.