Avoid adding duplicate table prefix into the same database

[hishamco]

Fixes #11803

[hishamco]

@CrestApps this the simpler approach that I come up with to fix the table prefix issue

[hishamco]

I just need to fix a broken tests

[hishamco]

We might need to add some unit tests, but I can provide a screenshot

[hishamco]

TenantTablePrefixValidator.mp4

[MikeAlhayek]

@hishamco what you did is simply checking if the prefix existing from the settings. There are many issues with this approach

1. What if a user want to use the same prefix on multiple servers or databases? You code will prevent this from happening
2. What is the connection string is invalid? No check is done. Okay may be this would be outside the scope of the original issues
3. What if the database exists before you're tenant is added. the UI will then allow you to add the prefix just for it to fail during the setup.

IMO, your approach isn't good enough or there is a better way to validate this to handle multi-senarios. If you look at the other PR #11822, you'll see that we are doing much more than just validating the prefix from the settings. What we are doing is 2 important steps

1. Attempt to make a connection to the database. If the connection failed to open, we report that the provided string is invalid
2. When the connection is valid, we check the to see if the TablePrefix_Document exists on the database, and if the document exists we tell the user that the prefix is already exists and we prevent them from using the same prefix.

I think with the other PR there are much more added benefits than just simply checking the configurations.

[hishamco]

What if a user want to use the same prefix on multiple servers or databases?

You can't use the same table prefix on the same database, that's what your issue is about. In case you want to use the same table prefix on multiple server it's not a problem because we are checking against the connection string

What is the connection string is invalid? No check is done. Okay may be this would be outside the scope of the original issues

We could validate the connection string if we need to

What if the database exists before you're tenant is added. the UI will then allow you to add the prefix just for it to fail during the setup.

I presume creating the database first then adding the tenant, coz the tenant is not creating a database AFAIK except for SQLite

Again I don't think there's an issue here. Validating database existance might be a plus

[hishamco]

One more thing I need to clarify let us try to differentiate between validating the connection string and checking table prefix (which is what your issue is about)

[MikeAlhayek]

@hishamco

You can't use the same table prefix on the same database, that's what your issue is about. In case you want to use the same table prefix on multiple server it's not a problem because we are checking against the connection string

I am aware of the issue. I listed the use cases that could lead to problem not what I want to allow. I am saying here that your code does not protect against this problem.

We could validate the connection string if we need to
The other PR does this already. This is why I recommend the other PR over this as that one gives you a near bullet proof connection validation.

[hishamco]

I didn't get your point, moreover all the checks that are available on the tenant validator previously don't check if the connection is valid or not, that why I said before validating the connection will be plus

Please let us chat via Gitter or WhatsApp and write down the conclusion here

[MikeAlhayek]

@hishamco
I don't think this is a good check. For example, look a the following variation of SQL-Server strings

Data Source=localhost;Database=Orchard;Integrated Security=True;
Server=localhost;Database=Orchard;Integrated Security=True;
Server=localhost;Database=Orchard;User Id=username1;Password=password
User Id=username1;Server=localhost;Database=Orchard;Password=password

All the above should be treated the same since they point to the same exact thing when looking at the combination of 3 (Provider, Server, and Database) + prefix.

The way to do it here is to let the provider parse the connection string and give you a common DbConnection object. Like create IDbConnectionFactory that would return a DbConnection from a given screen and then use the common info in the DbConnection to create a unique key for each connection and see if these exists.

[hishamco]

We already provider a sample for each connection string ;) nothing but I can handle this easliy if we need to support such variations

[MikeAlhayek]

Yeah, I don't think that is enough check. Not everyone follow the sample string and even then, sometimes people use different username to to connect to the same database.

Do keep in mind that my original PR was just to inspect all connections string in the configuration "after paring them". But during the last meeting, @sebastienros suggest that we take it one step further and actually validation the database connection to make sure the the tables do not physically exists on the server. So if the recommendation not to do connection validation, I can tweak the PR #11822 and eliminate the physical connection validation step and just reply on the connection settings like how the PR original started. But I personally like @sebastienros recommendation which is why I tooks the PR one step further.

I would suggest we wait and see more feedback from @jtkech , @Skrypt and @agriffard so we are not wasting time making changes into both placed. Meanwhile I suggest you tag this PR with do not merge.

[hishamco]

As I said in other PR, let your check the table existance to avoid invalid serial instead of table prefix issue, this way it's clear for all what's going on

[hishamco]

@Skrypt can we triage this PR and the related one #11822