YesSqlOptions and DbConnectionValidator concerns.

[jtkech]

• YesSqlOptions may have different values per tenant, but we use it in DbConnectionValidator on a shell that is not yet built so the .ValidateAsync() may use wrong values.

  I thought about only using configuration to be able to retrieve the config section of a tenant before it is built, but YesSqlOptions not only contains simple fields but also implementations as TableNameConvention.

  I started a PR using IConfigureNamedOptions<YesSqlOptions> providing options by tenant name but there is another concern if we upgrade the code on an existing application, see below.

• When building an existing tenant we should not use YesSqlOptions but the actual values that was used to create, update or setup the tenant. On an existing application if you do the following as documented, you get again the setup screen.

  .ConfigureServices(s => s.Configure<YesSqlOptions>(o => o.TablePrefixSeparator = "__"))
  

  No problem for e.g. TablePrefix that is selectable and then hold by ShellSettings but for options coming from YesSqlOptions as TablePrefixSeparator this is not yet the case.

  Not a big issue, we could also hold all YesSqlOptions on ShellSettings, but YesSqlOptions not only contains simple fields but also implementations as TableNameConvention.

  There are solutions, maybe some providers activating implementations by type names, notice that this would also allow to configure all YesSqlOptions from configuration sources.

  I can work on it but first I prefer to be sure of what we want.

[MikeAlhayek]

@jtkech currently the YesSqlOptions.TablePrefixSeparator is stored in each tenant config file. So when the connection is configured for each tenant, then we used the stored value which would provide the correct value.

When we validate a new tenant, then we use the current global value as we should since this global configuration is for defaults. I think the problem with the DbConnectionValidator is that it assumes we are only validation new tenants which is wrong. I think it should be changed so that the ValidateAsync() accepts a single parameter DbConnectionValidationContext which allows the consumer to provide all the value they want to validate. So the DbConnectionValidationContext would look something like this

public class DbConnectionValidationContext
{
    public string DatabaseProvider { get; set; }
    public string ConnectionString { get; set; }
    public string TablePrefix { get; set; }
    public string ShellName { get; set; }
    public string ShellName { get; set; }
    public string TablePrefixSeparator { get; set; }
    public string Schema { get; set; }  // this is new
    public IdentityColumnType  IdentityColumnType  { get; set; }  // this is new
    public ITableNameConvention TableNameConvention { get; set; }
}

Additionally, I think we should also provide a a new TableNameConbvention for OC so that it uses the _yesSqlOptions.TablePrefixSeparator value. Then this new implementation would have a static method on it called Create(string tablePrefixSeparator) so that we can easily create a new instance of this class.

Now, when we validate new tenant from TenantValidator, we can provide DbConnectionValidationContext using the global defaults.

[jtkech]

Okay, feel free to start a PR around this when you will have time.

Only from memory so in an unordered way.

currently the YesSqlOptions.TablePrefixSeparator is stored in each tenant config file. So when the connection is configured for each tenant, then we used the stored value which would provide the correct value.

I removed it at the time I was assuming that YesSqlOptions should be for all tenants and should have the same values for the lifetime of a given application.

Hmm, when you say options may be different per tenant, did you mean that in fact the options are always provided in the same way for all tenants, e.g. from an always enabled feature or from the app with our ConfigureServices() helper, and then that different tenants may have different values only because when they are setup the options values were different. So meaning no different options per tenant at a given time, only one global way for all tenants but that may provide different values in the lifetime of the app.

If so that would be more reasonable, okay they would provide default values when creating/setup a tenant, and these default values may change e.g. to create/setup new tenants.

When we validate a new tenant, then we use the current global value as we should since this global configuration is for defaults.

Okay then for the case of new tenants

I think the problem with the DbConnectionValidator is that it assumes we are only validation new tenants which is wrong.

Yes, I don't assume this as I'm using it in the TenantRemoval PR before removing a tenant, we already use it when saving and before setup a tenant, hmm but here maybe you mean by new tenant a tenant that is not yet setup, anyway I agree.

Yes good idea to use a DbConnectionValidationContext, at some point I thought about passing ShellSettings but I think you want to decouple things as much as possible from OC Shell.

Yes I know for the new IdentityColumnType, for now I would use the same name as in YesSql and then change it if it wil be also changed on YesSql side, to be in sync on what we are using.

---

Yes need to be sure of what we want, and then document it accordingly.

So okay, If I understood correctly the above, at this point still a problem for the options that are related to implementations, knowing if I remeber correctly that only TableNameConvention is used by DbConnectionValidator.

Additionally, I think we should also provide a a new TableNameConvention for OC so that it uses the _yesSqlOptions.TablePrefixSeparator value.

Okay, do you mean only one static way to implement TableNameConvention for OC, if so that would solve the problem, but then why making it an option, maybe because you still want it in the options to be used in another context, but for OC this option will always point to this static implementation.

If so okay, the simple options would be hold by ShellSettings, the only implementation needed by DbConnectionValidator would be static. We could then also provide these simple default options from configuration source as appsettings.json, but only if worth to do.

But maybe I completely misunderstood what you meant ;)

[MikeAlhayek]

It may be easier to explain it via code.

But the idea when you validate when adding/setting up a tenant, you provide the values to the validator from the global option. However, when removing a tenant, you want to pass values from current ISession or the ShellSetting.

We cannot assume that the setting will not change thought the life of the application which is why I stored the separator in the ShellSettings. Not sure why that was removed, but I think it should be put back. If the ShellSettings does not have a value (null), we assume the hard coded _ for backward compatibility. But if empty string exists, then we don't assume _ as this would mean that the user does not want any separator. With that put back, it should be easy to provide TableNamingConvention that uses our options.

I'll try to submit a PR to hopefully explain it better.

[jtkech]

Okay so I think we now are more in sync on what may have to be done.

But the idea when you validate when adding/setting up a tenant, you provide the values to the validator from the global option.

Okay then, so it seems that I understood what you meant in my previous comment

If so that would be more reasonable, okay they would provide default values when creating/setup a tenant, and these default values may change e.g. to create/setup new tenants.

Which is not the same as having YesSqlOptions per tenant, I understood it this way because for example the TableNameConvention implementation can't be held by ShellSettings.

But now that TableNameConvention is a static implementation, that's okay but maybe we don't need to make it an option, just init the store with something created with the separator to use.

... which is why I stored the separator in the ShellSettings. Not sure why that was removed

I did it because of the above

but I think it should be put back.

I now agree but see in the below section what we already support out of the box.

But if empty string exists, then we don't assume _

Yes less easy to manage when coming from configuration, but if only from options it is easier.

So okay looks like we are better in sync but see the following.

---

That said, what you have to know is that ShellSettings has 2 inner ShellConfiguration, the one that can be injected with IShellConfiguration, also accessible by using ShellSettings["SomeProp"} but only for strings, and another one for a limited set of settings needed to select and run a tenant, both are using the regular configuration stack, on the first one we stack the tenant specific source e.g. the appsettings.json under the tenant folder, on the second one we stack e.g. the shared tenants.json, the values in these last stacked config/settings sources (that are mutable) will then always win.

So ShellSettings already use the full config stack, for example we can already provide a TablePrefix from configuration, if we update it through the editor it override it in the specific tenant source e.g. appsettings.json under the tenant folder, other edited values may be persisted in tenants.json.

---

So just to say that all YesSqlOptions that are simple fields, which now seems to be sufficient to use the DbConnectionValidator, can already be provided automatically from the regular Configuration stack without any code. So for example you can provide global values under the OrchardCore section, or tenant specific values under a section having the tenant name. More over you can use any config source as env variables. Then we will be able automatically to use shellSettings['TablePrefixSeparator'].

So for these simple fields, as suggested at some point we could just say that they are configurable through regular config sources. For example if the default separator is not configured we could use the default of the default which would be the underscore used by yesSql by default.

So YesSqlOptions would be only used for other options being implementations, not TableNameConvention as it would be a static thing using a given separator.

This way we could retrieve the configured (or not) value of a given tenant before the related container is built, at least without having to create a scope on it.

[MikeAlhayek]

Additionally, I think new tenant screen should have the options where the admin can provide a schema, separator, identity column type. All this would be saved in the ShellSettings as previously mentioned. By default, these fields will come from the YesSqlOptions

[jtkech]

First let's talk about only fields that can be held by ShellSettings, not implementation options.

Additionally, I think new tenant screen should have the options where the admin can provide a schema, separator, identity column type. All this would be saved in the ShellSettings as previously mentioned.

Okay no problem but before the problem was for the needed TableNameConvention that can't be held by ShellSettings, but that's okay if we now assume that TableNameConvention is globally configured, which was not obvious at the beginning, that's why at some point I thought about only have global options.

By default, these fields will come from the YesSqlOptions

I don't think so, now that these simple fields will be managed through ShellSettings which already uses the Configuration stack, we already have automatically all what we need to configure these options.

For example we already can pre-configure the TablePrefix from configuration, if so when you edit a tenant you will see it, then if you change the value it will be saved in the tenant specific appsettings, so that if you change again the configuration, an existing tenant will still use the value used to setup it.

So for example TablePrefixSeparator and Schema, regardless they are editable or not, can just be managed as TablePrefix (which doesn't need to be provided by YesSqlOptions). The fact that they are editable just allow to override them and save them in the tenant specific config source (the only mutable source) that has the higher precedence.

---

About the other options being implementations, here also we would need to ensure that we resolve the same values for a given existing tenant, knowing that for now these options can't be held by ShellSettings, but maybe here you planned to use them in a fixed way and globally.

Will see later and then add to the documentation what people can do.

---

About the identity column type, we have services as IndexingTaskManager that rely on the fact that the Id is a number on which we can apply e.g. the filter Id > @param, so for now I would keep the same enum type and name as currently used on YesSql side, IdentityColumnSize as I remember.

[MikeAlhayek]

By default, these fields will come from the YesSqlOptions
I mean when adding a new tenant, we would load the UI with the YesSqlOptions until the appsettings of the tenant is generated, then we always read from there.

Okay no problem but before the problem was for the needed TableNameConvention that can't be held by ShellSettings
The TableNameConvention should not be a problem once that is something we can create per request using options from ShellSettings. Here is an implementation of OC's implementation of the ITableNameConvention "I have not yet tested that code"

public class DefaultTableNameConvention : ITableNameConvention
{
    private readonly string _tablePrefixSeparator;

    private DefaultTableNameConvention(string tablePrefixSeparator)
    {
        _tablePrefixSeparator = tablePrefixSeparator ?? throw new ArgumentNullException(nameof(tablePrefixSeparator));
    }

    public DefaultTableNameConvention(ShellSettings shellSettings)
        : this(shellSettings["TablePrefixSeparator"] ?? "_")
    {
    }

    public const string DocumentTable = "Document"; // maybe even make this configrable
    
    public string GetIndexTable(Type type, string collection = null)
    {
        if (String.IsNullOrEmpty(collection))
        {
            return type.Name;
        }

        return collection + _tablePrefixSeparator + type.Name;
    }

    public string GetDocumentTable(string collection = null)
    {
        if (String.IsNullOrEmpty(collection))
        {
            return DocumentTable;
        }

        return collection + _tablePrefixSeparator + DocumentTable;

    }

    public static ITableNameConvention Create(string tablePrefixSeparator)
    {
        return new DefaultTableNameConvention(tablePrefixSeparator);
    }
}

So when you validating a new tenant, you can create ITableNameConvention using DefaultTableNameConvention.Create("value would be coming from the UI")

When you validating the connection before the setup and when you configuring the connection new DefaultTableNameConvention(shellSettings).

When you remove the tenant, ISession.Store.Configurations.NameConvention

So for example TablePrefixSeparator and Schema, regardless they are editable or not, can just be managed as TablePrefix (which doesn't need to be provided by YesSqlOptions). The fact that they are editable just allow to override them and save them in the tenant specific config source (the only mutable source) that has the higher precedence.
I don't think you would want that. YesSql now have Schema as new option in the configuration.

About the identity column type, we have services as IndexingTaskManager that rely on the fact that the Id is a number on which we can apply e.g. the filter Id > @param, so for now I would keep the same enum type and name as currently used on YesSql side, IdentityColumnSize as I remember.
This should be no issue at the moment since YesSql only provide int32 or int64 so both will with with no issue.

In summary, I think all the properties should be stored in the appsettings of the tenant so when ShellSettings is created, it also contains the configuration used by the tenant. the the YesSqlOptions would be used just in the initial request to provide default settings on the first get request in the admin create page.

[jtkech]

By default, these fields will come from the YesSqlOptions
I mean when adding a new tenant, we would load the UI with the YesSqlOptions until the appsettings of the tenant is generated, then we always read from there.

You don't need this as we already can configure them from any config source, e.g. the main appsettings.json, doing so you will see them when first editing a tenant, then when updating the tenant specific appsettings it will always win.

Updated: The same way we don't need to provide a TablePrefix from YesSqlOptions, but we already can define for it a default value from configuration.

Step by step, I will read your other comments.

[MikeAlhayek]

You don't need this as we already can configure them from any config source, e.g. the main appsettings.json, doing so you will see them when first editing a tenant, then when updating the tenant specific appsettings it will always win.
Yes but then you would assume that the default are the current settings from the Default tenant. I think having options that that could change is better than assumption.

[jtkech]

the YesSqlOptions would be used just in the initial request to provide default settings

That can be provided from Configuration

[jtkech]

Yes but then you would assume that the default are the current settings from the Default tenant. I think having options that that could change is better than assumption.

If nothing is configured to provide default values, you just need to apply default of the default values. As you do in YesSql options through a property initializer (the default of the default) if nothing is set afterwards.

[jtkech]

I don't think you would want that. YesSql now have Schema as new option in the configuration.

That's another concern, if so we can document it to not do it, then would have to be managed a little differently, maybe only its initial value or yes keep it in YesSql options, anyway it is already possible and at the end it will be e.g. in the tenant specific appsettings.json which is a config source.

[jtkech]

I'm not against keeping YesSql options for these fields but they can already come from configuration, so if at some point you check if ShellSettings already contains a value for a given option it may have already came from configuration. That said maybe we just need to don't say to people that they can do this ;)

[jtkech]

Just saw your DefaultTableNameConvention example, okay I already understood it like this, a static thing using a provided separator, which I didn't know at the beginning.

About other implementation options, how did you plan to configure them, in a fixed way, globally or per tenant?

[MikeAlhayek]

@jtkech the TableConvention would be per tenant during the configuration of the sqlserver and then registered "I think we already register it as a service"

[jtkech]

Okay for TableNameConvention that currently we set in a PostConfigure if no value has been provided, anyway that will be okay with the DefaultTableNameConvention factory that you suggested.

To anticipate, the question was more related to other implementations as IdentifierAccessorFactory, how did you plan to configure them, in a fixed way, globally or per tenant?

[hishamco]

Seems I will lose in the long discussion ;) talking with code could simply the things

[jtkech]

@hishamco Don't worry I'm going to close this thread ;)

@MikeAlhayek Okay, I think it was good to be aware of what is provided oob from Configuration, but yes not so easy to anticipate what would be static in OC, which options are global or not, not good to be provided from configuration, so I'm not against using YesSqlOptions as a first source of default values.

So feel free to open a PR to provide what you want and that you know better than me, maybe at least even if it is editable or not, set from the beginning the separator in ShellSettings, and then even if the first default value doesn't come from Configuration but YesSqlOptions, anyway when the ShellSettings will be saved in the tenant specific appsettings, it will be part of the configuration stack.

So that's okay, normally we will be able to retrieve the separator from ShellSettings without having to create a scope, so maybe I will not have to change anything in the Tenant Removal PR.

Then add what is needed around the fixed TableNameConvention factory you suggested, and I think we don't need anymore the services.PostConfigure<YesSqlOptions>() that I added to ensure this option has a value that then we always use to config the store and then in sync when used in DbConnectionValidator.

Yes because here still not clear for me, okay TableNameConvention will be a fixed thing in OC but the factory needs an actual separator, so for example in DbConnectionValidator where we use the separator, maybe we should not use the TableNameConvention option as is but an instance created on the fly using the right separator retrieved from ShellSettings. Maybe here the option itself should be the factory.

About how other implementation options are intended to be used, I don't have enough info, at least for now they can't be held by ShellSettings, so maybe at least they should be global for all tenants. So the code could be kept as is, just to know if we agree on this, and if so we could document it, e.g. that they should not rely on a feature that may be deactivated.

Anyway I will close this issue and will see the last details if you open a new PR.

[jtkech]

@MikeAlhayek

Did you see my previous comment where I suggest to open a new PR and another issue as this one is already too long, at least to fix the current known issue if the TablePrefixSeparator is configured on an existing application (we get again the setup screen). I can start something if you don't have time.

[MikeAlhayek]

@jtkech yes. I saw the comment. I am planning to start a PR, but won't be soon. If you want to start PR go for it. Anyway, whom even starts a PR first, please draft it so we have one PR instead of multiple

[jtkech]

Okay cool then, will see.

[MikeAlhayek]

@jtkech I created and branch and started to make some updates toward this. Nothing to share yet more to know that I started. I'll draft a PR after a have some to share probably tomorrow. If you have started PR, feel free to continue and at then end we can compare the approaches and see which one would fit better

[hyzx86]

What happened?

[jtkech]

In your connection string try to add Encrypt=false or TrustServerCertificate=True

See #12653

[hyzx86]

Thanks, But I haven't set this option in most programs, : /

[MikeAlhayek]

@jtkech PR #12683 is ready to review. Feel free to directly commit changes to the PR if needed.

@hyzx86 PR #12683 added more info about the error into the log file.

[jtkech]

Okay will look at it this night