Allow escaped column and table names in postgres. #450
Conversation
| String qry = " select column_name, data_type, character_maximum_length " | ||
| + " as direction from information_schema.columns where "; | ||
|
|
||
| if (qualifiers.length == 2) { | ||
| qry += " lower(table_schema)=? and lower(table_name)=? "; | ||
| qry += " table_schema=? and table_name=? "; |
There was a problem hiding this comment.
Are we making tables case-sensitive here? Are the other tests still working fine, will both below succeed:
|update|users|
|username=|name|
|adent2|arthur dent|
|UPDATE|USERS|
|USERNAME=|NAME|
|adent2|arthur dent|
There was a problem hiding this comment.
Just to put in correct way if you create table or columns without " postgres automatically convert it to lower case adn then it will by stored in information schema. I added that test into test suite.
There was a problem hiding this comment.
Just to put in correct way if you create table or columns without " postgres automatically convert it to lower case adn then it will by stored in information schema. I added that test into test suite.
Have you pushed that test, I don't see it yet?
There was a problem hiding this comment.
OK... Update actually works a bit different than e.g. Insert. It's interesting to check also Insert with both upper and lower case - maybe in the same test can be split to check both scenarios:
|insert|users|
|name|username|
|arthur dent|adent|
|INSERT|USERS|
|NAME|USERNAME|
|ford prefect|fpref|
|zaphod beeblebrox|zaphod|
Will this work?
There was a problem hiding this comment.
I pushed modified test. For lower case is everything in SimpleUpdate all modification upper case are in SimpleUpdate2
|
Thank you @igieon for submitting this pull request! Looks like a useful feature, just needs some polishing. I just posted a few comments inline. |
| @@ -74,7 +74,7 @@ protected String parseCommandText(String commandText) { | |||
| paramName = ""; | |||
| String dataType = rs.getString(2); | |||
| DbParameterAccessor dbp = createDbParameterAccessor( | |||
| paramName, | |||
| '"' + paramName + '"', | |||
There was a problem hiding this comment.
I wonder if we can avoid escaping that here (since it has always been the original name unmodified)?
There was a problem hiding this comment.
I just added one new test for this. We need to put there escapped character because its column name and we need to escape collumn name, because in information schema is alway unescapped stored. Meaning that if you use unexcapped character is column always stored as lower case and we can escaped it.
There was a problem hiding this comment.
I just added one new test for this. We need to put there escapped character because its column name and we need to escape collumn name, because in information schema is alway unescapped stored. Meaning that if you use unexcapped character is column always stored as lower case and we can escaped it.
Yes, but what I was wondering is if we can do the escaping somewhere else? I guess for buildInsertCommand we can easily handle since it's inside environment class; for Update - hmm, maybe we can introduce some general ability for case-sensitive escaping (there is similar issue for other database types). I'm not sure if anything else is affected?
There was a problem hiding this comment.
For me when i look into code everything could be change if we modify NameNormaliser and little refactor code around it. As i look now my change doesn't affect weird function parameters. I want just implement this case, because we need to transform old weird naming schema to new one understandable views and we want to have covered with tests ;-).
There was a problem hiding this comment.
Right. I think the current change is well isolated (only pgsql code is affected) and is working good enough. We can go with it as initial version and think of possible refactoring later.
|
Merging... @igieon, thanks a lot for that contribution! |
|
Just a final touch: @igieon, could you squash your commits into single one before we merge the PR? |
|
Squashed. Just the question when you make new release ? |
Thanks! Can we just have the commit message description refined a bit (says
I think we can make it pretty soon. Do you have any preference? |
- allow table names like "Pers+""+Pos" in PostgresSQL - allow column names like "Info""+desc" - look usage in tests DbFit/AcceptanceTests/JavaTests/PostgresTests/FlowMode/SimpleUpdateEscaped.html
|
Could be like that ? |
|
Yes, it looks good now 👍 . Thank you! |
Allow escaped column and table names in postgres.
Allow to have table names like public."Pos++Pers" and column names like "Pers++Pos+id"