Opened 3 years ago

Closed 11 months ago

Last modified 11 months 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: master
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 3 years ago.
diff with a solution and a regression test

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 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 3 years ago by rodolfo_3

diff with a solution and a regression test

comment:2 Changed 3 years ago by rodolfo_3

  • Has patch set

comment:3 follow-up: Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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 3 years 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...

comment:12 Changed 11 months ago by claudep

  • Needs tests unset
  • Version changed from 1.4 to master

comment:13 Changed 11 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 11 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 11 months ago by Shai Berger <shai@…>

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