Fix DbConnectionValidator when Shell Settings from database

[MikeAlhayek]

Fix #12334

@jtkech here is the request PR.

[jtkech]

@MikeAlhayek

Yes, we are closed to solution.

since in this case the table would already exists with the shell settings.

With the ShellDescriptor not shells settings, but I saw that you now query ShellDescriptor so that's okay. Yes if there is a shells settings document it only says that we use settings from database (in fact there is another shells configuration document in that case), but it doesn't indicate that the provided pair of database and prefix is already in use by a tenant. Note: Any other document can be added to the same database and prefix before a setup by another app level functionality as Shells from database does.

So when the document table exists, checking the ShellDescriptor document really means if it is already used or not by a tenant, this for the provided pair of database and table prefix.

However, in the Tenant validator we should check for both DocumentTableFound or ShellDescriptorDocumentFound

The setup service and Tenant validator can do the same check as AddDatabaseShellsConfiguration() (or another app level functionality) can be configured with any database and prefix. The only thing that means that a pair of database and prefix is in used is when the document table exists AND the ShellDescriptor document exists in it.

[jtkech]

@malhayek2014

For info the new prefix constraint I was talking is about the url prefix (not the table prefix), we can still setup the Default tenant with a non empty url prefix, but now in that case we can't create a non default tenant with an empty url prefix (if hostname is also empty) which I think is wrong.

That said, just tried we can still setup the Default tenant with a non empty table prefix, and a non default tenant with an empty table prefix. Anyway, even if it was no more allowed I would still keep the concept of the association of a given database and a given table prefix (even if empty).

So for me the setup service and the tenant validator should use the same checks and with the same messages, for example a message stating that the provided association of the database and table prefix is already in use by an OC tenant.

Would need to be retried but based on the current code looks like that we would only need to just check for ShellDescriptorDocumentFound in the tenant validator as in the setup service and then use the same message mentioning the database and the provided table prefix.

[MikeAlhayek]

@jtkech I disagree here. In the setup context I think all we need to check here whether the ShellDescriptor document exists or not. If the table exists without the ShellDeacriptor document, we'll also continue.

However, in the tenant validator which is used in other places like OC.Tenants, we should also check if the table exists. We need to prevent the use of the database prefix if a table called Document exists "even if it has a different scheme than YesSql has". For example, what if we are connecting to a database that has that has example1_Document with no records. In that case we get DocumentTableFound and we should prevent the tenant from being added.

I think we should also add a catch for an exception when the column Type does mot exists, we should also return DocumentTableFound.

[jtkech]

I totally disagree and I would give up ;)

But good news I may agree but not for the same reason (see the 2nd section).

I disagree here. In the setup context I think all we need to check here whether the ShellDescriptor document exists or not. If the table exists without the ShellDeacriptor document, we'll also continue.

I agree and the same for any tenant.

However, in the tenant validator which is used in other places like OC.Tenants, we should also check if the table exists. We need to prevent the use of the database prefix if a table called Document exists "even if it has a different scheme than YesSql has"

I don't agree, I would say also if it also use YesSql.

what if we are connecting to a database that has that has example1_Document with no records. In that case we get DocumentTableFound and we should prevent the tenant from being added.

I don't agree.

---

But just realized that with the tenant removal feature, when we will remove a given tenant the related Document table will be removed, so the same table prefix should not be also used e.g. by shells from database, otherwise we would lose all shells settings when removing this tenant.

So finally I may agree with you but not for the same reason, not about what we would have to do before creating/setup a tenant but because we will be able to delete a tenant, otherwise I would still think that the validation would have to be the same for any tenant (Default tenant or not).

Most of the time we use an empty table prefix for both the Default tenant and to retrieve shells from database, but fortunally we will not be able to remove the default tenant, so that's okay.

Note: Good to keep as possible the same management for any tenant (Default tenant or not), when we talk about the Default tenant as the Host tenant for me it is wrong, yes it has specificities but most of the things are done in the same way, for example configuring, releasing, reloading, setup and so on.

[MikeAlhayek]

@jtkech okay. Here is how I see things

When adding new tenant using OC.Tenants, at that point the default site already exists so it does not matter is we check for existing ShellDescriptor or not. What matters then is if the table exists or not. So if the user uses site1 as the prefix and if the table site1_Document exists for whatever reason, we should prevent it from being used. This table could be created for other reason that OC... At least, I don't see a good reason to allow it.

However, since the Default tenant can be created on the same Document table that the ShellConfigurations stores in, we can make an exception for it as only look for the document. So if the table exists with a ShellDescriptor entry, then we allow it to be used. Which actually create the Document table if it does not exists?

We should call the DbConnectionValidator on the code that actually created the Document table when shell configuration is used to prevent an error if the connection is invalid at that point.

I think we should also change the query we are executing to something like this instead so ensure that the table that is called document, has the same schema as YesSql would have.

  SELECT TOP 1 Content, Version
  FROM [OCTest3].[dbo].[Site1_Document]
  WHERE Type = '....'

Anyhow, you know OC better than me by far, so whatever decision you'll come back with I'll proceed with so we can fix this issue.

[jtkech]

@MikeAlhayek

No problem always good to understand and I may be wrong in some places.

Anyway, as said above we may keep this PR as it is because of the incoming tenant removal feature, I'm open to change the query if needed, I will re-think about it and re-do some tests tomorrow.

When adding new tenant using OC.Tenants, at that point the default site already exists so it does not matter is we check for existing ShellDescriptor or not..

No if I understand what you mean, each tenant has its own ShellDescriptor in its Document table.

However, since the Default tenant can be created on the same Document table that the ShellConfigurations stores in, we can make an exception for it.

But this is the case of any tenant because ShellConfigurations can be configured with any table prefix. But you may be right because of the incoming tenant removal feature.

We should call the DbConnectionValidator on the code that actually created the Document table when shell configuration is used to prevent an error if the connection is invalid at that point.

Here it's okay to let it fail and log the error.

[MikeAlhayek]

I'll do some more testing myself tomorrow as well and we'll come to a final approach.

Unfortunately, Dapper always throws SqlExeption whether the column does not exists or the table does not exists. We can't really relay on the where type =... logic. Because if the table exists but the column type does not exists we return DocumentTableNotFound.

I'll check it more tomorrow and looking into using the error # for more clues.

[MikeAlhayek]

@jtkech I made some updates above like added the missing comments as per your request. Additionally, I added explicit exception validation.

I also confirmed that in order for the code to work correctly, we should only check for DbConnectionValidatorResult.ShellDescriptorDocumentFound in the SetupService while checking both DbConnectionValidatorResult.ShellDescriptorDocumentFound and DbConnectionValidatorResult.DocumentTableFound in the TenantValidator.

Since this is the point of argument here, I strongly suggest that you test out my code before we make any additional logic change to it since I am confident that it works as intended.

[jtkech]

To be sure we are in sync, do you now agree that normally any tenant including the Default one can be checked in the same way? Because all can have an empty table prefix or not, all have a Document table with an unique related ShellDescriptor, and so on.

But that finally we want to make the distinction just because we will be able in the future to remove any non default tenant and its related Document table which then should not be used by e.g. app level services as shells settings/configurations from database.

---

So if finally we need to make the distinction, doing it by just not checking the same things in the tenant validator and the setup service looks wrong because e.g. the setup service is used to setup any tenant, this is the validator that would need to use an additional contextual data as isDefaultShell.

I did some changes locally that I tried, If you don't mind I will push them on your branch so that it will be easier to see what I meant and other tweaks I did, then feel free to update what you don't like.

Updated: Also I used a different way to check if the table columns exist without catching all possible provider exceptions and their error codes.

[MikeAlhayek]

@jtkech no I still disagree. SetupContext should not be checking for TableFound. Please test out my code first before making changes to it just so you see why I still saying that TableFound cannot be part of the setup context. The DbValidator validate the connection regardless if you have a prefix or not. It'll only allow one tenant to have no prefix. So if no-prefix is used, no other tenant can have no-prefix. This isn't something new in this PR. This is how we have it in main branch today.

Feel free to make any changes to the PR. But please test out what I have here first.

[jtkech]

@MikeAlhayek

I tested your code, it works but we can't create some use case by only using the admin UI because of the tenant validator, which is good, but this doesn't imply that we should not be consistent when checking any tenant in the setup service. Please test my code too, if you want.

Anyway I will analyze your last comments and will answer to them.

Hmm, also when I add some info, and I may be wrong, you rarely react to say if you knew it or not, for example when I said that each tenant has its own ShellDescriptor in its Document table.

[MikeAlhayek]

Does this solve the original problem?

[MikeAlhayek]

@jtkech I do apologize if sometimes I miss commenting on every point you share. Sometime it's hard to follow your chain of thought specially when I am using my phone instead of my PC. I never mean to ignore your comments as they are usually very informative and helpful.

But yes I know about Flags attribute and that ShellDescriptor document is available for every tenant.

[jtkech]

@MikeAlhayek Okay cool no problem, was just to be sure ;)