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

Exists database rule uses the wrong connection #43914

Closed
trovster opened this issue Aug 29, 2022 · 6 comments
Closed

Exists database rule uses the wrong connection #43914

trovster opened this issue Aug 29, 2022 · 6 comments

Comments

@trovster
Copy link
Contributor

  • Laravel Version: 9.26.1
  • PHP Version: 8.0.20
  • Database Driver & Version: MySQL / 10.8.3-MariaDB

Description:

When using new Exists(Model::class, 'column') with a table name which has a database prefix fails to validate correctly when also matching the required connection name.

I have two main connections; both have multiple databases so all my table names are prefixed with the database name. Adding this prefix allows relationships (via joins) to work correctly between databases on the same connection, which might have the same table name (eg db1.users and db2.users). A second connection, which is named "crm" has a database called crm and thus the table names are crm.users.

I have investigated the issue and found that removing $connection = null; from line #945 in ValidatesAttributes.php solves the issue. I am not sure what the condition solves, but it breaks in this scenario.

Steps To Reproduce:

Set up two connections and set the default connection. The other connection should be named the same as the table database prefix (eg crm).

/** @var string */
protected $connection = 'crm';

/** @var string */
public $table = 'crm.tableName';
@driesvints
Copy link
Member

You should not set a connection name in your table name. That's not supported.

@trovster
Copy link
Contributor Author

I am not setting the connection name on the table. I am setting the database name. They are two independent things.

@paulmichaeldev
Copy link

paulmichaeldev commented Aug 29, 2022

@driesvints When you say "connection name" can you clarify what you're referring to?

  • "Connection name" - as per the connection names defined in the database.php config file in a Laravel installation
  • "Database name" - as in the physical database name on a MySQL/PostgreSQL/etc server

@trovster is using the database.table notation to reference another database that the connection defined within Laravel has access to. For clarity you could say the connection name is crmconnection with the database name crm and therefore the model would be:

/** @var string */
protected $connection = 'crmconnection';

/** @var string */
public $table = 'crm.some_table';

In this instance, accessing another database on the same connection using database.table notation should work - while a setup where a database user can access multiple databases is not best practice for security reasons, SELECT * FROM crm.some_table; is perfectly valid SQL and will return a resultset.

@driesvints
Copy link
Member

Heya, setting the database name is also not supported. I don't think we document that anywhere.

@driesvints
Copy link
Member

That being said. We could maybe get a fix in and get this documented if it technically works. You could try a PR with a fix and a PR to the docs? It's just that I've seen people try to use it like this in the past and they always run into edge cases because take into account that it can work like this.

@trovster
Copy link
Contributor Author

I have been looking into this further and was about to put in a PR which removes the code. But I have found the original issue which was addressed at #37580 – they seem to imply the x.y syntax is connection related, when it should be database related;

class User extends Model {
   $table = 'public.users';
}
Rule::unique(User::class) // don't work -> Database connection [public] not configured.

They say they

added a fix to that problem, with a unit test to validate so that if a table has a '.' and the $connection is the same as the prefix, it sets the $connection to null, but I decided to create this issue to check if the current behavior (where the $table prefix is treated like a connection) is the intended behavior before submitting a PR

For me the syntax is database.table because you define the connection separate;

/** @var string */
protected $connection = 'connectionName';

The code to nullify the connection IF it is the same as the database name seems like an incorrect assumption. I removed the code and the tests still passed, so I am not sure they were robust enough. I am not sure how to proceed with this now.

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

No branches or pull requests

3 participants