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

Design meeting notes - August 20, 2015 #2899

Closed
ajcvickers opened this issue Aug 21, 2015 · 16 comments
Closed

Design meeting notes - August 20, 2015 #2899

ajcvickers opened this issue Aug 21, 2015 · 16 comments
Milestone

Comments

@ajcvickers
Copy link
Member

ajcvickers commented Aug 21, 2015

Error handling in Data Annotations

Data annotations are user-written code so any error in the usage of data annotations should be notified to the user. Since data annotations can be used by frameworks other than EF and may not specify the full configuration of the model, throwing an exception in all erroneous cases would just block the user.

Therefore whenever data annotations are used in an ambiguous way, a warning will be logged using the logger. Though if the data annotations are specifying an invalid model configuration (esp. while using ForeignKey or InverseProperty), an error will be logged. Since the logger has not been implemented yet, an exception will be thrown at the places where error should be logged for now.

Below is the possible errors in data annotations and behavior of EF about them.

  • Key

    • KeyAttribute in derived type - If using TPH, this will throw while setting key since key cannot be set on derived entity type.
    • Composite Primary Key - Due to column ordering not being implemented, it is not possible to specify composite PK using data annotations. If there are multiple properties with Key attribute and if the PK is not specified using fluent API for such entity type then an exception will be thrown.
  • MaxLength& StringLength

    • Invalid length value - Since these attributes can be used by other frameworks also, EF will ignore if invalid value is specified. The length value should be greater than zero.
  • Timestamp

    • Timestamp on non-byte array property - In EF6, this threw an exception because it gets mapped to rowversion for SqlServer which requires the property to be byte array. In EF7, this will not throw any exception since provider other than SqlServer may provide a store type which is concurrency token and database generated but not byte array.
  • ForeignKey & InverseProperty

    • Since these attributes are used together to declare relationships, if the specified configuration is invalid relationship then exception will be thrown. In the case of ambiguity, warning will be logged and relationships will be created based on whatever can be interpreted from the given configuration. Following are some examples:
      • ForeignKey on property in both related entity types - 2 relationships will be formed with respective foreignkey property. If both navigations are connected by InverseProperty then throw.
      • ForeignKey on property and navigation in same entity type don't point to each other - throw.
      • ForeignKey on both navigations don't match - 2 relationships will be formed with each navigation and respective foreign key property. If both navigations are connected by InverseProperty then throw.
      • Composite FK - Due to no column ordering, composite FK must be specified on navigation using ForeignKey. If ForeignKey on multiple properties then throw.
      • Invalid list of FK properties on navigation - throw.
      • Navigation pointed by ForeignKey on property not found - will be implemented when logger is added.
      • Navigation pointed by InverseProperty not found or self or has different return type - throw.
      • InverseProperty on both navigations don't point to each other - throw.

Aligning property discovery with type mapping

Presently PropertyDiscoveryConvention uses fixed set of clr types to identify primitive properties, irrespective of what is supported by the providers. This puts limitation on what clr types can be included in the model as primitive properties. It can also create a model with properties which cannot be mapped by the current provider. Ideally, supported primitive types should be dependent on the current provider. To enforce this, property discovery will be aligned TypeMapper.

Following will be the workflow:

  • PropertyDiscoveryConvention will pull all the properties from entity's clr type which can be mapped by the current provider as primitive properties.
  • The remaining properties, will be added as navigation properties if possible. This would also discover new entity types if applicable.
  • Once the model is fully built, if all the properties in entity's clr type are not added as primitive/navigation properties or marked as ignored by user (using NotMapped attribute or fluent API), then exception will be thrown providing information of the property which cannot be mapped by TypeMapper.

Relationship API naming

We've been thru a few iterations of the relationship API in EF7 to see if we could come up with something that was a bit more intuitive. In past versions we have seen folks find the relationship API be the hardest part of EF to understand.

Based on feedback and our own experience using the API we don't think what we have now is necessarily much better than what was in EF6. We are also making an effort to align APIs with their EF6 equivalent unless there is a compelling reason not to.

There are a couple of changes in the EF7 API which we think are compelling to keep:

  • Removing the required/optional aspect of the relationship (i.e HasRequired/HasOptional) and just have it be based on the nullability of the foreign key property(s).
  • Removing the principal/dependent aspect of the relationship (i.e. WithRequiredPrincipal/WithRequiredDependent) which was super confusing in EF6 (we can never remember how it works and we designed it).

Based on this, we will be adopting the below API that keep some of the changes but more closely aligns with the EF6 naming. These changes are only API name changes and not funcitonal.

one to many

EF6

modelBuilder.Entity<Post>()
    .HasRequired(p => p.Blog)
    .WithMany(b => b.Posts);

EF7 Curent

modelBuilder.Entity<Post>()
    .Reference(p => p.Blog) // Required/optional comes from nullability of FK
    .InverseCollection(b => b.Posts);

EF7 New

modelBuilder.Entity<Post>()
    .HasOne(p => p.Blog) // Required/optional comes from nullability of FK
    .WithMany(b => b.Posts);

one to many with FK

EF6

modelBuilder.Entity<Post>()
    .HasRequired(p => p.Blog)
    .WithMany(b => b.Posts)
    .HasForeignKey(p => p.BlogId);

EF7 Current

modelBuilder.Entity<Post>()
    .Reference(p => p.Blog) // Required/optional comes from nullability of FK
    .InverseCollection(b => b.Posts)
    .ForeignKey(p => p.BlogId);

EF7 New

modelBuilder.Entity<Post>()
    .HasOne(p => p.Blog) // Required/optional comes from nullability of FK
    .WithMany(b => b.Posts)
    .HasForeignKey(p => p.BlogId);

one to zero-or-one

EF6

modelBuilder.Entity<ProductInfo>()
    .HasRequired(i => i.Product)
    .WithOptional(p => p.ProductInfo);

EF7 Current

modelBuilder.Entity<ProductInfo>()
    .Reference(i => i.Product)
    .InverseReference(p => p.ProductInfo);
modelBuilder.Entity<ProductInfo>()
    .Reference(i => i.Product)
    .InverseReference(p => p.ProductInfo)
    .ForeignKey<ProductInfo>(i => i.ProductId);

EF7 New

modelBuilder.Entity<ProductInfo>()
    .HasOne(i => i.Product)
    .WithOne(p => p.ProductInfo);
modelBuilder.Entity<ProductInfo>()
    .HasOne(i => i.Product)
    .WithOne(p => p.ProductInfo)
    .HasForeignKey<ProductInfo>(i => i.ProductId);

We will also rename PrincipalKey() to HasPrincipalKey() for consistency.

Discussion

Please comment below to provide feedback, ask questions, etc.

@ajcvickers ajcvickers added this to the Discussions milestone Aug 21, 2015
@natemcmaster
Copy link
Contributor

The design notes are still empty... https://github.com/aspnet/EntityFramework/wiki/Design-Meeting-Notes--August-20,-2015. I would like to see what I missed last week.

@rowanmiller
Copy link
Contributor

@natemcmaster just added the relationship API notes

@natemcmaster
Copy link
Contributor

@rowanmiller So, no updates yet for many-to-many relationships?

@divega
Copy link
Contributor

divega commented Aug 24, 2015

@natemcmaster not a v7 RTM feature.

@davidroth
Copy link
Contributor

How does WithOne distinguish between required and optional if ForeignKey properties are not used? Or does EF7 require FK properties?

@rowanmiller
Copy link
Contributor

@davidroth there is always a foreign key property... it just depends whether it is in your CLR class or in shadow state 😄. We do have a sugar .Required() API on the relationship that just calls .Required() on the foreign key for you (whether it is CLR or shadow state).

@jnm2
Copy link

jnm2 commented Sep 2, 2015

So this touches on something I've long wished for. It's great that CLR primitive type mappings will not be limited for the providers, but will it be possible to add your own primitive type mappings?

For example, we use a DateLocal struct extensively. It wraps a DateTime and has equivalent members and has an explicit conversion from DateTime that strips away time-of-day information and converts UTC time to local if necessary. We would like to use this instead of DateTime on entity date fields since it reinforces the fact that time of day will not be persisted. It also would integrate with our business logic perfectly. We wouldn't have to read the entity value as DateTime and explicitly cast to DateLocal every time when we know it was already persisted as such. DateTimeUtc has a similar task.

For another example, we occasionally use Double0To1 and PositiveInteger structs which do nothing more than enforce our own rules, e.g. throwing errors on explicit cast from int -1 to PositiveInteger. We would also like to introduce an Id<TEntity> struct so that primary keys and foreign keys can be compared on our entities with static type checking.

We have type converters for these types to easily convert between the custom struct and the wrapped CLR type, so data binding and serialization play well with these types. My question is, will our projects be able to hook into the type mapper and provide converters for these custom types so that we can use these types for EF primitive properties one day, even though providers will not know about them?

@duskreq
Copy link

duskreq commented Sep 2, 2015

EF team: I liked where you were headed with the semantic changes for defining relationships, but I also understand why you ultimately decided to return nearly full circle. However, I must admit that I'm not a big fan of inferring whether the relationship is required or optional based on the nullability of the foreign key's type. What if that logic varies from application to application and the domain classes are shared? Or what if the logic depends on certain conditions? Or what if the foreign key is defined in an abstract class and required/optional differs between concrete classes? I'd rather define that logic using the fluent API in my context class (potentially with conditional statements) than in my domain class. So, basically I'd prefer the EF6 syntax or the EF7 Current syntax over the EF7 New syntax.

How about this: what if you were to replace HasOne with Has and WithOne/WithMany with With, and then provide an optional enumeration parameter with values like ExactlyOne, ZeroOrOne, AtLeastOne, etc.

So, if the developer writes:
modelBuilder.Entity<Post>().Has(p => p.Blog).With(b => b.Posts);
EF can infer the cardinality based on whether Blog is nullable and whether Posts is a collection (thus achieving your goal).

But, if the developer writes:
modelBuilder.Entity<Post>().Has(p => p.Blog, Cardinality.ExactlyOne).With(b => b.Posts);
the developer is specifying that Blog is required even if it is nullable.

And this:
modelBuilder.Entity<Post>().Has(p => p.Blog).With(b => b.Posts, Cardinality.AtLeastOne);
means that EF should throw an exception if the application attempts to persist a blog with no posts.

That would also enable this:
Cardinality conditionalCardinality = (someCondition ? Cardinality.ExactlyOne : Cardinality.ZeroOrOne);
modelBuilder.Entity<Post>().Has(p => p.Blog, conditionalCardinality).With(b => b.Posts);

@smitpatel
Copy link
Member

@jnm2 - In past we have discussed about ability to add user-defined custom type mapping. Issue #242 track such scenarios. Though it is in backlog and probably won't be available in RTM.

@jnm2
Copy link

jnm2 commented Sep 3, 2015

@duskreq Wouldn't it impose consistency, which is a good thing? If the relationship is in fact zero or one at the persistence level, the possibility always exists that the data you pull will have a null foreign key (even with abstract classes). Why would you ever not want a nullable foreign key in that case? Similarly, if the relationship is in fact one to one, why would you ever want a nullable foreign key? If you're going to be explicit about the shape of the data, where better to be consistent and explicit than in the data model itself? In my mind, fluent config is best for things specific to EF but when it comes to specifying the type and relational shape of the data, that's what the data model is.

@duskreq
Copy link

duskreq commented Sep 4, 2015

@jnm2 In an ideal scenario, yes, imposing consistency is very good. However, I often work with a database for a third-party application, and I can't modify the schema lest I break the application. The database is littered with examples of "required" foreign keys on nullable columns. So, either the domain class is inconsistent with the database schema (i.e. I make the property non-nullable), or the context is inconsistent with the domain class. I happen to choose the former option, which will work fine in EF7 New, but I imagine at some point I will need to choose the latter option, which EF7 New wouldn't allow.

Also, consider this hypothetical scenario, which is closer to the gist of my concern. Say I create an application and sell it to two customers. Customer A wants the foreign key required but Customer B doesn't. In EF6 and EF7 Current I could make the foreign key property a nullable type, and then I could create an app.config setting and conditional logic in my context class, thus determining whether the foreign key is required at runtime. In EF7 New I would have to maintain separate code bases for the domain classes, as the nullability of the foreign key property must be predetermined at compile time.

@jnm2
Copy link

jnm2 commented Sep 4, 2015

@duskreq I follow that, but you can't rely on the third party database to provide a non-null value. If a null shows up in the database and you materialize your entities assuming it will never show up, that's a problem. Since the third party app is poorly designed, I would assume that's the nature of the game. You should deal with the poor design explicitly by modeling the nullability and checking for it instead of hiding the problem and hoping it never brings down your app (or causes a silent miscalculation). In other words, for the poorly designed third party database, null checking is a potential business concern.

Also, data models and domain models are inconsistent a lot of the time depending on context. Domain models often take a different shape than the persistence. I wouldn't be surprised if you're already handling an impedance mismatch between your data models and domain in other areas.

Your second scenario is similar. As long as the database might give you a null, why try to hide that in application B? What is gained by not coding both application A and application B to check for the null that might legally show up?

@rowanmiller
Copy link
Contributor

@duskreq By default the nullability of the FK property in the model is the same as the CLR type, but you can always make the FK required in the model without it being a non-nullable CLR type (the best example of this is wanting to make a string property required).

APIs like the one you proposed (modelBuilder.Entity<Post>().Has(p => p.Blog).With(b => b.Posts);) break down pretty quick with IntelliSense because the compiler often can't chose the right overload based on the lambda pointing to a collection or reference. This then breaks follow on APIs such as ForeignKey since the ability to provide strong typing relies on identifying which is the dependent/principal entity based on the previous API calls. We actually originally tried this approach in EF4.1 and then tried to make it work again in the early stages of EF7... but concluded again that it just doesn't work.

@duskreq
Copy link

duskreq commented Sep 4, 2015

Ah, so I would specify FKIdProp.Required() in a separate statement. That makes sense - thank you for clarifying.

@smitpatel
Copy link
Member

@duskreq - You can also chain Required() in same statement defining the relationship which will set the foreign key properties to be non-nullable.

@ajcvickers
Copy link
Member Author

We are closing this issue because no further action is planned for this issue. If you still have any issues or questions, please log a new issue with any additional details that you have.

@divega divega changed the title Design meeting discussion: August 20, 2015 Design meeting notes - August 20, 2015 Aug 31, 2017
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants