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

Rebuilt query is bad #301

Closed
williamdes opened this issue May 1, 2020 · 7 comments
Closed

Rebuilt query is bad #301

williamdes opened this issue May 1, 2020 · 7 comments
Assignees
Milestone

Comments

@williamdes
Copy link
Member

        $parser = new Parser(
            'CREATE DEFINER=`root`@`%` PROCEDURE `foo`( $foo int )' . "\n" . 'select $foo'
        );
        $stmt = $parser->statements[0];
       // BAD !!!!!!!!!
        $this->assertEquals(
            'CREATE DEFINER=`root`@`%` PROCEDURE `foo` (`$` FOO, ``)  select $foo',
            $stmt->build()
        );
@williamdes
Copy link
Member Author

array(2) {
  [0] =>
  class PhpMyAdmin\SqlParser\Components\ParameterDefinition#20668 (3) {
    public $name =>
    string(1) "$"
    public $inOut =>
    NULL
    public $type =>
    class PhpMyAdmin\SqlParser\Components\DataType#20682 (3) {
      public $name =>
      string(3) "FOO"
      public $parameters =>
      array(0) {
        ...
      }
      public $options =>
      class PhpMyAdmin\SqlParser\Components\OptionsArray#20692 (1) {
        ...
      }
    }
  }
  [1] =>
  class PhpMyAdmin\SqlParser\Components\ParameterDefinition#20693 (3) {
    public $name =>
    NULL
    public $inOut =>
    NULL
    public $type =>
    NULL
  }
}


Time: 00:00.970, Memory: 22.00 MB

There was 1 failure:

1) PhpMyAdmin\SqlParser\Tests\Components\ParameterDefinitionTest::testParseComplex
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'$foo'
+'$'

In sql-parser/tests/Components/ParameterDefinitionTest.php

The test does not pass and the ParameterDefinition detects another parameter

    public function testParseComplex()
    {
        $parser = new Parser(
            //'CREATE DEFINER=`root`@`%` PROCEDURE `$a`(IN `$a` INT)' . "\n" . '    NO SQL' . "\n" . 'SELECT $a'
        );
        $component = ParameterDefinition::parse(
            $parser,
            $this->getTokensList('CREATE DEFINER=`root`@`%` PROCEDURE `foo`( $foo int )')
        );
        var_dump($component);
        $this->assertEquals('$foo', $component[0]->name);

    }

@williamdes
Copy link
Member Author

@Tithugues Would you want to have a look at this one ?

@Tithugues
Copy link
Contributor

@williamdes , is there any unit test written with this test case please? (Even marked as skipped for now or incomplete.)

I'll see if I find any time to fix this.

Thank you.

@williamdes
Copy link
Member Author

@Tithugues no, only the code I pasted here needs to be used to create new tests

Thank you :)

@Tithugues
Copy link
Contributor

🤞

@Tithugues
Copy link
Contributor

I had to update some unit tests:

  • parseCreateFunctionErr3: the input was incorrect (if I'm right)
  • ErrorTest was using a "$" as an unexpected character… unfortunately, this is the one that can be used as name now…

I hope this will be OK. There are many unit tests on this project. And no one broke with this modification.

@williamdes
Copy link
Member Author

@Tithugues The commit seems right but I would add one more check in the test to be sure there is only one ParameterDefinition and not more than one because this triggers a bug in phpMyAdmin when I want to export the procedure on 5.1
because some code in the ParameterDefinition will str_replace name NULL and it will crash.

Long story short, a new assertion would be great to be sure only one ParameterDefinition` is found.
Bad:

[1] =>
  class PhpMyAdmin\SqlParser\Components\ParameterDefinition#20693 (3) {
    public $name =>
    NULL
    public $inOut =>
    NULL
    public $type =>
    NULL
  }

@williamdes williamdes self-assigned this May 2, 2020
@williamdes williamdes added this to the 4.6.2 milestone May 2, 2020
williamdes added a commit that referenced this issue May 5, 2020
Pull-request: #302
Fixes: #301

Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit that referenced this issue May 5, 2020
Signed-off-by: William Desportes <williamdes@wdes.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants