Sql Storage refactor

[alfhv]

This is not an issue but a request. Could be possible to refactor SqlWebHookStore to be more open to extensions ? I'm creating a custom Store for another DB, main change is I use a different EF context, I'm forced to duplicated 90% of code just to create my own context because the EF Context is created all the time with an explicit new

using (var context = new WebHookStoreContext())

thanks.

[HenrikFrystykNielsen]

sure, is it that you would like the Context to be parameterized somehow?

[alfhv]

good thanks ! no need context parameters for now.

[HenrikFrystykNielsen]

so you are good for now?

[alfhv]

hi, no sorry, i'm not good, i still need a custom webhookStore able to accept Sql context and any other EF context.

[HenrikFrystykNielsen]

Quick question: is it just the DbContext that requires modification or is it also the Registration class (which actually contains the data)?

[alfhv]

how I see it, according my current use case, changes should be done on SqlWebHookStore class. Lets says we call it as DbWebHookStore, then in some way we tell this class the Type of storeContext class it should use, ex: SqlWebHookStoreContext(current WebHookStoreContext), OracleWebHookStoreContext, AnyDbWebHookStoreContext...

I say we inject the Type and not an instance of storeContext, because we keep the current implementation for using() block :

using (var context = new MyOwnWebHookStoreContext()) // previously WebHookStoreContext()
            {
                var registrations = await context.Registrations.ToArrayAsync();
                ...
           }

of course we can't do new MyOwnWebHookStoreContext() here, a factory may be need.

The storeContext class should fill requirements:

• derived from DbContext (so it is IDisposable),
• has a member: public virtual DbSet< TypeOfModel > Registrations { get; set; }

TypeOfModel is the Registration class that contains the data, it should fill requirement:

• have members: User, Id, ProtectedData, RowVer

I need a custom implementation because row types could not be the same from one DB to another, ex: RowVer is [TimeStamp] byte[] for Sql but this dont work on Oracle(at least in my tests) so I'm using different type for RowVer (and a custom rowversion implementation of course)

let me know your thoughts

[rposener]

Question: Why not just create your own IWebHookStore? It seems like that is what you want to do (different provider - OracleWebHookStore)? The IWebHookStore is actually quite simple to implement.

Having just done work for asp.net core port, that would be the logical DI unit to replace. It seems that making that context more extensible is just adding complexity to a simple solution when there is a logical extension point which is basically a wrapper for what you are talking about already.

If you created an OracleWebHookStore and submitted a pull request it would be a great add from the community - that's where SqlWebHookStore came from to start with!

[alfhv]

Answer: well...yes it is simpler with the IWebHookStore, actually my OracleWebHookStore inherit from WebHookStore.

My current implementation is exactly the same as SqlWebHookStore, just copy and paste the file and run a replace WebHookStoreContext by WebHookOracleStoreContext

you end up having changed only this lines:

using (var context = new WebHookOracleStoreContext()) { ... }

This WebHookOracleStoreContext is the Oracle EF context and here is all the code I have there:

public class WebHookOracleStoreContext : DbContext
{
    public WebHookOracleStoreContext()
        : base(new OracleConnection(ConfigurationManager.ConnectionStrings["MS_OracleStoreConnectionString"].ConnectionString), true)
    {
        Database.SetInitializer<WebHookOracleStoreContext>(null);
    }

    public virtual DbSet<RegistrationModel> Registrations { get; set; }

    protected override void OnModelCreating(DbModelBuilder modelBuilder)
    {
        modelBuilder.HasDefaultSchema(string.Empty);

        modelBuilder.Entity<RegistrationModel>().ToTable("WEBHOOKS");
        modelBuilder.Entity<RegistrationModel>().Property(r => r.User).HasColumnName("WHUSER");
        modelBuilder.Entity<RegistrationModel>().Property(r => r.Id).HasColumnName("ID");
        modelBuilder.Entity<RegistrationModel>().Property(r => r.ProtectedData).HasColumnName("PROTECTEDDATA");
        modelBuilder.Entity<RegistrationModel>().Property(r => r.RowVer).HasColumnName("ROWVER");//.IsConcurrencyToken();
        //modelBuilder.Entity<WebHookOracleRegistration>().Property(r => r.RowVer).IsRowVersion();
        base.OnModelCreating(modelBuilder);
    }
}

Commented code is what dont work on Oracle because there is no Timestamp column. Then, in my RegistrationModel class, i have just changed this line:

public decimal RowVer { get; set; }

on Oracle side then I'm using a simple Sequence+trigger updating this column. Maybe not the better way to do but at least it do the work for now.

And that's all to have an Webhook Oracle Store.

As you can see, my changes on the Store class is only in the new of context. I didn't put my code on PROD, i'm only testing locally but it is working fine, didn't find yet a situation where the current SqlStore implementation dont work for Oracle, maybe we could have different type of exceptions, dont know just a guess.

So, that's why I have requested some refactor in order to avoid the duplication of Store class.

Note: EntityFramework code-first steps (add-migration, update-database, etc..) don't work very well with oracle I think there are few bugs around:

• ORA-01918: user 'dbo' does not exist
• Error 'OracleInternal.Common.ODTSettings.m_bUseLongIdentifiers' While adding migration
• oracle entityframework The context cannot be used while the model is being created

I'm using this package:

<package id="Oracle.ManagedDataAccess.EntityFramework" version="12.1.2400" targetFramework="net451" />

at least on my local can't make it work so i'm creating/updating Oracle DB table using a separate script.

[HenrikFrystykNielsen]

Great discussion! And I would of course love a PR in the end :)

Just as an experiment, I created the gist [1] with a refactoring of SqlWebHookStore so that there is an abstract DbWebHookStore<TContext,TRegistration> base class which takes a DbContext and an implementation of the Registration interface as entity.

Would you mind seeing if that works? Hopefully you should be able to just take the files and add them to your local project.

Thanks!

Henrik

[1] https://gist.github.com/HenrikFrystykNielsen/30ccb672c224cbad83b2b0eebc288f3c

[alfhv]

all good with the new code !! now is very simple to extend to any DB implementation ! problem for me is I'm on corporate network with strong security policies that dont allow me to pull or push from github any code so only way to collaborate is by posts. I could do it from home, yes, but I'll dont have the full code or DB to test, so i apologize to don't submit any PR.

Only one thing to discuss with you:

The connectionString is passed on the settings to DbWebHookStore() and there is the method CheckSqlStorageConnectionString() to check if it really exist on config. connStr name is hardcoded to "MS_SqlStoreConnectionString", no problem with that.

But point is that this connection string is not used anywhere on DbWebHookStore but on the context class itself: SqlWebHookStore or OracleWebHookStore in my case as follow:

public WebHookOracleStoreContext()
        : base(new OracleConnection(ConfigurationManager.ConnectionStrings["MS_OracleStoreConnectionString"].ConnectionString), true)
    {...}

as you can see, I dont rely on settings received by DbWebHookStore() to get the connection string, I'm looking again at configurationManager directly, first because the constant WebHookStoreContext.ConnectionStringName not accesible and second because there is no link or reference to received settings on Store.

So, question is if I'm doing it in wrong way, maybe I need to use some available property or method to get the same connection string... ?? or maybe we can link, pass or make available in some way the settings(the connectionString actually) on the context class implementation, so we ensure that same connection string received and tested by the Store is the one used by the context, or at least it is available on context side, even if we want to do something else there.

[HenrikFrystykNielsen]

thanks for the feedback -- this makes sense. The update is committed 9056117c7b6293753cceecd5dcc16aa48f10decb and should go into release fairly soon.

[HenrikFrystykNielsen]

@alfhv -- the update is out on nuget version rc1c. Good luck!