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

Add DateTime/Date/TimeOfDay support. Simplify EF7 entity update logic. #278

Merged
merged 3 commits into from Jan 21, 2016

Conversation

Projects
None yet
3 participants
@rayao
Contributor

rayao commented Jan 19, 2016

#273

Also, removed EF.SqlServer dependency from Restier.EF7

@rayao

This comment has been minimized.

This is kind of ugly. Anyone has better approach?

@rayao rayao added cla-not-required and removed cla-required labels Jan 19, 2016

return EdmCoreModel.Instance.GetDate(efProperty.Nullable);
}
}
}

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

Is your code written specially for EF7 here? I am not sure about the difference that in my code the DB type name comes from the ColumnAttribute. But I guess we can keep both code here and use a compilation constant to differentiate EF6 and EF7?

This comment has been minimized.

@rayao

rayao Jan 20, 2016

Contributor

No this works only for EF6, EF7 has a much cleaner approach.
EF7 shares most code files from EF6, but has dedicated ModelProducer and ChangeSetPreparer.

@@ -209,6 +209,24 @@ private static object ConvertIfNecessary(Type type, object value)
return (DateTime)dateValue;
}
// Convert to System.DateTime supported by EF from DateTimeOffset.
if (value is DateTimeOffset)

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

We have almost identical code here:) I just need to move some of my comments here later after your commit.

This comment has been minimized.

@rayao

rayao Jan 20, 2016

Contributor

If you like I can revert all EF6 changes to make you merge easier.

@@ -202,6 +203,29 @@ private static void SetComputedAnnotation(EdmModel model, IEdmProperty target)
{
return null;
}
if (Type.GetTypeCode(propertyType) == TypeCode.DateTime)

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

I remember some .NET profiles have retired Type.GetTypeCode thus we copied the whole TypeCode enum into ODataLib. Is there any reason we are not using TypeHelper.IsDateTime() here?

This comment has been minimized.

@rayao

rayao Jan 20, 2016

Contributor

Oh it's my fault, this is written before I brought TypeHelper in, will change it.

{
public static Type GetUnderlyingTypeOrSelf(Type type)
{
return Nullable.GetUnderlyingType(type) ?? type;

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

Great! I forgot to take nullable temporal types into account when comparing type in ChangeSetPreparer and the converters. I will change to use this helper class for other type comparisons later.

@@ -121,7 +121,7 @@ protected override void OnModelCreating(Data.Entity.ModelBuilder modelBuilder)
modelBuilder.Entity<Order>(entityBuilder =>
{
entityBuilder.ToTable("Orders");
entityBuilder.HasMany(e => e.Order_Details).WithOne(e => e.Order).Required().ForeignKey(e => e.OrderID);
entityBuilder.HasMany(e => e.Order_Details).WithOne(e => e.Order).Required().ForeignKey(e => e.OrderID);

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

please use space instead of tab in source code.

// Then apply all the properties of the new instance to the instance to be updated. This will set any unspecified
// properties to their default value.
PropertyEntry propertyEntry = dbEntry.Property(propertyPair.Key);
Type type = propertyEntry.CurrentValue.GetType();

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

Actually here is a bug because the current value could be null thus there is no type here. I've fixed the bug in EF6 project but not EF7. I think we shall have a task #280 to sync the latest features from EF6 to EF7.

if (propertyType == typeof(TimeSpan))
{
RelationalPropertyAnnotations annotations = new RelationalPropertyAnnotations(efProperty, null);
var columnType = annotations.ColumnType;

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Jan 20, 2016

Contributor

Is this new feature in EF7's efProperty?

This comment has been minimized.

@rayao

rayao Jan 20, 2016

Contributor

Yeah this relies on EF7 annotations, EF7 model configurations work in this way.

@lewischeng-ms lewischeng-ms added this to the 0.4 milestone Jan 20, 2016

@rayao

This comment has been minimized.

Contributor

rayao commented Jan 20, 2016

Hi Lewis, perhaps I'd better remove all EF6 changes to make your merge easier? Or you can check in first and let me merge.

@lewischeng-ms

This comment has been minimized.

Contributor

lewischeng-ms commented Jan 21, 2016

@rayao, my PR need to be based on yours and use some code you wrote. So please address the comments and merge this PR first if all tests can pass and there is no code analysis issue during compilation. Thanks!

rayao added a commit that referenced this pull request Jan 21, 2016

Merge pull request #278 from rayao/BugFix
Add DateTime/Date/TimeOfDay support. Simplify EF7 entity update logic.
Borrowed Northwind DB config changes from Lewis's PR, to make baseline test pass.

@rayao rayao merged commit 7f71b2c into OData:master Jan 21, 2016

@rayao rayao deleted the rayao:BugFix branch Jan 21, 2016

@rayao rayao restored the rayao:BugFix branch Jan 21, 2016

@rayao

This comment has been minimized.

Contributor

rayao commented Jan 21, 2016

Sorry I messed up the branch hierarchy. Didn't notice there're check-ins before merge. Since it tells me "no conflict", I took that for "no change since last sync".
I'm not very familiar with git commands, I'm afraid of messing it up further, could someone help restore a single lineage?

@lewischeng-ms

This comment has been minimized.

Contributor

lewischeng-ms commented Jan 22, 2016

@rayao Don't worry about that. You didn't mess up the master branch history. It's OK to merge a pull request like this. For future, if you want to make the commits a single line, you can first do a git pull --rebase OData master to pull and rebase all the latest changes from OData/master then do a git push OData master to check in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment