Make sure keywords are single upper words #107

mvorisek · 2024-05-17T10:39:12Z

Into 1.4.x as this is needed for #106 bugfix.

In this PR, we make sure all "reserved" keywords are single word - this is how SQL specifications (MySQL/MariaDB/SQLite at least) define them.

The "reserved" keywords are special as they need to be always quoted*. The "reserved" keyword list will be updated in #106 to contain all and only the "reserved" keywords.

* this is not only helpful for maintaining the lists properly, but also needed for #73 as only "reserved" keywords can be upper cased safely (issue #95).

tests/TokenizerTest.php

tests/clihighlight.txt

greg0ire · 2024-05-17T21:30:18Z

Changes are explained by added tests & commit messages.

Changes are described, but they are not explained by commit messages (maybe by tests, but that's not sufficient I'm afraid). For instance, what makes you think keywords appearing in reservedTopLevel must also appear in reserved?

mvorisek · 2024-05-17T21:35:34Z

what makes you think keywords appearing in reservedTopLevel must also appear in reserved?

"reserved" words are special - they needs to be quoted even if used on places where only identifier is possible (ex select x as AND imply a parse error for both MySQL and SQLite). This PR is cleanup of current code before #106 is landed.

In #106, "reserved" words will be asserted by complete lists.

greg0ire · 2024-05-18T08:13:25Z

I'm asking because reserved and derived properties seems mostly used inside this code block, where being part of reserved does not seem to do much if you are already part of reservedTopLevel or reservedNewline:

sql-formatter/src/Tokenizer.php

Lines 887 to 932 in 6eb85e0

    
           // A reserved word cannot be preceded by a '.' 
        
           // this makes it so in "mytable.from", "from" is not considered a reserved word 
        
           if (! $previous || $previous->value() !== '.') { 
        
               $upper = strtoupper($string); 
        
               // Top Level Reserved Word 
        
               if ( 
        
                   preg_match( 
        
                       '/^(' . $this->regexReservedToplevel . ')($|\s|' . $this->regexBoundaries . ')/', 
        
                       $upper, 
        
                       $matches, 
        
                   ) 
        
               ) { 
        
                   return new Token( 
        
                       Token::TOKEN_TYPE_RESERVED_TOPLEVEL, 
        
                       substr($string, 0, strlen($matches[1])), 
        
                   ); 
        
               } 
        
               // Newline Reserved Word 
        
               if ( 
        
                   preg_match( 
        
                       '/^(' . $this->regexReservedNewline . ')($|\s|' . $this->regexBoundaries . ')/', 
        
                       $upper, 
        
                       $matches, 
        
                   ) 
        
               ) { 
        
                   return new Token( 
        
                       Token::TOKEN_TYPE_RESERVED_NEWLINE, 
        
                       substr($string, 0, strlen($matches[1])), 
        
                   ); 
        
               } 
        
               // Other Reserved Word 
        
               if ( 
        
                   preg_match( 
        
                       '/^(' . $this->regexReserved . ')($|\s|' . $this->regexBoundaries . ')/', 
        
                       $upper, 
        
                       $matches, 
        
                   ) 
        
               ) { 
        
                   return new Token( 
        
                       Token::TOKEN_TYPE_RESERVED, 
        
                       substr($string, 0, strlen($matches[1])), 
        
                   ); 
        
               } 
        
           }

Is there a part of the code where being part of reserved matters even if you are already part of the two other properties?

mvorisek · 2024-05-18T09:01:22Z

I'm asking because reserved and derived properties seems mostly used inside this code block, where being part of reserved does not seem to do much if you are already part of reservedTopLevel or reservedNewline:

I have now removed the test I added testing if all new line/top level keywords are also reserved, as in theory, this does not have to be true.

Currently the lists are in quite bad shape, for ex. WITH keyword is already present in reservedToplevel but also in reserved.

Reserved keywords should be maintained however on its own and they will be asserted in #106, the reason is "reserved" keyworda are special as they must be always quoted - and they will be needed to address #95. "reserved top level" and "reserved new line" are just bad historical names.

mvorisek · 2024-05-21T10:04:22Z

@greg0ire I have now amended the commits with explanation messages.

mvorisek · 2024-05-21T18:18:08Z

@greg0ire I would like to continue with #106, please, can you review this bugfix PR?

SenseException · 2024-05-21T22:38:27Z

tests/TokenizerTest.php

+    public function testKeywordsReservedAreUpperCasedAndWithoutWhitespace(): void
+    {
+        $tokenizerReserved = $this->getTokenizerList('reserved');
+
+        $kwsDiff = array_filter($tokenizerReserved, static function ($v) {
+            return $v !== strtoupper($v) || preg_match('~\s~', $v) !== 0;
+        });
+
+        self::assertSame([], $kwsDiff);
+    }


What is this method supposed to test? This is checking the format of the reserved keywords.

All reserved keywords must be a single word in upper case (so we can rely on it when comparing againts other UC string).

goetas

Overall I do not see the need for this change (except of we want to keep the array of keywords alphabetically sorted)

goetas · 2024-05-22T11:31:01Z

tests/TokenizerTest.php

+        return $res;
+    }
+
+    public function testKeywordsReservedAreUpperCasedAndWithoutWhitespace(): void


I think that this test is testing something "too" internal. they way how properties are used in the class should not be IMO really tested. changing the implementation at this level of depth should not really impact tests IMO

explained in #107 (comment)

src/Tokenizer.php

goetas · 2024-05-22T14:45:55Z

@mvorisek just some feedback, The commit 2f84308 makes it very hard to review your changes...

i see a lot of small changes even in the other two commits about sorting variables in the arrays that honestly add little value and makes the reviewer work much harder

goetas · 2024-05-22T14:51:42Z

Would it be possible to isolate the "real" changes you did from the "esthetical" changes?

mvorisek · 2024-05-22T15:01:03Z

@goetas sure - https://github.com/doctrine/sql-formatter/compare/2f843086...54731080?expand=1

mvorisek · 2024-05-25T06:26:36Z

@greg0ire the sorting PR (#117) was merged, can this PR be merged as well?

greg0ire · 2024-05-25T09:31:22Z

I don't know if @goetas @derrabus and @SenseException are satisfied with your answers. Since you can't get approvals, I am closing this PR and I suggest you do one PR per commit, so that there is more hope to close open discussions, and so that we do not have to look at too many other PRs and issue to understand what you are trying to do.

In these new PRs, try to be less pushy, and to explains things better, with clearer sentences. Take your time. I'll give you an example:

to be able to use faster CS comparison, they are always CI

Here it is unclear if you are referring to things before or after your changes, and it is unclear what you are referring to with "they".

mvorisek · 2024-05-26T06:04:19Z

@greg0ire please reopen this PR and I will extract the 2nd commit into another PR. Thank you.

mvorisek · 2024-05-30T07:37:35Z

PR is done and I did my best to explain the change in the PR description.

greg0ire · 2024-05-30T11:54:00Z

src/Tokenizer.php

@@ -182,13 +182,10 @@ final class Tokenizer
        'MYISAM',
        'NAMES',
        'NATURAL',
-        'NO OTHERS',


I think I might not be understanding what you are doing here… shouldn't you be adding back NO as well as OTHERS to this list?

NO or OTHERS must not be added into "reserved" keywords as both are not "reserved" by SQL/MySQL spec.

Make sure keywords are single upper words

Your commit message does not mention that at any point.

The PR description does. If the commit message should be amended, please tell me what text to put there.

Well maybe it should be a separate commit, since it's an entirely separate concern.

We need to end up exactly with the changes as this PR does - please tell me exactly what to do, am I right you want separate commit for 'NO OTHERS' keyword removal? Anything else?

2 commits:

one that removes NO OTHERS and explains why, and also takes a guess at why NO OTHERS was present in this list in the first place

another that is about making sure we deal with single words, and also explains why that is necessary.

Done and thank you.

Why do you think NO OTHERS was present in the list in the first place? You cite MySQL grammar, but this library is not solely about MySQL, is it?

Why do you think NO OTHERS was present in the list in the first place?

See https://github.com/search?q=repo%3Amysql%2Fmysql-server+%22NO+OTHERS%22&type=code MySQL tests for usecases and https://github.com/antlr/grammars-v4/blob/4264f19a7d/sql/mysql/Oracle/MySQLParser.g4#L1208 for full grammar.

It is however not a "reserved" keyword because of two reasons. Keyword must be a single word. And must be listed as reserved in MySQL or MariaDB docs.

You cite MySQL grammar, but this library is not solely about MySQL, is it?

The jdorm library was for MySQL dialect only. In #106 many keywords will be added as the current list is far from complete.

"NO" or "OTHERS" are not reserved keywords per MySQL grammar

The "reserved" keywords are special as they need to be always quoted and per MySQL/SQLite spec they are always single word and case insensitive.

greg0ire · 2024-05-30T15:48:25Z

Thanks @mvorisek !

mvorisek force-pushed the fix_reserved_words branch from 8bd332e to 78c8eee Compare May 17, 2024 11:30

mvorisek marked this pull request as ready for review May 17, 2024 11:31

mvorisek mentioned this pull request May 17, 2024

Add all "type reserved" MySQL keywords #106

Merged

greg0ire reviewed May 17, 2024

View reviewed changes

tests/TokenizerTest.php Outdated Show resolved Hide resolved

greg0ire reviewed May 17, 2024

View reviewed changes

tests/clihighlight.txt Show resolved Hide resolved

mvorisek requested a review from greg0ire May 17, 2024 19:36

mvorisek force-pushed the fix_reserved_words branch 2 times, most recently from 790bb74 to c03edef Compare May 17, 2024 21:09

mvorisek force-pushed the fix_reserved_words branch 2 times, most recently from 2c9a539 to 3eff766 Compare May 18, 2024 08:54

mvorisek marked this pull request as draft May 21, 2024 09:50

mvorisek force-pushed the fix_reserved_words branch from 3eff766 to 5473108 Compare May 21, 2024 10:01

mvorisek marked this pull request as ready for review May 21, 2024 10:02

greg0ire approved these changes May 21, 2024

View reviewed changes

greg0ire requested review from goetas, SenseException and derrabus May 21, 2024 21:03

SenseException reviewed May 21, 2024

View reviewed changes

mvorisek requested a review from SenseException May 21, 2024 23:16

goetas reviewed May 22, 2024

View reviewed changes

mvorisek requested a review from goetas May 22, 2024 11:40

mvorisek force-pushed the fix_reserved_words branch 2 times, most recently from df99b3a to a47492d Compare May 24, 2024 07:49

mvorisek marked this pull request as ready for review May 24, 2024 07:50

mvorisek requested review from greg0ire and goetas May 24, 2024 07:55

greg0ire closed this May 25, 2024

greg0ire reopened this May 26, 2024

mvorisek marked this pull request as draft May 30, 2024 07:17

mvorisek force-pushed the fix_reserved_words branch from a47492d to cbe343f Compare May 30, 2024 07:18

mvorisek changed the title ~~Fix reserved keywords lists~~ Make sure keywords are single upper words May 30, 2024

mvorisek marked this pull request as ready for review May 30, 2024 07:28

mvorisek force-pushed the fix_reserved_words branch from cbe343f to 229f681 Compare May 30, 2024 07:32

mvorisek marked this pull request as draft May 30, 2024 07:34

mvorisek force-pushed the fix_reserved_words branch from 229f681 to 58998dd Compare May 30, 2024 07:35

mvorisek marked this pull request as ready for review May 30, 2024 07:36

greg0ire reviewed May 30, 2024

View reviewed changes

mvorisek added 2 commits May 30, 2024 16:45

Remove "NO OTHERS" from reserved keywords

53833c7

"NO" or "OTHERS" are not reserved keywords per MySQL grammar

Make sure keywords are single upper words

ff667cc

The "reserved" keywords are special as they need to be always quoted and per MySQL/SQLite spec they are always single word and case insensitive.

mvorisek force-pushed the fix_reserved_words branch from 58998dd to ff667cc Compare May 30, 2024 14:48

greg0ire approved these changes May 30, 2024

View reviewed changes

greg0ire added this to the 1.4.1 milestone May 30, 2024

greg0ire merged commit 4605e91 into doctrine:1.4.x May 30, 2024
10 checks passed

mvorisek deleted the fix_reserved_words branch May 30, 2024 16:24

greg0ire added the bug Something isn't working label Aug 5, 2024

github-actions bot mentioned this pull request Aug 5, 2024

Merge release 1.4.1 into 1.5.x #135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure keywords are single upper words #107

Make sure keywords are single upper words #107

mvorisek commented May 17, 2024 •

edited

Loading

greg0ire commented May 17, 2024

mvorisek commented May 17, 2024

greg0ire commented May 18, 2024

mvorisek commented May 18, 2024 •

edited

Loading

mvorisek commented May 21, 2024

mvorisek commented May 21, 2024

SenseException May 21, 2024

mvorisek May 21, 2024

goetas left a comment

goetas May 22, 2024

mvorisek May 22, 2024

goetas commented May 22, 2024 •

edited

Loading

goetas commented May 22, 2024

mvorisek commented May 22, 2024

mvorisek commented May 25, 2024

greg0ire commented May 25, 2024

mvorisek commented May 26, 2024 •

edited

Loading

mvorisek commented May 30, 2024

greg0ire May 30, 2024

mvorisek May 30, 2024

greg0ire May 30, 2024

mvorisek May 30, 2024

greg0ire May 30, 2024

mvorisek May 30, 2024

greg0ire May 30, 2024

mvorisek May 30, 2024

greg0ire May 30, 2024

mvorisek May 30, 2024 •

edited

Loading

greg0ire commented May 30, 2024

Make sure keywords are single upper words #107

Make sure keywords are single upper words #107

Conversation

mvorisek commented May 17, 2024 • edited Loading

greg0ire commented May 17, 2024

mvorisek commented May 17, 2024

greg0ire commented May 18, 2024

mvorisek commented May 18, 2024 • edited Loading

mvorisek commented May 21, 2024

mvorisek commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetas commented May 22, 2024 • edited Loading

goetas commented May 22, 2024

mvorisek commented May 22, 2024

mvorisek commented May 25, 2024

greg0ire commented May 25, 2024

mvorisek commented May 26, 2024 • edited Loading

mvorisek commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek May 30, 2024 • edited Loading

Choose a reason for hiding this comment

greg0ire commented May 30, 2024

mvorisek commented May 17, 2024 •

edited

Loading

mvorisek commented May 18, 2024 •

edited

Loading

goetas commented May 22, 2024 •

edited

Loading

mvorisek commented May 26, 2024 •

edited

Loading

mvorisek May 30, 2024 •

edited

Loading