Design meeting notes - December 4, 2014

[ajcvickers]

Graph behaviors

Goals

• Add relatively simple support for attaching graphs that can be done for RTM
• Try not to block future plans for higher-level services in this area
• Be more of a pit-of-success for attaching disconnected graphs than we have in the old stack

Background

The old stack always attached the entire graph of reachable entities in the same state. That is, if you called Attach, then everything reachable would be put in the Unchanged state. When attaching a graph where some entities already existed and some were new this meant that:

• The graph was forced through an "invalid" state where entities where marked in a state that they would not ultimately end up as. For Attach this invalid state was often cause an exception due to primary key conflicts on objects that did not yet have a key and therefore all had the default value for the CLR type--e.g. zero.
• Even if Add was used to avoid the exception from Attach, the application code was still forced to do a graph traversal to fixup the invalid state.
• There were no built-in mechanisms to determine what state new entities should be put in, so this had to be written by the application.

Ultimately, we want to support the following strategies for determining the state of new entities:

• Compare with existing graph, either containing full or partial information
  • This graph could be obtained by querying the database--i.e. the Merge behavior
  • Or the graph of state info could be obtained from information travelling with the entities through the client round-trip
• Use primary key values—new objects don’t have a PK set yet, so they should be Added, while other entities should be Unchanged (or possibly Modified)
• Attach aggregate root and entire aggregate is attached with given state

Proposal

The main part of the proposal is to introduce a new method

void AttachGraph(object root, Action<EntityEntry> callback)

This method starts at the root entity and uses graph traversal to find other entities in the graph. For each entity the callback delegate is invoked which can set the state of the entity as appropriate. The callback delegate could also do other things such as setting original values or adding shadow state.

Notes:

• Only entities that are not already being tracked will get the callback. That is, graph traversal in a particular direction will stop as soon as a tracked entity is encountered.
• Internally, this method will likely use a general purpose graph iterator for the model, which will be made publicly available.
• We considered returning a list of the entries that were attached, but decided this was YAGNI for now. It is not a re-compile breaking change to change void into something else later.
• We discussed if this method should go on the context, the set, both the context and set, or the change tracker. We decided on the change tracker.
• We will not add a params overload for multiple roots at this time.
• There will be an Async version because key generation can require a database access.

An additional part of the proposal is to introduce a simple version of AttachGraph:

void AttachGraph(object root)

This method would use a callback that looks at the primary key value to determine whether an entity should be Added or Unchanged.

Notes:

• It should be possible to get access to this callback such that it can be used together with other actions such as setting shadow state, or so that it can be used to mark objects as Modified instead of Unchanged.
• We should look at integration with DI such that the behavior of this method can be changed by service replacement.
• This strategy will only work when entities that should be Added do not have key values assigned and are making use of EF key generation. The method will throw if it encounters an entity that does not have key generation enabled and has no key value assigned.

We also discussed removing the single object Add/Attach/Update/Remove methods and replacing with:

EntityEntry<TEntity> Attach<TEntity>(TEntity entity, EntityState state)

This is because each of these four methods does exactly the same thing just with a different state. We decided not to do this as it was felt the existing methods would be easier to understand and preserves some level of experience from EF6. Also, this method is effectively the same as setting the state with context.Entry(Foo).State = newState. However we decided that given setting state can now be a significant amount of work and may be async we will make the State property read-only and add SetState and SetStateAsync methods.

DbSet Queries

Background

Currently DbSets are IQueryables and using them results in a database query. However, they also have Add, Remove, etc. and so act as a way to introduce entities into the unit of work, etc. But since the query is still a database query it means that if those new entities are not returned when the set is enumerated. This is usually unintuitive to new users. For example:

using(var db = new BloggingContext())
{
    var blog = new Blog();
    db.Blogs.Add(blog);
    Assert.True(db.Blogs.Contains(blog)); // Fails as database doesn't yet include the new blog
}

On the other hand, most people seem to like this syntax for writing database queries and do find it intuitive:

using(var db = new BloggingContext())
{
    var blogs = db.Blogs.ToList();
}

So having the DbSet work as a database query is not universally bad.

Options

One option which would be perhaps the most intuitive is to make DbSet queries integrate database results with local results and return the union of both. We would like to do this, but we don't have time to do it now. Given that we can't do this now, other options are:

• Keep the current current implementation
  • This is not intuitive to new users in the case where entities have been added to the set
  • This is consistent for users of previous versions of EF
  • The simple use of DbSet properties for database queries is preserved
• Make DbSet not work for queries at all and add properties/methods for different queries
  • DbSet.DatabaseQuery, DbSet.LocalQuery, etc.
  • DbSet would not be IQueryable/IEnumerable at all, and the simple database query pattern we have now would fail to compile
  • This option as the advantage that the type of query is explicit and discoverable, and new types of query can be added in the future.
  • The DbSet could maybe be made some form of query in a later release.
• The same as the previous option, except that DbSet acts as either a local or combined query.
  • As mentioned above, we do not have time to do the combined query at this time.
  • The problem with making it a local query is that simple code that would previously return results from the database will now do nothing, which will be very confusing.
• Remove Add/Remove from DbSet, or create a new type that represents just a database query and does not allow adding/removing entities.

We decided to go with the first option and keep the current semantics. This is because:

• Changing DbSet to not be a query at all will result in people having to figure out the new pattern and use it, and this new pattern is not much better for common database query cases.
• Changing DbSet to be just a local query will be even more confusing because people will expect database results and instead get nothing.
• Making DbSet return the combined results is scoped out at this time.

Hopefully in the future we can implement the combined query semantics and then enable this with a flag on the context or through a new DbSet type.

Migrations and namespaces

Recent changes mean that when a new migration is added it will be placed into the same namespace as the previous namespace for that context. There was agreement that this was good, but the following tweaks were suggested:

• Currently, the namespace is used to determine the folder location. No attempt is made to look at the file system. This is fine when namespaces match file system structure, but will result in people doing manual work to move scaffolded migrations when the file system and namespace to not align. To fix this, we will look for a file with the expected name in the project structure and, if found, use that as the folder location for the new migration. If no such file is found the namespace will be used as it is now.
• Currently migrations for a context are put in the Migrations folder/namespace. If multiple contexts are used then migrations for the new context must be manually moved. Instead, if migrations for a second context are scaffolded they will be put into a Migrations/ContextName folder/namespace. Note that this may require fully-qualifying the context name in the attribute to avoid conflicts.

Discussion

Please comment on this issue to provide feedback, ask questions, etc.

[julielerman]

I have a question about AttachGraph. Sometimes I have a graph and just want to attach or add the root and not worry about someone having reference data attached as a navigation property. For example if I have an address object and developer set Address.Country=entityInstanceOfSomeCountry, I want to avoid adding the country by mistake if I add the address. (There is the question of whether or not the CountryFK property has been populated.) Would be nice to have some method ala AttachRoot that would attach the root, work out FK values for scalar FK properties and leave the navigations null. I've been struggling with this in disconnected EF since v1. Thanks

[tonysneed]

Hi Julie,

I think the problem you're describing (setting entities for navigation properties to Added) won't be an issue in EF7, because Add will be non-recursive. For example, settings Address to Added will not automatically set Country to added. I had to contend with the legacy behavior in my Trackable Entities project where I set EntityState based on state metadata traveling with entities through the client round-trip, which made applying change state recursively quite tricky to say the least. :)

Thumbs up from me for the the improved support for the disconnected scenario, including the proposed AttachGraph method. I like the idea of making public the graph iterator, which could be re-purposed for client-side graph traversal, and I support plug-ability with DI integration.

Regarding the simplified version [void AttachGraph(object root)], the docs would need to clearly state that it's intended only for entities that use EF key generation (for ex, identity). So it wouldn't work with Customer from Northwind, where they PK is a string. I'm fine with that, however, because it's more of a "quick and dirty" version of AttachGraph.

Cheers,
Tony

[julielerman]

Thanks. I'll go play with this stuff.

On Dec 27, 2014, at 3:19 AM, Anthony Sneed notifications@github.com wrote:

Hi Julie,

I think the problem you're describing (setting entities for navigation properties to Added) won't be an issue in EF7, because Add will be non-recursive. For example, settings Address to Added will not automatically set Country to added. I had to contend with the legacy behavior in my Trackable Entities project where I set EntityState based on state metadata traveling with entities through the client round-trip, which made applying change state recursively quite tricky to say the least. :)

Thumbs up from me for the the improved support for the disconnected scenario, including the proposed AttachGraph method. I like the idea of making public the graph iterator, which could be re-purposed for client-side graph traversal, and I support plug-ability with DI integration.

Regarding the simplified version [void AttachGraph(object root)], the docs would need to clearly state that it's intended only for entities that use EF key generation (for ex, identity). So it wouldn't work with Customer from Northwind, where they PK is a string. I'm fine with that, however, because it's more of a "quick and dirty" version of AttachGraph.

Cheers,
Tony

—
Reply to this email directly or view it on GitHub.

[julielerman]

another point I'd like to mention is to please do what you can to provide consistent behavior. There are so many anomalies with current behavior of add/attach/etc. Sometimes there is logic to it if you really grok what's going on but in some cases, even I can't explain why results are different from one scenario to another. I think root add/attach/udpate/remove only affecting the root ... period ...should remove a bulk of the problems. It will be the proposed attachgraph method that you'll need to watch out for. Thanks

[rowanmiller]

@julielerman @tonysneed - We also have an overload of AttachGraph that takes an Action where you can set the state you want the entry to be in. Not setting the state means the entry remains effectively detached... so for the more complex scenarios you can always have logic that only attaches/adds certain entities.

[pacoweb]

I agree with @julielerman. Although you can use the overload I prefer a more explicit and easy to use method (AttachRoot). It would be especially useful for people starting to use the API.

[tonysneed]

I really like a version of AttachGraph that accepts an Action because it lets you set the entity state using whatever criteria you want. For example, I would set state based on some sort of metadata (tracking state), including property-level state for partial updates.

I'm not so crazy about a version of AttachGraph that actually sets state to Updated or Added. First, I'm not sure how useful it would be -- it would not support partial updates, deletes, or adds with non-integer keys. Second, the name is a bit misleading. Attach should, well, attach the entity, that is, set the state to UnChanged. But I'm still not sure how useful that would be, because you would have to "replay" the updates, which is not ideal, or call Add/Remove, which already attach the entity.

So my vote would be to keep this version:

void AttachGraph(object root, Action<EntityEntry> callback)

And drop this one:

void AttachGraph(object root)

Cheers,
Tony

[pacoweb]

I agree with @tonysneed that the AttachGraph method that accepts callback will be very useful . But (IMHO) I also think it is important to create a method (AttatchRoot like @julielerman says) to facilitate basic scenarios without having to worry about writing the callbak to customize the entity state.
Right now I have a couple of projects where I could use it.

Thanks!

[rowanmiller]

@pacoweb - How would AttachRoot differ from the DbSet.Attach which just attaches the entity that you pass to it?

[julielerman]

Yeah @pacoweb you may not realize that attach/add only touch the root now, not the graph. Big break from the past but probably the only way to ensure consistency. I wasn't aware of that when I opened this. So now the issue is working out if/when/how/which WRT attaching the rest of the graph (if there is one).

[pacoweb]

@rowanmiller - @julielerman - My only concern was the name . I like apis with explicit names. I like AttachGraph is clear. AttachRoot would also be very clear.

Thanks!

[adamvoss]

There does not appear to be any discussion surrounding deletions when attaching a graph. Is this tracked anywhere?

[ajcvickers]

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.