Skip to content

Add support for CURRENT_TIMESTAMP() calls with parentheses #151

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

Merged
merged 2 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions tests/WP_SQLite_Translator_Tests.php
Original file line number Diff line number Diff line change
Expand Up @@ -2859,6 +2859,46 @@ public function testDefaultNullValue() {
);
}

public function testCurrentTimestamp() {
// SELECT
$results = $this->assertQuery(
'SELECT
current_timestamp AS t1,
CURRENT_TIMESTAMP AS t2,
current_timestamp() AS t3,
CURRENT_TIMESTAMP() AS t4'
);
$this->assertIsArray( $results );
$this->assertCount( 1, $results );
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $results[0]->t1 );
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $results[0]->t2 );
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $results[0]->t3 );

// INSERT
$this->assertQuery(
"INSERT INTO _dates (option_name, option_value) VALUES ('first', CURRENT_TIMESTAMP())"
);
$results = $this->assertQuery( 'SELECT option_value AS t FROM _dates' );
$this->assertCount( 1, $results );
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $results[0]->t );

// UPDATE
$this->assertQuery( 'UPDATE _dates SET option_value = NULL' );
$results = $this->assertQuery( 'SELECT option_value AS t FROM _dates' );
$this->assertCount( 1, $results );
$this->assertEmpty( $results[0]->t );

$this->assertQuery( 'UPDATE _dates SET option_value = CURRENT_TIMESTAMP()' );
$results = $this->assertQuery( 'SELECT option_value AS t FROM _dates' );
$this->assertCount( 1, $results );
$this->assertRegExp( '/\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d/', $results[0]->t );

// DELETE
// We can only assert that the query passes. It is not guaranteed that we'll actually
// delete the existing record, as the delete query could fall into a different second.
$this->assertQuery( 'DELETE FROM _dates WHERE option_value = CURRENT_TIMESTAMP()' );
}

/**
* @dataProvider mysqlVariablesToTest
*/
Expand Down
18 changes: 17 additions & 1 deletion wp-includes/sqlite/class-wp-sqlite-translator.php
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,23 @@ public function get_return_value() {
* @throws Exception If the query is not supported.
*/
private function execute_mysql_query( $query ) {
$tokens = ( new WP_SQLite_Lexer( $query ) )->tokens;
$tokens = ( new WP_SQLite_Lexer( $query ) )->tokens;

// SQLite does not support CURRENT_TIMESTAMP() calls with parentheses.
// Since CURRENT_TIMESTAMP() can appear in most types of SQL queries,
// let's remove the parentheses globally before further processing.
foreach ( $tokens as $i => $token ) {
if ( WP_SQLite_Token::TYPE_KEYWORD === $token->type && 'CURRENT_TIMESTAMP' === $token->keyword ) {
$paren_open = $tokens[ $i + 1 ] ?? null;
$paren_close = $tokens[ $i + 2 ] ?? null;
if ( WP_SQLite_Token::TYPE_OPERATOR === $paren_open->type && '(' === $paren_open->value
&& WP_SQLite_Token::TYPE_OPERATOR === $paren_close->type && ')' === $paren_close->value ) {
unset( $tokens[ $i + 1 ], $tokens[ $i + 2 ] );
}
}
}
$tokens = array_values( $tokens );

Comment on lines +775 to +791
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code works as expected, but this seems like the wrong place to put it.
Ideally, we should clean up the brackets in the Lexer, but I couldn't find a good place to do it there.

@adamziel @aristath do you have any suggestions on where to put this code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bgrgicak I tried to move it here: 43b628d

Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This definitely better but it still feels like a "random" piece of code in that function.

I will let @adamziel and @aristath decide on this one as I don't know enough about the codebase to make this decision.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, I have the opposite feedback. This Lexer class is a direct port of the PHPMyAdmin Lexer and is meant to tokenize MySQL queries. parentheses are valid in MySQL and removing them in there sounds like an abstraction leak. Not that it matters with the AST Parser/Lexer explorations coming. I'm happy to get this change in as it is as I don't think we'll stick to the current lexer in the longer term, but if you were keen to relocate it then its original location in the Translator class seem fine to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamziel If you prefer the variant from the first commit, I can just drop the last one and repush, and we can go with that for now. I guess if the new approach replaces the old one, it can be still valuable to add the test case, and we can have the functionality earlier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please and agreed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamziel Thanks! Repushed.

$this->rewriter = new WP_SQLite_Query_Rewriter( $tokens );
$this->query_type = $this->rewriter->peek()->value;

Expand Down
Loading