Opened 12 years ago

Closed 9 years ago

Last modified 9 years ago

#18782 closed Bug (fixed)

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

Reported by: rodolfo_3 Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: mysql introspection table view
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no 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 12 years ago.
diff with a solution and a regression test

Download all attachments as: .zip

Change History (16)

comment:1 by Claude Paroz, 12 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

by rodolfo_3, 12 years ago

Attachment: fix18782.diff added

diff with a solution and a regression test

comment:2 by rodolfo_3, 12 years ago

Has patch: set

comment:3 by Anssi Kääriäinen, 12 years ago

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 by Claude Paroz, 12 years ago

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 by Anssi Kääriäinen, 12 years ago

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 by Claude Paroz, 12 years ago

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 by Anssi Kääriäinen, 12 years ago

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.

in reply to:  3 comment:8 by rodolfo_3, 12 years ago

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 by rodolfo_3, 12 years ago

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 by Claude Paroz, 12 years ago

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 by Anssi Kääriäinen, 12 years ago

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...

comment:12 by Claude Paroz, 9 years ago

Needs tests: unset
Version: 1.4master

comment:13 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:14 by Claude Paroz <claude@…>, 9 years ago

Resolution: fixed
Status: newclosed

In ed297061a676e629983664031d77da1d0a70af7d:

Fixed #18782 -- Prevented sql_flush to flush views

Thanks rodolfo_3 for the report and the initial patch, and
Josh Smeaton, Shai Berger and Tim Graham for the reviews.

comment:15 by Shai Berger <shai@…>, 9 years ago

In d128eac316dd5a8578fbae506028a3f2ade49420:

Changed Oracle test-user creation to grant privileges instead of roles

because the roles (specifically RESOURCE) are deprecated.
Also added optional support for creating views in tests, and made an
introspection test fail (rather than skip) if a view cannot be created
due to lacking privileges.

Refs #18782

Thanks Tim Graham for review, and Josh Smeaton

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