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 Thomas Ormezzano)

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 Thomas Ormezzano, 6 years ago

Component: UncategorizedDatabase layer (models, ORM)
Owner: changed from nobody to Thomas Ormezzano
Status: newassigned
Type: UncategorizedNew feature

comment:2 by Thomas Ormezzano, 6 years ago

Description: modified (diff)

comment:3 by Dan Davis, 6 years ago

Cc: Dan Davis added
Has patch: unset
Type: New featureUncategorized

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.

comment:4 by Dan Davis, 6 years ago

Resolution: invalid
Status: assignedclosed

thomazzo,

I've reviewed the general and MySQL specific OPTIONS and settings for the test database, as listed:

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 Adam Johnson, 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 Adam Johnson, 6 years ago

Resolution: invalid
Status: closednew
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:7 by Adam Johnson, 6 years ago

Cc: Adam Johnson added

comment:8 by Dan Davis, 6 years ago

Owner: changed from Thomas Ormezzano to Dan Davis
Status: newassigned

@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 Adam Johnson, 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 Dan Davis, 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 Dan Davis, 6 years ago

Has patch: set

comment:12 by Thomas Ormezzano, 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:13 by Thomas Ormezzano, 6 years ago

Owner: changed from Dan Davis to Thomas Ormezzano
Last edited 6 years ago by Tim Graham (previous) (diff)

comment:14 by Adam Johnson, 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.

Last edited 6 years ago by Tim Graham (previous) (diff)

comment:15 by Carlton Gibson, 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!

comment:16 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 9625d13:

Fixed #29882 -- Added events and stored routines to MySQL's cloned test databases.

Note: See TracTickets for help on using tickets.
Back to Top