Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLite support via a MySQL parser #9

Merged
merged 109 commits into from
Feb 23, 2023
Merged

SQLite support via a MySQL parser #9

merged 109 commits into from
Feb 23, 2023

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Feb 8, 2023

Description

Implements SQLite support using a MySQL lexer instead of regular expressions to:

  • Greatly improve security
  • Solve most WordPress core unit test failures
  • Make the plugin more maintenable

See aaemnnosttv/wp-sqlite-db#55 to learn more about the problems with the current implementation.

Status:

  • WordPress works with no failing queries and escaping issues
  • WooCommerce works
  • Most unit tests pass: Tests: 14853, Assertions: 47545, Errors: 15, Failures: 37, Warnings: 34, Skipped: 60, Risky: 2. The remaining failures will have to be fixed in WordPress core.

Remaining tasks:

  • Fixing the lexer – porting went wrong – token->keyword is always null
  • Removing the parser dependency
  • Fixing the remaining WordPress test failures (by supporting any remaining query types)
  • For each fixed failure in ^, add a test case in the translator to enable fast regression spotting
  • Porting Lexer PHPUnit tests
  • API cleanup
  • Add docstrings
  • Merge!

Detailed status

This is it!

Here's a PHPunit report from the latest WordPress trunk:

Tests: 14853, Assertions: 47545, Errors: 15, Failures: 37, Warnings: 34, Skipped: 60, Risky: 2.

Note this PR won't get us to 100% passed tests. Fixing the remaining failures will require patching WordPress core.

A rundown of the failures

Tests_DB

Tests_DB test private wpdb methods:

    class WpdbExposedMethodsForTesting extends WP_SQLite_DB {}
    self::$_wpdb = new WpdbExposedMethodsForTesting();

I doesn't make a lot of sense when an SQLite wpdb alternative is used.

Tests_DB::test_replace_value_too_long_for_field
Tests_DB::test_process_fields_value_too_long_for_field

Tests for truncation of long strings inserted into varchar fields. However, SQLite only has TEXT fields, no varchar() fields. The text values are not going to be truncated. It doesn't seem like a useful test in SQLite context.

Tests_Comment

test_wp_update_comment fails because it counts updated rows and founds 1 when it expects 0. This is happening because in SQLite, all the rows matched by an UPDATE query are reported as affected, even if the actual data did not change. In MySQL, a row is reported as updated only if the actual data changes:

14) Tests_Comment::test_wp_update_comment
Failed asserting that 1 is identical to 0.

test_comment_field_lengths fails because it relies on MySQL truncation of too long strings. In SQLite, all text data is stored using the text datatype which means no truncation occurs:

15) Tests_Comment::test_comment_field_lengths
Failed asserting that 65535 is identical to 65536.

Tests_Comment_WpComment

Tests_Comment_WpComment::test_get_instance_should_succeed_for_float_that_is_equal_to_post_id fails because it compares ints and floats which always yields false in SQLite. It's similar to:

SELECT * FROM wp_posts WHERE id=42.0 
-- The actual ID is an integer 42

The failure:

13) Tests_Post_wpPost::test_get_instance_should_succeed_for_float_that_is_equal_to_post_id
Attempt to read property "ID" on bool

Tests_Menu_Walker_Nav_Menu

This failure is unrelated to SQLite changes. The test depends on a global state and won't work without the --process-isolation CLI option:

27) Tests_Menu_Walker_Nav_Menu::test_start_el_with_empty_attributes with data set #0 ('', '')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<li id="menu-item-833" class="menu-item-833"><a>Post title 0002501</a>'
+'<li class="menu-item-833"><a>Post title 0002501</a>'

The --process-isolation flag cannot be used, however, as it triggers the following error:

Serialization of 'pdo' is not allowed

Tests_Theme_ThemeDir

Tests_Theme_ThemeDir::test_broken_themes

I'm not sure why it broke, but here's the error:

Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
         'Title' => 'Child and Parent Theme'
         'Description' => 'The theme defines itself as its parent theme. Please check the <code>Template</code> header.'
     )
+    'tt1-blocks' => Array &3 (
+        'Name' => 'tt1-blocks'
+        'Title' => 'tt1-blocks'
+        'Description' => 'Stylesheet is missing.'
+    )
 )

cc @aristath

@adamziel
Copy link
Collaborator Author

adamziel commented Feb 8, 2023

I pushed basic support for more query types

The remaining ones I spotted:

SHOW FULL COLUMNS FROM `testprefix_options`
SHOW INDEX FROM wptests_commentmeta;
SHOW INDEXES FROM wptests_test_truncated_index WHERE Key_name='a_key';
SHOW TABLES LIKE 'wptests\\_dbdelta\\_create\\_test'
SHOW VARIABLES WHERE Variable_name='collation_connection'
START TRANSACTION;
TRUNCATE TABLE testprefix_options

The loose code living in the loops would ideally be brought back into the main Translator class.

I'm still trying to figure out an internal API for moving around the token stream – let's keep iterating.

@adamziel
Copy link
Collaborator Author

adamziel commented Feb 9, 2023

This PR now supports installing, navigating, and logging into WordPress!

The code could use a good cleanup. If this PR was a piece of art, it would be Pollock's painting. I'd like it to be tidy and nicely documented. I don't think of any part of it as final – let's tweak, improve, or completely rewrite it until it's clean and easy to grasp.

Also, the test suite boots but many tests still fail – I'm sure some queries aren't correctly translated. Also, the integration with $wpdb needs a solid revision, the current one is just a stopgap to get things going.

@adamziel
Copy link
Collaborator Author

adamziel commented Feb 9, 2023

Test run report:

Tests: 14853, Assertions: 41375, Errors: 201, Failures: 1869, Warnings: 17, Skipped: 81, Risky: 1.

$this->print_error( $this->last_error );
return false;
}

if ( preg_match( '/^\\s*(create|alter|truncate|drop|optimize)\\s*/i', $query ) ) {
if ( preg_match( '/^\\s*(set|create|alter|truncate|drop|optimize)\\s*/i', $query ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally these checks would be handled in the engine class – it already sets all this data based on a query type.

Comment on lines 406 to 435
do {
$error = null;
try {
$translation = $this->translator->translate(
$statement,
$this->found_rows_result
);
} catch ( PDOException $error ) {
if ( $error->getCode() !== self::SQLITE_BUSY ) {
return $this->handle_error( $error );
}
}
$this->execute_insert_query( $statement );
break;

case 'create':
$this->return_value = $this->execute_create_query( $statement );
break;

case 'alter':
$this->return_value = $this->execute_alter_query( $statement );
break;

case 'show_variables':
$this->return_value = $this->show_variables_workaround( $statement );
break;

case 'showstatus':
$this->return_value = $this->show_status_workaround( $statement );
break;

case 'drop_index':
$this->return_value = false;
if ( preg_match( '/^\\s*(DROP\\s*INDEX\\s*.*?)\\s*ON\\s*(.*)/im', $statement, $match ) ) {
$this->query_type = 'alter';
$this->return_value = $this->execute_alter_query( 'ALTER TABLE ' . trim( $match[2] ) . ' ' . trim( $match[1] ) );
} while ( $error );

$stmt = null;
$last_retval = null;
foreach ( $translation->queries as $query ) {
$this->queries[] = "Executing: {$query->sql} | " . ( $query->params ? 'parameters: ' . implode( ', ', $query->params ) : '(no parameters)' );
do {
$error = null;
try {
$stmt = $this->pdo->prepare( $query->sql );
$last_retval = $stmt->execute( $query->params );
} catch ( PDOException $error ) {
if ( $error->getCode() !== self::SQLITE_BUSY ) {
throw $error;
}
}
} while ( $error );
}
Copy link
Collaborator Author

@adamziel adamziel Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One MySQL query can translate into many SQLite queries so the entire translation/execution block should be inside of a transaction – otherwise we could end up with a half-executed sequence of SQLite queries.

There might be one active already, though, so beginTransaction() and friends should support nested transactions via SAVEPOINT and RELEASE

@aristath aristath marked this pull request as ready for review February 22, 2023 12:15
@aristath aristath merged commit 39fa104 into main Feb 23, 2023
adamziel added a commit to WordPress/wordpress-playground that referenced this pull request Mar 17, 2023
Migrates Playground from regexp-based [wp-sqlite-db](aaemnnosttv/wp-sqlite-db#55) to the official [sqlite-database-integration](https://github.com/WordPress/sqlite-database-integration) plugin which [translates queries using SQL parser](WordPress/sqlite-database-integration#9) instead of regular expressions. 

This makes for a more reliable SQLite integration that passes almost all WordPress unit tests.

[Learn more](aaemnnosttv/wp-sqlite-db#55) about the problem with regular expressions.
@adamziel adamziel mentioned this pull request Mar 24, 2023
@batonac batonac mentioned this pull request Jul 18, 2023
@swissspidy swissspidy deleted the lexer-translation branch July 20, 2023 09:29
Pookie717 added a commit to Pookie717/wordpress-playground that referenced this pull request Oct 1, 2023
Migrates Playground from regexp-based [wp-sqlite-db](aaemnnosttv/wp-sqlite-db#55) to the official [sqlite-database-integration](https://github.com/WordPress/sqlite-database-integration) plugin which [translates queries using SQL parser](WordPress/sqlite-database-integration#9) instead of regular expressions. 

This makes for a more reliable SQLite integration that passes almost all WordPress unit tests.

[Learn more](aaemnnosttv/wp-sqlite-db#55) about the problem with regular expressions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants