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

[5.4] Can't Wrap DB Names Containing Spaces When Aliasing #17312

Merged
merged 2 commits into from
Jan 16, 2017
Merged

[5.4] Can't Wrap DB Names Containing Spaces When Aliasing #17312

merged 2 commits into from
Jan 16, 2017

Conversation

voidstate
Copy link
Contributor

Database names containing spaces are perfectly legal in Microsoft SQL Server.

Currently, when using SQL Server, if you have a database which includes spaces, the grammar handles wrapping it fine.

So this:

My Sample Database.dbo.my_table.my_column

Correctly becomes:

[My Sample Database].[dbo].[my_table].[my_column]

But if you also try to alias a column when the database contains spaces, it breaks.

This:

My Sample Database.dbo.my_table.my_column AS my_aliased_column

Becomes:

[My] AS [Sample]

I have traced the error to Database/Grammar/wrapAliasedValue() using explode( ' ' ) - the code thinks the spaces in the database name are the spaces around the "AS".

We can fix this by using regex instead of explode, splitting on the "AS" more accurately.

Just replace the explode with this:

$segments = preg_split( '/\s+as\s+/i', $value );

Note: When you have an Eloquent belongsToMany method that has to include the database name (eg. we need to join across DBs), this type of alias is created automatically, generating this error.

I originally committed this to 5.3 (#17292) and was asked to commit here instead.

Database names containing spaces are perfectly legal in Microsoft SQL Server.

Currently, when using SQL Server, if you have a database which includes spaces, the grammar handles wrapping it fine.

So this:

My Sample Database.dbo.my_table.my_column

Correctly becomes:

[My Sample Database].[dbo].[my_table].[my_column]

*But* if you also try to alias a column when the database contains spaces, it breaks.

This:

My Sample Database.dbo.my_table.my_column AS my_aliased_column

Becomes:

[My] AS [Sample]

I have traced the error to Database/Grammar/wrapAliasedValue() using explode( ' ' ) - the code thinks the spaces in the database name are the spaces around the "AS".

We can fix this by using regex instead of explode, splitting on the "AS" more accurately.

Just replace the explode with this:

$segments = preg_split( '/\s+as\s+/i', $value );

Note: When you have an Eloquent belongsToMany method that has to include the database name (eg. we need to join across DBs), this type of alias is created automatically, generating this error.

I originally committed this to 5.3 (#17292) and was asked to commit here instead.
@GrahamCampbell GrahamCampbell changed the title Can't Wrap DB Names Containing Spaces When Aliasing [5.4] Can't Wrap DB Names Containing Spaces When Aliasing Jan 13, 2017
@GrahamCampbell
Copy link
Member

Please add tests to cover this.

@voidstate
Copy link
Contributor Author

I don't know how to edit a pull request, but this test works on the new code (and fails on the current code):

For /tests/Database/DatabaseQueryBuilderTest.php

public function testAliasWrappingWithSpacesInDatabaseName()
{
	$builder = $this->getBuilder();
	$builder->select('w x.y.z as foo.bar')->from('baz');
	$this->assertEquals('select "w x"."y"."z" as "foo.bar" from "baz"', $builder->toSql());
}

@GrahamCampbell
Copy link
Member

I don't know how to edit a pull request

Just push to the same branch.

@voidstate
Copy link
Contributor Author

Thanks Graham.

Unfortunately, I'm a Git newb and am really struggling to do this - just spent over 2 hours wrestling with it and have totally failed. I have added that test case and run it and it works but can't add it to the patch-2 branch.

Should I start again and make a PR containing both changes or could someone else add the test for me.

Thanks for your help!

@fernandobandeira
Copy link
Contributor

fernandobandeira commented Jan 13, 2017

@voidstate Just browse https://github.com/voidstate/framework/blob/patch-2/tests/Database/DatabaseQueryBuilderTest.php click on the pencil, do your changes and save it'll save into patch-2 and the commit will appear here...

@voidstate
Copy link
Contributor Author

@fernandobandeira Thanks. That was much easier than what I was trying to do. :)

@GrahamCampbell
Copy link
Member

Re-triggering the CI services. :)

@ntzm
Copy link
Contributor

ntzm commented Jan 13, 2017

What happens if the table name contains [space]as[space]? Like foo as bar?

@voidstate
Copy link
Contributor Author

"As" is a reserved word in most rdbms. http://www.petefreitag.com/tools/sql_reserved_words_checker/?word=As

@taylorotwell taylorotwell merged commit 5a9e55e into laravel:5.4 Jan 16, 2017
symfony-splitter pushed a commit to illuminate/database that referenced this pull request Jan 16, 2017
* Can't Wrap DB Names Containing Spaces When Aliasing

Database names containing spaces are perfectly legal in Microsoft SQL Server.

Currently, when using SQL Server, if you have a database which includes spaces, the grammar handles wrapping it fine.

So this:

My Sample Database.dbo.my_table.my_column

Correctly becomes:

[My Sample Database].[dbo].[my_table].[my_column]

*But* if you also try to alias a column when the database contains spaces, it breaks.

This:

My Sample Database.dbo.my_table.my_column AS my_aliased_column

Becomes:

[My] AS [Sample]

I have traced the error to Database/Grammar/wrapAliasedValue() using explode( ' ' ) - the code thinks the spaces in the database name are the spaces around the "AS".

We can fix this by using regex instead of explode, splitting on the "AS" more accurately.

Just replace the explode with this:

$segments = preg_split( '/\s+as\s+/i', $value );

Note: When you have an Eloquent belongsToMany method that has to include the database name (eg. we need to join across DBs), this type of alias is created automatically, generating this error.

I originally committed this to 5.3 (laravel/framework#17292) and was asked to commit here instead.

* Adding Tests

RE: laravel/framework#17312
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.

5 participants