Column names can include spaces and other non 'w' characters

[yewtc]

MSSQL allows spaces (and other non-word characters) in column names. At the point that this code is executed we are looking at column names and not accessors,

[dboehmer]

While this regex is not DBMS-specific all new allowed characters (all of \W) seem to be relevant for MS-SQL only. So I suggest to still limit the allowed characters to those allowed in MS-SQL.

I took a quick look at https://docs.microsoft.com/en-us/sql/odbc/microsoft/column-name-limitations?view=sql-server-2017

• ` is not allowed
• | is not allowed

So I suggest [^`|]+.

[ribasushi]

On 11/22/2018 07:39 AM, Daniel Böhmer wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In lib/DBIx/Class/ResultSource.pm <#129 (comment)>: > @@ -1531,7 +1531,7 @@ sub reverse_relationship_info { sub __strip_relcond { +{ map - { map { /^ (?:foreign|self) \. (\w+) $/x } ($_, $_[1]{$_}) } + { map { /^ (?:foreign|self) \. (.+) $/x } ($_, $_[1]{$_}) } While this regex is not DBMS-specific all new allowed characters (all of |\W|) seem to be relevant for MS-SQL only. So I suggest to still limit the allowed characters to those allowed in MS-SQL. I took a quick look at https://docs.microsoft.com/en-us/sql/odbc/microsoft/column-name-limitations?view=sql-server-2017 * |`| is not allowed * ||| is not allowed So I suggest |[^`|]+|.

Apologies for nto replying earlier to this. Generally there should be *no* limitations in this specific portion of the codebase, the presence of \w+ is an oversight, and the original PR is correct. However - there are a few other places that need a similar update, and as the PR came with no tests of any sort - the spots got missed :( I will circle back to this before the end of the year, I promise.

[yewtc]

Sorry about not replying to your reply for some time. Anyway.
I take the point about about looking at valid MSSQL column name characters, but it might happen that pipe and back tick are valid in other sqls. I think that mysql could include either it quoted.

[pdl]

Looks like postgres allows everything except NUL in column names.

SELECT 'pipe' AS "|", 'backtick' AS "`", 'backslash' AS "\", 'TAB' AS U&"\0009"; -- ok
SELECT 'NUL' AS U&"\0000"; -- fails

[ribasushi]

Folks, the issue here isn't how to fix the code - I already indicated that doing .+ above was the right thing.

However since I wrote my last comment I realized I already went via a completely different approach in the devel branch a bit ago in 86be9bc. I will try to put out a new trial release addressing the perf. issue holding this back and have you folks give it a spin.

Sorry for the silence on this one, I could have used a less stressful set of holidays :(

[ribasushi]

Folks, the issue here isn't how to fix the code

I should have clarified this further: the reason this particular PR was left open was detailed earlier in #129 (comment) :

...However - there are a few other places that need a similar update, and as the PR came with no tests of any sort - the spots got missed :(

Even though there is a different implementation to be released soon which solves the problem by accident, it would still be good to have a test for this. So if time permits please contribute a test to go with the minimal changeset by @yewtc above.

Thanks!