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

Supporting any IDbSet<> implementations #159

Merged
merged 4 commits into from Aug 24, 2015
Merged

Supporting any IDbSet<> implementations #159

merged 4 commits into from Aug 24, 2015

Conversation

@mkemal
Copy link
Contributor

@mkemal mkemal commented Jul 31, 2015

Currently, restier does not support IDbSet<> implementations other than DbSet<>. So, if one implements a custom IDbSet<> implementation, or even extends the default implementation, DbSet<>, he/she would not be able to use it with RESTier. Because, as far as i know by inspecting the ModelMapper implementation, any IDbSet<> implementation not being DbSet<> is rejected.

For example, when using IdentityDbContext, because it defines the entity sets as IDbSet<>, i have not be able to work it with Restier.

To overcome this problem, i created my custom IModelMapper by just copying the default one, and adding the changes i included in this commit. Currently i am using this implementation and I don't know if there is any reason for this constraint in the default implementation. So, instead of asking you if this is a proper solution, and explaining what i did in details, i decided to make a pull request.

If it may lead any problems with the rest of the framework, please let me know about it. Because, as i said, currently i use this implementation with my custom IModelMapper implementation which i created just to overcome this problem.

@msftclas
Copy link

@msftclas msftclas commented Jul 31, 2015

Hi @mkemal, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

@msftclas msftclas commented Jul 31, 2015

@mkemal, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@karataliu
Copy link
Contributor

@karataliu karataliu commented Aug 3, 2015

@mkemal
Thanks for the contribution! The change looks good in general.

Can you help fix the following issues:

  1. The code won't compile, unless System.Linq namespace using added;
  2. There are some build warnings from FxCop, please clean up all warnings.

@mkemal
Copy link
Contributor Author

@mkemal mkemal commented Aug 3, 2015

@karataliu

I had copied the changes from my custom implementation. Because i didn't even open the project in VS, I had never compiled it before submitting. So, i had forgotten to include 'Linq' namespace import. Sorry for that.

Here are the changes i included in the new commit:

  1. I added the using statement for the 'Linq' namespace.
  2. I run the VS 2013 Code Analyze and converted the 'GetGenericParent' method to static, according to analysis results.

@congysu congysu added this to the v0.3 milestone Aug 3, 2015
@@ -60,10 +61,10 @@ public ModelMapper(Type dbContextType)
if (property != null)
{
var type = property.PropertyType;
if (type.IsGenericType &&
type.GetGenericTypeDefinition() == typeof(DbSet<>))
var parent = GetGenericParent(type);
Copy link
Contributor

@congysu congysu Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a TypeExtensions.FindGenericType method:

public static Type FindGenericType(this Type type, Type definition)

Copy link
Contributor Author

@mkemal mkemal Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be used, however implementation of the TypeExtensions.FindGenericType seems a bit costly in terms of computational complexity. Say the type represented by the parameter type is at level h in its inheritance tree (distance from the type of object) and there are n interfaces implemented by it. Here are some cases:

  • If the parameter definition does not represent an interface, then the complexity of the method would be O(h),
  • If the parameter definition represents an interface implemented by type, then the complexity of the method would be O(n)
  • However, if the parameter definition represents an interface which is not implemented by type, then the complexity would be O(n h) which represents the worst case complexity of this method.

To make the worst case complexity O(max(n, h)), at least the inner foreach loop which checks the definition against the implemented interfaces should be moved out of the while loop.

If it is OK for you, i can add the changes proposed above and for its usage in ModelMapper to this pull request.

Copy link
Contributor

@congysu congysu Aug 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is OK for me - C# is with single inheritance, and GetInterfaces returns all interfaces the class/interface implements, and inherits. Please proceed with the change. Thanks.

Copy link
Contributor Author

@mkemal mkemal Aug 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the changes on FindGenericType method. However, TypeExtensions class appears to be marked internal, as it is defined in System namesapce. So, i couldn't use it from the ModelMapper class. It seems to me, the best solution is moving TypeExtensions class out from System namespace and make it public. However, as it will effect all places where the method is used, i didn't take any action.

Copy link
Contributor

@congysu congysu Aug 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can link the TypeExtensions class to Microsoft.Restier.EntityFramework.csproj. An example: https://github.com/OData/RESTier/blob/master/src/Microsoft.Restier.EntityFramework/Microsoft.Restier.EntityFramework.csproj#L40-L45

Copy link
Contributor Author

@mkemal mkemal Aug 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the trick; it is good to know the usage of such compile time dependencies. I pushed the changes.

@@ -13,34 +13,82 @@ internal static partial class TypeExtensions
BindingFlags.IgnoreCase |
BindingFlags.DeclaredOnly;


Copy link
Contributor

@congysu congysu Aug 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding comments. Code comments are optional for internal/private class and/or method.

@karataliu karataliu merged commit b797d73 into OData:master Aug 24, 2015
@karataliu
Copy link
Contributor

@karataliu karataliu commented Aug 24, 2015

merged this change. Thanks @mkemal .

@karataliu karataliu added this to the 0.3.0-beta2 milestone Sep 6, 2015
@karataliu karataliu removed this from the 0.3.0-RTM milestone Sep 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants