fix: simulate query matched rows wrong count by kamioon · Pull Request #18536 · phpmyadmin/phpmyadmin

kamioon · 2023-07-16T20:59:42Z

This is a bug fix for #18533
The <> operator excludes NULL values. read more

Signed-off-by: kamioon <kamran.azari@gmail.com>

codecov · 2023-07-16T21:34:33Z

Codecov Report

Patch coverage has no change and project coverage change: -0.27 ⚠️

Comparison is base (aa3496f) 48.02% compared to head (824f850) 47.76%.

❗ Current head 824f850 differs from pull request most recent head 1551484. Consider uploading reports for the commit 1551484 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##             QA_5_2   #18536      +/-   ##
============================================
- Coverage     48.02%   47.76%   -0.27%     
+ Complexity    17011    17010       -1     
============================================
  Files           607      607              
  Lines         72342    72339       -3     
============================================
- Hits          34741    34550     -191     
- Misses        37601    37789     +188

Flag	Coverage Δ
dbase-extension	`48.58% <0.00%> (-0.02%)`	⬇️
recode-extension	`48.06% <0.00%> (-0.03%)`	⬇️
unit-7.2-ubuntu-latest	`48.06% <0.00%> (+<0.01%)`	⬆️
unit-7.3-ubuntu-latest	`48.51% <0.00%> (+0.02%)`	⬆️
unit-7.4-ubuntu-latest	`48.46% <0.00%> (-0.04%)`	⬇️
unit-8.0-ubuntu-latest	`48.50% <0.00%> (-0.03%)`	⬇️
unit-8.1-ubuntu-latest	`48.49% <0.00%> (-0.81%)`	⬇️
unit-8.2-ubuntu-latest	`48.53% <0.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
libraries/classes/Import/SimulateDml.php	`53.33% <0.00%> (-30.80%)`	⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

liviuconcioiu · 2023-07-17T17:17:43Z

libraries/classes/Import/SimulateDml.php

-                $notEqualOperator = ' IS NOT ';
+                $diff[] = $set->column . ' IS NOT ' . $set->value;
+            }else{
+                $diff[] = '(' . $set->column . ' <> ' . $set->value . ' OR ' . $set->column . ' IS NULL)';


I would get rid of both parentheses here, since aren't need. Examples of generated queries with and without parentheses.

SELECT `b` FROM `test` WHERE `a` = 2 AND ((`b` <> 'test' OR `b` IS NULL)) SELECT `b` FROM `test` WHERE `a` = 2 AND (`b` <> 'test' OR `b` IS NULL) SELECT `b`, `c` FROM `test` WHERE `a` = 1 AND ((`b` <> 'test' OR `b` IS NULL) OR (`c` <> 'test' OR `c` IS NULL)) SELECT `b`, `c` FROM `test` WHERE `a` = 1 AND (`b` <> 'test' OR `b` IS NULL OR `c` <> 'test' OR `c` IS NULL) SELECT `b`, `c` FROM `test` WHERE `a` = 2 AND (`b` IS NOT NULL OR (`c` <> 'test' OR `c` IS NULL)) SELECT `b`, `c` FROM `test` WHERE `a` = 2 AND (`b` IS NOT NULL OR `c` <> 'test' OR `c` IS NULL)

It makes sense. considering that they are all OR, it is not really necessary. I've removed them.

libraries/classes/Import/SimulateDml.php

add space before and after the else keyword Signed-off-by: kamioon <kamran.azari@gmail.com>

MoonE · 2023-07-17T20:33:55Z

libraries/classes/Import/SimulateDml.php

            if (strtoupper($set->value) === 'NULL') {
-                $notEqualOperator = ' IS NOT ';
+                $diff[] = $set->column . ' IS NOT ' . $set->value;
+            } else {
+                $diff[] = $set->column . ' <> ' . $set->value . ' OR ' . $set->column . ' IS NULL';
            }


Some love for the MySQL <=> operator?

Suggested change

if (strtoupper($set->value) === 'NULL') {

$notEqualOperator = ' IS NOT ';

$diff[] = $set->column . ' IS NOT ' . $set->value;

} else {

$diff[] = $set->column . ' <> ' . $set->value . ' OR ' . $set->column . ' IS NULL';

}

$diff[] = 'NOT ' . $set->column . ' <=> (' . $set->value . ')';

It broke my brain 😂
Thanks for sharing it

Thank you @MoonE ! I kept the additional parentheses that you have added it seems to keep the value safe. but are there any special conditions without them that ruin the query?

This can happen with an update query like

UPDATE a SET b = c OR d WHERE e OR f

It would generate NOT b <=> c OR d instead of NOT b <=> (c OR d)

With that in mind the original condition should also be set in parenthesis:

if (! empty($diff)) { - $where .= ' AND (' . implode(' OR ', $diff) . ')'; + $where = '(' . $where . ') AND (' . implode(' OR ', $diff) . ')'; }

to generate the condition
WHERE (e OR f) AND (NOT b <=> (c OR d)) instead of
WHERE e OR f AND (NOT b <=> (c OR d))

Signed-off-by: kamioon <kamran.azari@gmail.com>

williamdes

This looks good

MoonE · 2023-07-19T21:56:43Z

@kamioon You also need to adjust the test here

phpmyadmin/test/classes/Import/SimulateDmlTest.php

Lines 51 to 54 in dd5d340

    
           'update statement' => [ 
        
               'UPDATE `table_1` SET `id` = 20 WHERE `id` > 10', 
        
               'SELECT `id` FROM `table_1` WHERE `id` > 10 AND (`id` <> 20)', 
        
           ],

and also modify the same query here

phpmyadmin/test/classes/Stubs/DbiDummy.php

Lines 2231 to 2235 in dd5d340

    
           [ 
        
               'query' => 'SELECT `id` FROM `table_1` WHERE `id` > 10 AND (`id` <> 20)', 
        
               'columns' => ['id'], 
        
               'result' => [['11'], ['12']], 
        
           ],

@williamdes @MauricioFauth Unit tests still pass because the test is marked incomplete here

phpmyadmin/test/classes/Stubs/DbiDummy.php

Line 135 in dd5d340

    
           Assert::markTestIncomplete('Non expected select of database: ' . $databaseName);

and PHPUnit is not run with --fail-on-incomplete / failOnIncomplete="true"

Signed-off-by: kamioon <kamran.azari@gmail.com>

MoonE

For what it's woth, I think this is ready.

bugfix for simulate query matched rows wrong count

2b8d793

Signed-off-by: kamioon <kamran.azari@gmail.com>

liviuconcioiu reviewed Jul 17, 2023

View reviewed changes

remove extra parentheses

a20e78a

add space before and after the else keyword Signed-off-by: kamioon <kamran.azari@gmail.com>

MoonE reviewed Jul 17, 2023

View reviewed changes

replaced with suggested commit from @MoonE with the <=> operator

824f850

Signed-off-by: kamioon <kamran.azari@gmail.com>

williamdes approved these changes Jul 18, 2023

View reviewed changes

williamdes requested a review from MauricioFauth July 18, 2023 07:14

MoonE mentioned this pull request Jul 19, 2023

Fail with incomplete test #18548

Merged

kamioon added 3 commits July 21, 2023 11:28

Merge branch 'phpmyadmin:QA_5_2' into QA_5_2

c5902e8

add parentheses for the main where condition in simulated update query

41ca884

Signed-off-by: kamioon <kamran.azari@gmail.com>

update tests for the simulated update query changes

1551484

Signed-off-by: kamioon <kamran.azari@gmail.com>

MoonE approved these changes Jul 25, 2023

View reviewed changes

williamdes mentioned this pull request Jul 25, 2023

Simulate query matched rows wrong count #18533

Closed

MoonE mentioned this pull request Jul 26, 2023

Fix simulate query count #18583

Merged

MauricioFauth approved these changes Aug 26, 2023

View reviewed changes

MauricioFauth merged commit 135724f into phpmyadmin:QA_5_2 Aug 26, 2023

MauricioFauth self-assigned this Aug 26, 2023

MauricioFauth modified the milestones: 6.0.0, 5.2.2 Aug 26, 2023

M393 mentioned this pull request Jun 25, 2025

[Bug]: "Simulate Query" in phpMyAdmin 5.2.1 Incorrectly Reports No Rows Affected #19741

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: simulate query matched rows wrong count#18536

fix: simulate query matched rows wrong count#18536
MauricioFauth merged 6 commits intophpmyadmin:QA_5_2from
kamioon:QA_5_2

kamioon commented Jul 16, 2023 •

edited by williamdes

Loading

Uh oh!

codecov bot commented Jul 16, 2023 •

edited

Loading

Uh oh!

liviuconcioiu Jul 17, 2023

Uh oh!

kamioon Jul 17, 2023 •

edited

Loading

Uh oh!

Uh oh!

MoonE Jul 17, 2023 •

edited

Loading

Uh oh!

williamdes Jul 17, 2023

Uh oh!

kamioon Jul 18, 2023

Uh oh!

MoonE Jul 18, 2023

Uh oh!

williamdes left a comment

Uh oh!

MoonE commented Jul 19, 2023 •

edited

Loading

Uh oh!

MoonE left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

kamioon commented Jul 16, 2023 • edited by williamdes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liviuconcioiu Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

kamioon Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MoonE Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williamdes Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

kamioon Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

MoonE Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

williamdes left a comment

Choose a reason for hiding this comment

Uh oh!

MoonE commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonE left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kamioon commented Jul 16, 2023 •

edited by williamdes

Loading

codecov bot commented Jul 16, 2023 •

edited

Loading

kamioon Jul 17, 2023 •

edited

Loading

MoonE Jul 17, 2023 •

edited

Loading

MoonE commented Jul 19, 2023 •

edited

Loading