READ COMMITTED requirement check doesn't account for database views

Created on 25 May 2023, about 2 years ago

Problem/Motivation

REPEATABLE-READ
The recommended level for Drupal is "READ COMMITTED". For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: {insert long list of native mysql views}. See the setting MySQL transaction isolation level page for more information.
READ-COMMITTED
For this to work correctly, all tables must have a primary key. The following table(s) do not have a primary key: {insert long list of native mysql views}. See the setting MySQL transaction isolation level page for more information.

Anyone that's worked with views will immediately know that this is because in the schema API and some other places, a view shows up largely like a table. I assume this is a pretty useful feature but it can also be frustrating if you don't take it into account in situations like this.

Steps to reproduce

1. Install Drupal
2. run this on your database. CREATE VIEW test AS SELECT * FROM users;
3. Visit the status page and see one of the above warning or errors.

Proposed resolution

This might be tricky. Adjusting findTables to not include views might be too big a change? That said, I don't see an interface for listing or interacting with what type of "table" you're interacting with so only including real tables might make sense.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ› Bug report
Status

Active

Version

10.1 ✨

Component
MySQL driverΒ  β†’

Last updated 11 days ago

Created by

πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @neclimdul
  • πŸ‡³πŸ‡±Netherlands daffie

    Bumping this to major. This is very confusing for site owners. Would be great if we could fix this before Drupal 10.1 is released. Maybe this should even be a "critical" one.

  • πŸ‡·πŸ‡ΊRussia Chi

    The issue only applies to MySQL/MariaDB views. Right?

  • πŸ‡³πŸ‡±Netherlands daffie

    @chi you are right.

  • πŸ‡¬πŸ‡§United Kingdom catch

    I think it would be reasonable to restrict ::findTables() to only finding actual database tables - we could do that with a change record probably.

    I tried to think of bc-friendly options (adding a second argument for $include_views = TRUE? adding a settings override?) but couldn't come up with a use case.

  • πŸ‡ΊπŸ‡ΈUnited States neclimdul Houston, TX

    I guess this just needs someone to write a patch?

  • Status changed to Needs work almost 2 years ago
  • πŸ‡ΊπŸ‡ΈUnited States jrb Raleigh-Durham Area, NC, USA

    Here's a patch to get things started. This will:

    1. Add an optional parameter to findTables() to exclude SQL views from the query.
    2. Use that parameter in the mysql_requirements() call in mysql.install.
  • last update almost 2 years ago
    Custom Commands Failed
  • πŸ‡ΊπŸ‡ΈUnited States anoopjohn Washington D. C.

    The patch works correctly on a 10.1.6 drupal site and stops showing warnings against views in the database.

  • πŸ‡ΊπŸ‡ΈUnited States micahw156

    Confirming this patch worked for me on 10.1.6 as well.

  • πŸ‡¦πŸ‡ΊAustralia nterbogt

    Confirm this works for me too.

  • πŸ‡¦πŸ‡ΊAustralia apiddington

    Confirming that this worked OK with 10.2.

  • πŸ‡¦πŸ‡ΊAustralia apiddington

    Patch at #8 no longer works with v11.2
    It seems that core/modules/mysql/mysql.install has been replaced by core/modules/mysql/src/Hook/MysqlRequirements.php

    This patch seems to work OK with v11.2: 3362726-9.exclude-views-from-table-list-11.2.patch

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Can you convert this to an MR?

    Tests don't run against patches.

  • First commit to issue fork.
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Added new test function.

  • Pipeline finished with Failed
    13 days ago
    Total: 222s
    #528128
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    Did you accidentally remove the actual change? I only see the test stub.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    I havent applied any of the patches yet. It needs to be done.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    findTables() has a new boolean parameter. The test would be setting the param to true in order to exclude views and allow only tables.

    The test will break against the current codebase because it will be setting a non-existent parameter. The code within the function is not going to be tested because the test wont reach it?

    Any ideas how such issues are correctly addressed in a kernel test?

  • Pipeline finished with Failed
    13 days ago
    Total: 159s
    #528201
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Okay, I see that the errors in the IS are triggered because views do not need to contain a primary key. There is existing code in other test functions involving the presence/ non-presence of primary keys. Possibly test coverage should be by extending such functions. Or perhaps can incorporate the primary key test code into the new test function..

    If test table (view) contains no primary key, check if findTables() 2nd parameter is set to TRUE. If it is, check if the table is a view. Assertion passes if it is a view with no primary key..

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    The core Schema.php has a method createTable() but has no equivalent method to create views. The tests rely on createTable(). Not sure it is worthwhile or feasible to support views in the piecemeal fashion of this issue.

    If we are going to take cognisance of views should be not build it into the database api including the core Schema.php. As it stands we will need, i think, to use raw sql queries in the tests ie CREATE VIEW statements as mentioned in the steps to reproduce.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    If we could add minimal support for DB views into core say by adding createView() method, possibly related CRUD methods too. Then do minor refactor of existing test functions eg the one that tests tables all have primary keys. So the test function would do a 'it has not got a primary key. Is it a view? Yes? In which case, no problem.'

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think for this issue we would manually run asql command to create a db view on the test db.

    Creating an api for creating and managing db views is it if scope here and not necessary.

    I don't know how we go about doing my recommendation here, just giving thoughts on approach and scope.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    I don't think we should change the default behaviour of findTables()... if we do we'll break any tests for contrib or custom code which are creating a view at the moment as we rely on findTables() to clear up test databases. Maybe we can introduce a flag that defaults to the current behaviour but allows you to optionally list only real tables or only views.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    That said I guess

        if ($original_prefix != $test_prefix) {
          $tables = Database::getConnection()->schema()->findTables('%');
          foreach ($tables as $table) {
            if (Database::getConnection()->schema()->dropTable($table)) {
              unset($tables[$table]);
            }
          }
        }
    

    is flawed because dropTable is not going to work... and looking at our dropTable() implementation

      /**
       * {@inheritdoc}
       */
      public function dropTable($table) {
        if (!$this->tableExists($table)) {
          return FALSE;
        }
    
        $this->executeDdlStatement('DROP TABLE {' . $table . '}');
        return TRUE;
      }
    

    Well the tableExists call is going to return false... so it's not going to even error.

    Also I think both SQLite and Postgres are unaffected because their implementations of findTables() is different.

    So... tldr; perhaps fixing findTables() to only return tables for MySQL is actually the way to go here!

  • Merge request !12442Prove MySQL is the outlier β†’ (Open) created by alexpott
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Just added an MR that shows that the MySQL driver is on it's own here by returning views from findTables()... so I think we're okay just to remove the capability from MySQL. Discussions around a database views API can happen elsewhere.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    My MR now also includes a fix that's limited to MySQL (and MySQLi) where the problem occurs.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    alexpott β†’ changed the visibility of the branch 3362726-read-committed-requirement to hidden.

  • Pipeline finished with Canceled
    13 days ago
    Total: 875s
    #528430
  • Pipeline finished with Failed
    13 days ago
    Total: 673s
    #528432
  • Pipeline finished with Success
    12 days ago
    Total: 4078s
    #528465
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    @alexpott

    I added code comment to: https://git.drupalcode.org/project/drupal/-/merge_requests/12442/diffs?c...

    Is that commit necessary for filtering views amongst tables?

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    +1 #27

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    #27 is wrong. See #28 - if your module creates a view it'll error on trying to clean up a test in Postgres - not sure what it'll do for MySQL but the drop table is certainly not going to clean up the table as expected. All of our code is assuming findTable() only returns tables - which is excatly how it works for Postgres and SQLite... it just doesn't work this way for MySQL. I would argue that findTables() shoudl only find tables because of this and that the current MySQL behaviour is the outlier. The tests and changes added here prove this.

    @oily the MySQL 5 thing is altered so that getComment() has a consistent return value for both MySQL and Postgres. FWIW the MySQL implementation has no business changing the FALSE to an empty string because of the preg_replace.

  • Pipeline finished with Success
    12 days ago
    Total: 6873s
    #528488
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    @alexpott The commit I queried.. I understand the purpose now. Line 688 reveals the relevance to this issue. If i had scrolled up a bit I would have seen it.

  • Pipeline finished with Success
    12 days ago
    #528540
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Test coverage in place. Test-only test fails as it should.

    Can move to RTBTC?

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @oily yes you can.

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London
  • Pipeline finished with Failed
    12 days ago
    Total: 168s
    #529150
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    I created the CR as per #6

  • Pipeline finished with Success
    11 days ago
    Total: 2861s
    #529176
  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    Reviewed CR. 'materialized' views is interesting. Just looked it up. Need to research further. Asking myself 'how to create material view as opposed to a normal view..' Could CR benefit from example Mysql query to create a material view? The example is the IS is: "CREATE VIEW test AS SELECT * FROM users;" That does create a 'material' view?

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    @oily it's just what they are called - see https://en.wikipedia.org/wiki/Materialized_view - also I felt it was important to distinguish from Drupal's use of the views as in the views module.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Ugh... nah I'm wrong. DBs implement both views and materialized views... old wires crossed in my head.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Gone for "database views" as the issue title here has... @oily thanks for questioning this!

  • πŸ‡¬πŸ‡§United Kingdom oily Greater London

    @alexpott Thanks for considering my suggestion. The CR looks good!

Production build 0.71.5 2024