Code

Opened 2 years ago

Last modified 23 months ago

#18782 new Bug

database "view" treated as "table" on flush (mysql)

Reported by: rodolfo_3 Owned by: nobody
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords: mysql introspection table view
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I'm using a mysql view (https://dev.mysql.com/doc/refman/5.0/en/create-view.html) on a Mysql database to replace a default "auth_user_group" table on a django project. I replace the table in a "post_syncdb" signal.

When I run the tests, django try to "flush" the database, executing a "truncate", it try do it with the view too. It's cause a database error, and fail the flush:

Error: Database test_yoshi couldn't be flushed. Possible reasons:
  * The database isn't running or isn't configured correctly.
  * At least one of the expected database tables doesn't exist.
  * The SQL was invalid.
Hint: Look at the output of 'django-admin.py sqlflush'. That's the SQL this command wasn't able to run.
The full error: (1146, "Table 'test_yoshi.auth_user_groups' doesn't exist")

It's happen because the command to inspect the database on mysql backend use the command "show tables". But, on mysql, views are returned by this command: https://dev.mysql.com/doc/refman/5.0/en/show-tables.html

The solution is replace the query on the method "get_table_list" on the file "db/backends/mysql/introspection.py":

    def get_table_list(self, cursor):
        "Returns a list of table names in the current database."
        cursor.execute("SHOW FULL TABLES WHERE TABLE_TYPE = 'BASE TABLE'")
        return [row[0] for row in cursor.fetchall()]

Attachments (1)

fix18782.diff (1.5 KB) - added by rodolfo_3 23 months ago.
diff with a solution and a regression test

Download all attachments as: .zip

Change History (12)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

Changed 23 months ago by rodolfo_3

diff with a solution and a regression test

comment:2 Changed 23 months ago by rodolfo_3

  • Has patch set

comment:3 follow-up: Changed 23 months ago by akaariai

I wonder if the approach here is correct, or if it would cause further problems. The PostgreSQL code does this in get_table_list():

        SELECT c.relname
        FROM pg_catalog.pg_class c
        LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
        WHERE c.relkind IN ('r', 'v', '')
            AND n.nspname NOT IN ('pg_catalog', 'pg_toast')
            AND pg_catalog.pg_table_is_visible(c.oid)

The c.relkind in ('r', 'v', '') says that we are explicitly asking for views, too... So, before committing this we should make clear if we want views or not from get_table_list(), and then make sure we are consistent on all backends.

IMHO the problem reported in this ticket is caused by having an unmanaged view used by a managed model. It might be better to do a hack and change the model._meta.managed to False in the post_sync() signal.

comment:4 Changed 23 months ago by claudep

It might be that this specific ticket could be solved with the tip given by akaariai. But generally, I think we should make it more clear. For example, we could add a keyword parameter to get_table_list (maybe table_names too), with_views. Or return a tuple (table_name, type) so client code can then act depending on type. For example, inspectdb could automatically set the managed flag to False for views.

comment:5 Changed 23 months ago by akaariai

I do like the idea of setting automatically managed=False for existing views. The problem I see with this approach is that this goes somewhat against the philosophy of Django - we do not alter the models based on inspection, we trust the models to be correct. So, on that basis a light -0 on this approach. One of the worries is that we would need to consistently set the managed flag in app loading, not only in syncdb/flush etc.

comment:6 Changed 23 months ago by claudep

I was only referring to inspectdb for the automatic managed flag. I do agree that for other parts of the code, it is the user's responsability to set it correctly. However, there might be other use cases where the table vs view distinction might be useful.

comment:7 Changed 23 months ago by akaariai

I don not see any drawback of returning a namedtuple with "tablename is_view" (or type instead of is_view) as return values. This is internal API and we can alter it as we wish. Also, automatically inspecting views as non-managed would be good. Maybe with a flag to inspectdb to only inspect tables/views/both.

comment:8 in reply to: ↑ 3 Changed 23 months ago by rodolfo_3

The PostgreSQL code are explicitly asking for views, but it isn't true on sqlite3:

  SELECT name FROM sqlite_master
  WHERE type='table' AND NOT name='sqlite_sequence'
  ORDER BY name

To return views using sqlite3, the "where" clause needs to be changed to something like

  type in ('table', 'view') and not name='sqlite_sequence'

The oracle backend return just tables, ignoring views:

  SELECT TABLE_NAME FROM USER_TABLES

Replying to akaariai:

I wonder if the approach here is correct, or if it would cause further problems. The PostgreSQL code does this in get_table_list():

        SELECT c.relname
        FROM pg_catalog.pg_class c
        LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
        WHERE c.relkind IN ('r', 'v', '')
            AND n.nspname NOT IN ('pg_catalog', 'pg_toast')
            AND pg_catalog.pg_table_is_visible(c.oid)

The c.relkind in ('r', 'v', '') says that we are explicitly asking for views, too... So, before committing this we should make clear if we want views or not from get_table_list(), and then make sure we are consistent on all backends.

IMHO the problem reported in this ticket is caused by having an unmanaged view used by a managed model. It might be better to do a hack and change the model._meta.managed to False in the post_sync() signal.

comment:9 Changed 23 months ago by rodolfo_3

Just set managed=False don't solve the problem on tests. Between every test case, django executes a sql_flush() to all tables. When do it for a view, an exception is raised. To use this solution, another patch is needed, changing the behavior of the "sql_flush" method of the mysql backend.

comment:10 Changed 23 months ago by claudep

  • Needs tests set

The 3 latest commits in https://github.com/claudep/django/commits/18782 show how we could address the issue.

I didn't address Oracle (no access) and some tests should probably be written. Apart from that, I would be interested to know if this is an acceptable path.

comment:11 Changed 23 months ago by akaariai

To me this looks a good path forward. Setting managed=False for views in inspectdb seems obviously correct. I guess skipping the flush of views (even if the model is managed) is OK, as that will not work in any case. Not that I have done anything like a thorough review...

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.