Design Meeting Notes - May 16, 2013
Time format in DbCommandLogger
It was suggested in a CodePlex discussion that the “executing at” time in the default DbCommandLogger output should be UTC time. This was discussed and the universal feeling on the team was that in this case local time formatted to current culture is preferable.
The reason for this is that the “timestamp” is not a timestamp designed to be used in the index of a log file or to be saved in an automated logging operation. It is instead a much more “conversational” log designed to produce human readable content probably
of most use in debugging. As such, the local time seems more appropriate and useful. However, we will look into adding time zone info to the output to make this absolutely clear. See
There was also discussion about whether or not the timestamp is even useful enough to include. The marginal opinion was that we should keep it, with the first person who can document a real-world case where it was useful getting a free beer.
Potential breaking change to IDbSet
The DbSet classes (generic and non-generic) inherit from a (generic or non-generic) IDbSet interface. IDbSet is intended only for creating test doubles, be these mocks or fakes. However, in EF6 DbSet has changed in four ways that if reflected in equivalent
changes for IDbSet would be breaking changes:
- FindAsync added
- AddRange/RemoveRange added
- Local return type changed to DbLocalView (this change may be reverted anyway)
The options discussed were:
- Make the breaking change
- EF6 has no binary compat story to EF5 anyway because of the move out of the Framework
- This is the easiest for anyone using a mocking framework such as Moq which will automatically handle the new members
- Manually created test doubles will break and need to be updated
- Doing this now leaves us in the same state the next time we want to add something to DbSet at which time there will hopefully be a binary compat story
- Make DbSet more mockable
- These means adding a protected constructor and making methods virtual
- Note that a type derived from DbSet that uses the protected constructor would create an object not be bound to any context and the methods would be no-ops. This makes it very like IDbSet from the perspective of creating test doubles.
- If we took this option we could potentially obsolete IDbSet
- It’s worth noting that no cases have been identified where this would be functionally different to using IDbSet for test doubles. However, there is strong feeling in the community that interfaces are preferred.
- Introduce an abstract base class
- This is essentially the same as the previous option except that DbSet itself stays the same and an abstract base class is instead inserted underneath it
- This has the advantage that it provides a way to communicate what the base type is for (test doubles)
- However, it adds a class that doesn’t really need to be there and means that you have to use this base class in your context if you ever want to use test doubles
- Use extension methods with delegation
- We have a pattern that we use for Include where the new members are added as extension methods which then delegate quickly into the real DbSet if the IDbSet is a real DbSet and otherwise use Reflection to call a method with the same signature on the test
- This works okay, but the pattern is obscure and people would likely have to know that the extension methods work in this way to use it effectively. Also, even when it is known that the extension method works in this way it can still be a bit confusing to
see how to use this with a mocking framework.
- Create a new interface type (e.g. IDbSet2) that extends the original interface type
- This meets all the requirements but is an ugly pattern that adds a new interface type when, conceptually, it is not needed. This gets even worse if we ever have to do it again.
The decision was to make DbSet more mockable. However, we will not obsolete IDbSet because this would create work for those currently using IDbSet who don’t need to use the new members. We will add guidance to IDbSet indicating that using DbSet is the way
to go for new code and, depending on feedback, we may choose to obsolete IDbSet in a future release.
Conversion to lightweight conventions
The configuration conventions used by EF are being converted to lightweight conventions are part of the the process to remove the “traditional” configuration conventions. As expected this has exposed some issues in the lightweight API for things that cannot
be done or cannot be done cleanly.
Navigation property configuration
It is currently not possible to configure navigation properties using lightweight conventions. We considered creating some version of the relationship mapping fluent API for lightweight conventions, but this API is both very tied to generic types and the
filtering they allow and also much more appropriate for configuring individual relationships rather than multiple relationships as is usually the case for conventions.
The proposed solution is to add a NavigationProperty method when using the Entities API that mirrors the Property method but has members suitable for navigation property configuration rather than scalar property configuration. We considered collapsing these
into just one Property method but the intersection of members is very small. Also, if this API is reflected on the normal fluent API then it makes sense to separate the concepts both to retain the fluent type filtering and keep the two concepts conceptually
- Should we have a top-level NavigationProperties method that mirrors the top-level Properties method? We decided not to do this now for two reasons:
- We don’t have an actual scenario where we need it and while it is possible to think of some where it might be useful the only ones we could think of would not work because of the second reason:
- We have no way of knowing if the potential navigation property will remain a navigation property in the final model. This is because every reference between types is initially considered a navigation property. However, if at a later time one of the types
is configured as a complex type or turned into a complex type by convention, then the potential navigation properties are no longer actual navigation properties.
- Should Properties also include navigation properties? We decided to not do this now for both the same reasons that we are not doing NavigationProperties and also because it is consistent with other APIs that navigation properties be considered differently
from scalar properties.
- We could choose to have an additional common API in the future, similar to what we have for the DbContext change tracker API.
- Should we introduce a NavigationProperty API on the non-conventions fluent API?
- This will likely be needed for o/c mapping changes such as mapping to fields.
- We will not do this for EF6 because it isn’t needed for anything we are doing in this release, but we will likely introduce it in the future.
Entities returning complex types
There are currently a few issues with the Entities methods:
- Entities currently returns only entity types, but we now need Entities to return types that have been configured as complex types.
- Currently, some types returned by Entities may get later transformed into complex types, which can be confusing.
- Entities shows up before Entity in Intelisense, which is annoying.
Given this we will rename this method to “Types”. Note that any configuration done on a type that can be done on either a complex type or an entity type should not change whether or not change the type from a complex type to an entity type. On the other
hand, explicitly configuring the type as a complex/entity type in the convention should be possible and honored. Also, configurations that imply and entity type (such as key configuration or navigation property configuration) should influence the complex type
convention in the normal way.
ModelNamespaceConvention is the only convention that cannot be converted to lightweight conventions. This is a convention used by DbContext to set the model namespace to the DbContext namespace. This makes the EDM look nicer for those cases where the EDM
is exposed, such as OData. It has always been possible to remove this convention, but never possible to use it to change the namespace in some other way. We have also not seen any requests to be able to do this. (The model namespace is never exposed for the
vast majority of EF users.)
- Add a property on DbModelBuilder and obsolete the convention. The problem with this is that it pollutes the model builder API for something that will hardly ever be used.
- Leave as is. This doesn’t substantially change the consolidation work and so is probably okay.
- Leave as is, but make the constructor public. This allows people to change the model namespace to something else if they want to.
We will leave as is but make the constructor public. Making the constructor public has very little value but also a very low risk change such that there doesn’t seem like a reason not to do it.
Lightweight conventions are wrapped as configuration conventions
This is the mechanism that allows a lightweight convention to be factored into a class and then added with the traditional convention API. The question is whether or not this should change given the move to only use lightweight conventions? The decision
was to leave the design as is since it seems to work generally as people expect including when the conventions get run. It would be possible to call Entities again inside the outer Entities call, and this would not work as expected, but this is not a natural
pattern and doesn’t seem like something to be too concerned about.