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

Fix #226 - Ignore end of comment string at the end of queries with MySQL-specific commands #424

Merged
merged 1 commit into from
Mar 11, 2023

Conversation

niconoe-
Copy link
Contributor

This is an attempt to fix #226 .

If it misses some use-cases to test, please tell me.

Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Thank you for working on the sql-parser 🚀
this fix only ignores the end, but without it what would the unit tests look like?
The queries where broken?

@niconoe-
Copy link
Contributor Author

niconoe- commented Mar 1, 2023

Thank you for working on the sql-parser 🚀
this fix only ignores the end, but without it what would the unit tests look like?
The queries where broken?

Kind of… if you look at the unit test defined in #226 , without the fix, the assertEquals fails because the content of the expression ends with the ˋ*/` token while it shouldn’t.
I don’t know how this could be interpreted but I guess if this is reused in the builder, it will create an invalid query.

@williamdes
Copy link
Member

Kind of… if you look at the unit test defined in #226 , without the fix, the assertEquals fails because the content of the expression ends with the ˋ*/` token while it shouldn’t.
I don’t know how this could be interpreted but I guess if this is reused in the builder, it will create an invalid query.

Oh okay, could you add a regression test for this ?

@williamdes williamdes added this to the 5.8.0 milestone Mar 9, 2023
Copy link
Member

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

Or maybe the current tests would fail without you fix ?

@niconoe-
Copy link
Contributor Author

niconoe- commented Mar 9, 2023

Or maybe the current tests would fail without you fix ?

That's is. Considering the unit test testMysqlCommands I added, it would fail if my fix isn't provided.

Without my fix, it behaved exactly like the issue's OP described, while with my fix, it's now OK.

@williamdes
Copy link
Member

Or maybe the current tests would fail without you fix ?

That's is. Considering the unit test testMysqlCommands I added, it would fail if my fix isn't provided.

Without my fix, it behaved exactly like the issue's OP described, while with my fix, it's now OK.

Awesome, ready for a merge

@williamdes williamdes changed the title Fix #226: ignore end of comment string at the end of queries with MySQL-specific commands. Fix #226 - Ignore end of comment string at the end of queries with MySQL-specific commands. Mar 11, 2023
@williamdes williamdes changed the title Fix #226 - Ignore end of comment string at the end of queries with MySQL-specific commands. Fix #226 - Ignore end of comment string at the end of queries with MySQL-specific commands Mar 11, 2023
@williamdes williamdes self-assigned this Mar 11, 2023
…es with MySQL-specific commands

Co-Authored-By: William Desportes <williamdes@wdes.fr>
@williamdes williamdes changed the base branch from master to 5.8.x March 11, 2023 13:45
williamdes added a commit that referenced this pull request Mar 11, 2023
Pull-request: #424

Signed-off-by: William Desportes <williamdes@wdes.fr>
@williamdes williamdes merged commit 76b8ae1 into phpmyadmin:5.8.x Mar 11, 2023
@williamdes
Copy link
Member

Thank you, I rebased your work after adding more test cases in: 049ed2d

@niconoe- niconoe- deleted the fix-226 branch June 21, 2023 13:19
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.

Parser leaves comment string at the end of the query in mysql-specific comment
2 participants