Add database options like TablePrefixSeparator, Schema, IdentityColumnType, and DocumentTable

[MikeAlhayek]

This PR addresses the concerns #12657

Here is a summary of the changes

Both TableNameConvention and TablePrefixSeparator were removed from YesSqlOptions.
The following properties were added to the ShellSettings of each tenant

• TablePrefixSeparator when this value does not exists we assume _ however, if empty string is found, we use no prefix for the tables.
• Schema this was added to allow you to provide a specific schema to use.
• IdentityColumnSize when this value is empty or invalid, we assume Int32
• DocumentTable when this value is empty we assume "Document"

If adding a new tenant failed due to missing Encrypt=false or TrustServerCertificate=True, we add the error to the log file to provide the admin more about the issue. Address issue #12679 and #12653.

@jtkech in the remove tenant code, you can use the new ITableNameConventionFactory to create an instance of ITableNameConvention with something like this

var options = DatabaseTableOptions.Create(shellSettingsBeingRemoved);

var tableNameConvention = _tableNameConventionFactory.Create(options);

ITableNameConventionFactory is a service that we register so you can inject it in ShellDbTablesInfo to create the tableNameConvention.

Added the following fields were added to Add/Edit tenant in the tenant UI and also to the setup screen when the database isn't configured.

[hishamco]

Can we merge #12363 before? One more thing I think we discussed before that the above changes could after 1.5, because the introduced changes might need testing before they got merge. All we know that this time is critical because we are so close to release 1.5 but I will leave this to @sebastienros to decide

I might review this after I finish some of my pending PRs ;)

[MikeAlhayek]

@hishamco please hold off of merging PR #12363 since there are few things changes here. As you can see the DbValidation was changed quite bit. I think this should be merged before 1.5. In 1.5 we added tablePrefixSeparator which was causing issues so this PR fixes these issue.

[Skrypt]

This is getting late for 1.5

[hishamco]

FYI @MikeAlhayek I'm not pushing to merge my PR ;) it's jsut refactoring notRhing else, but this PR has introduce new things that might have consequences. I'm totally agree with @Skrypt that this PR is too late, I appreciate your efforts but we are in a stable state to release 1.5, so adding anything else it will require integration, verification and validation

[jtkech]

@MikeAlhayek Okay, looks now much better ;)

[MikeAlhayek]

@jtkech glad you like the change :) If you see no further changes, please approve the PR. Thought on merging this in 1.5 to avoid having to deal with an obsolete YesSqlOptions later?

[jtkech]

I will try it this night

[jtkech]

As a last comment, so we can select some new options while creating/editing a tenant, then not on the setup form. So how can we define them for the Default tenant on a fresh setup, if not intended that's okay knowing that it is still possible from the regular Configuration.

Yes, some issues quite easy to fix, the main concern being that we can't persist through Shell Settings an empty string, but I prefer this approach even if there are remaining things to fix.

Otherwise LGTM.

[MikeAlhayek]

@jtkech I made all the changes including adding the same value to the API and to the workflow activities.

UPDATED

I also added the database properties to the Setup screen so even when the Default site is being setup, we can provide/change the options

[jtkech]

Okay cool, I will look at it asap.

[jtkech]

Only did a quick review but LGTM.

Yes for now the NULL workaround seems okay when binding from a create/edit/setup form, because we know that the separator field is there and that a value is provided even if it is binded to null.

But in other places we may need to respect the existing usage of APIs, already defined WFs and existing AutoSetup configs, meaning if nothing is changed the _ separator is still intended to be used.

Yes, this is the only remaining thing I still have to think about.

[MikeAlhayek]

@jtkech I made the last suggested changes. Let me know if you see any last items here. Please check the update I made to the workflow and the api controller to make sure I did what you suggested.

[jtkech]

@MikeAlhayek

I did a review, sorry if too long but one thing imply two others (exponential), maybe some parts could be managed in separate PR(s). One main point would be to manage new database fields like the existing ones as much as possible, which would be easier if we don't support empty separators, or if all binding sources (behaving dfferently) should provide the same dedicated value for this purpose.

[jtkech]

Okay, just saw the last meeting, looks like what I did is in sync with what was suggested.

[MikeAlhayek]

@jtkech

I tested out all the new changes you added to this PR. All looks good to me and it also falls inline with all the feedback we had during the meeting. I have some minor suggestions here.

[jtkech]

@MikeAlhayek

Okay done but wait, maybe some very last little simplifications

[MikeAlhayek]

@jtkech thanks. We'll check it tomorrow and ensure there is no more feedback from @sebastienros tomorrow before we merge it.

[hishamco]

May be I'm late to review this, but I suffered last two weeks from a damaged hard disk in my previous laptops. I just write down some comments that might be consider

[hishamco]

I'm not sure why the partials relying on EditTenantViewModel, it would be nice to use the UI components in both Create & Edit views

[jtkech]

Please wait before merging, need to check the last review / commits, at least fix unit tests.

[MikeAlhayek]

@jtkech no worries. Either approve or merge it when you are done making changes to it. Thank you

[jtkech]

I already approved it a while ago ;) I'm merging it