Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question] About IStorageContext #97

Closed
postb99 opened this issue Nov 14, 2017 · 5 comments
Closed

[Question] About IStorageContext #97

postb99 opened this issue Nov 14, 2017 · 5 comments
Labels

Comments

@postb99
Copy link

postb99 commented Nov 14, 2017

Hi Dmitry,

After your changes in #96, I had to modify some of my own code with additional cast here and many ugly casts :-) here

Thus a question, why not adding .Entry(T) and .Set<T> to IStorageContext interface? These methods seem agnostic enough to be added to interface, although they come from Entity Framework first.

@DmitrySikorsky
Copy link
Member

Yes, I've found this issue too today. I think, that was not so good idea and I should fix it in beta3. The reason I did it is here: https://github.com/ExtCore/ExtCore/blob/master/src/ExtCore.Data.EntityFramework/RepositoryBase.cs#L25

When I had protected IStorageContext storageContext; defined as StorageContextBase, I had to cast storageContext to StorageContextBase in the SetStorageContext method in order to assign the value to the variable. That made impossible to assign any implementation of the IStorageContext interface, except ones that inherit StorageContextBase. So it was not possible to use Identity.

Now I don't know what to do. Whether I have to make storageContext member to have StorageContextBase type again, or to write some extension method that will return IStorageContext as DbContext. I think that we should have the member with IStorageContext as type, because it should be possible to assign any implementation of that interface. From other point of view, this RepositoryBase class is related to the EntityFramework, so maybe that variable should have DbContext type.

I don't think we should add some members to the IStorageContext interface, because they are related only to the EF. And there are more members that should be available, like transactions, Database property etc.

What do you think?

@DmitrySikorsky
Copy link
Member

DmitrySikorsky commented Nov 14, 2017

Perhaps, we should add some additional member to the EF implementation of the repository. Like,

protected DbContext dbContext;

So we will have both storageContext and dbContext members. But I don't like this idea.

So maybe the extension method that will cast IStorageContext to DbContext is the best solution?

@DmitrySikorsky
Copy link
Member

...So, after some research, I think the best way would be to modify RepositoryBase<TEntity> class like this:

public abstract class RepositoryBase<TEntity> : IRepository where TEntity : class, IEntity
{
  protected DbContext storageContext;
  protected DbSet<TEntity> dbSet;

  public void SetStorageContext(IStorageContext storageContext)
  {
    this.storageContext = storageContext as DbContext;
    this.dbSet = this.storageContext.Set<TEntity>();
  }
}

This seems to be logical to have EF DbContext object inside the EF repositories without any casts. And you still have to provide the implementation of the IStorageContext interface.

I'm going to fix this tomorrow if you don't have any comments regarding this.

@postb99
Copy link
Author

postb99 commented Nov 15, 2017

Thanks for the research, it sounds very good like this, it is consistent !

@Xarkam
Copy link
Contributor

Xarkam commented Nov 15, 2017

Thank you. I will quickly update and remove all the cast that we had put.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants