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

[sql-hint] identifier quote is not escaped if identifiers contain it - or spaces #4626

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

crazy4chrissi
Copy link

@crazy4chrissi crazy4chrissi commented Mar 4, 2017

There are several bugs in sql-hint. The following table / column names don't work correctly. Assume the identifier quote is ", then the identifier some"name needs to be escaped as "some""name".

Also, spaces cause problems: Assume a table is called table name and we start auto completion after "table na, we don't get "table name" but all na*hints.

I wrote several failing tests that show this problem contained in this pull request. Currently, this pull request fixes all except two of the failing tests. Maybe fixing the remaining two is more easy for you than me. It seems it would require me to dig deep into CodeMirror to understand what's going on.

…s not correctly escape column identifiers in table names, column names etc. For example a table name back`tick needs to be escaped like this: `back``tick`. Same for double quotes as column identifiers.
…amed my"table are now doublicated correctly (e.g. quoting produces: "my""table"). This fixes 2 failing tests, but 2 others still fail.
@crazy4chrissi
Copy link
Author

(continuous-integration fails because this pull request introduces two new tests regarding bugs that it does not fix yet)

@marijnh
Copy link
Member

marijnh commented Mar 4, 2017

Yeah, sql-hint is a mess and I regret ever having merged it. Oh well. Do you intend to work on fixing the other failures? I'd prefer not to have our CI red because of this.

@crazy4chrissi
Copy link
Author

Haha, okay. I can't believe the phpMyAdmin developers use codemirror with sql-hint and don't bother about such issues. Anyway, I will try to fix the other two issues. But it will take more time I guess because I need to better understand the way the hinter works to find where the problem is.
Maybe you can already merge the fixes provided in this pull request without the failing tests. Then I will create a new pull request once the other 2 are fixed.

@marijnh
Copy link
Member

marijnh commented Mar 4, 2017

I need to better understand the way the hinter works to find where the problem is.

It might, depending on how much effort you want to put into this, be more productive to rewrite the thing than to try and salvage it.

I've merged 7c87848 (minus the trailing whitespace) as 3c81665

@diegogvieira
Copy link

diegogvieira commented Apr 26, 2017

Please, consider this issue:

#4712

I already have a working code, I just do not know how to submit my changes.

Thanks

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.

3 participants