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

[10.x] Fix using schema names in SQL server #49929

Closed
wants to merge 13 commits into from
Closed

[10.x] Fix using schema names in SQL server #49929

wants to merge 13 commits into from

Conversation

arjenvanoostrum
Copy link
Contributor

The change in #49020 resulted in breaking changes for users utilizing schema's in SQL server. Much like Postgres, SQL server has the ability to store tables and other objects in different schemas in the database. Forementiond PR resulted in:

  • false positives for hasTable() and hasColumn when checking a table that exists in another schema
  • false negatives for hasTable() and hasColumn when checking a table in a non-default schema

This PR fixes these cases and adds tests to the class. As there are quite some similarities with Postgres, I reused some of these methods.

@hafezdivandari , I noticed your comment on the PR regarding the Postgresql:

Schema::hasTable() left unchanged on PostgreSQL to avoid a breaking change on a rare case that user is calling this function with a 3-parts search path Schema::hasTable('database.schema.table'), although this usage is non-standard IMO and should be Schema::connection('database')->hasTable('schema.table'). I'll cover this on another PR to master branch.

I agree with you this is non-standard and even so, not possible. As the postgres docs state: https://www.postgresql.org/docs/current/infoschema-tables.html

table_catalog sql_identifier
Name of the database that contains the table (always the current database)

The existing compileTableExists will only find records in the current database. @cbj4074 Maybe you can explain the mentioned use case?

@hafezdivandari
Copy link
Contributor

@arjenvanoostrum Are you trying to fix something on this PR or add support for specifying schema on SQL Server?
Please mark this PR as draft for now, as the tests are failing.

I know that SQL Server supports specifying schema just like PostgreSQL, but it would be a breaking change to add support for this on 10.x, you may target master instead.

@driesvints driesvints marked this pull request as draft January 31, 2024 14:55
@arjenvanoostrum
Copy link
Contributor Author

Hi @hafezdivandari, thanks for your quick reply.

Up until the 10.34 release it was possible to use schema names in hasTable and hasColumn for many years due to the use of the object_id() function in SQL server in the compileTableExists method that takes care of this:

public function compileTableExists()
{
return "select * from sys.sysobjects where id = object_id(?) and xtype in ('U', 'V')";
}

As of 10.34 this is no longer possible which was a breaking change for us. So I think it should be a fix and no added support. We have to halt updates to our apps.

I tried to fix it without reverting your work in #49020 and in a way that is similar to how Postgres Builder deals with this to improve the framework. I just pushed a last version but seem to have some trouble fixing the style, will look at that tomorrow

…o bugfix-sqlserver-schema-use

# Conflicts:
#	tests/Database/DatabasePostgresSchemaBuilderTest.php
@hafezdivandari
Copy link
Contributor

hafezdivandari commented Feb 1, 2024

@arjenvanoostrum this PR is breaking change and won't get merge to a patch release. I'll send another PR to fix this.

But please note that using schema names on SQL Server were never actually supported / tested on Laravel and few methods conditionally work with schema names (e.g. using table prefix will break this), the question is, are you actually using hasColumn() with schema name on SQL Server or fixing hasTable() is enough for your current use case? because It's easy to fix hasTable() on a patch release but fixing hasColumn is tricky. Can you wait for Laravel 11?

@arjenvanoostrum
Copy link
Contributor Author

@hafezdivandari Yes we do use hasColumn with schema names. I see Laravel 11 is scheduled for Q1, We will wait for that. Maybe we should also add hasView?

And should other parts of the framework be adjusted? I am thinking of the Blueprint methods create and alter?

@hafezdivandari
Copy link
Contributor

@arjenvanoostrum good, I'll send a PR for master then, we can fully support schema name on almost all methods on Laravel 11.

@cbj4074
Copy link
Contributor

cbj4074 commented Feb 5, 2024

Hello! Thanks for the mention here, and apologies for the slow reply.

With regard to the use-case for Schema::hasTable('database.schema.table'), I will have to get my head back into this to provide a useful answer.

It's been a few years since I've looked at any of this, but my recollection is that there are valid use-cases for wanting to see if a given schema contains a table when said schema is not necessarily (or even explicitly) within the database that is defined on the connection.

I would have to dig deeper for a specific example in my own code, but my recollection is that we had a need to perform this check against a different database (not the one defined on the connection in question).

@arjenvanoostrum When you say

The existing compileTableExists() will only find records in the current database.

Are you sure? The query looks to include table_catalog = ?, which, if memory serves, effectively specifies the database:

/**
* Compile the query to determine if a table exists.
*
* @return string
*/
public function compileTableExists()
{
return "select * from information_schema.tables where table_catalog = ? and table_schema = ? and table_name = ? and table_type = 'BASE TABLE'";
}

Again, I'd have to run some of my custom integration tests, ideally against these PRs, and revisit this to provide more information.

@arjenvanoostrum
Copy link
Contributor Author

Hi @hafezdivandari I'll await your PR than, thanks for picking up. I'll close this one.

And hi @cbj4074! I didn't test it on a live PostgresSQL before, though it should behave like that accoring to the docs (https://www.postgresql.org/docs/current/infoschema-tables.html) which state that it will always be the current database. So, when testing with a docker Postgres:
image
So, when connected with the database test_database there is no record with a value other than test_database available in the tables view, and a query for any table in the table_catalog = 'postgres' does not return any record.

So, trying to use this compileTableExists in the current version to check the existence of a table in another database than the connected database will always return false.

@hafezdivandari
Copy link
Contributor

hafezdivandari commented Feb 12, 2024

@arjenvanoostrum I just sent another PR to 11.x to bring full support for non-default schema names on pgsql and sqlsrv, you may check #50019 and #49148

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.

4 participants