NH-3488 - Strongly typed Inserts, Updates and Deletes #391
Conversation
|
A bit of clarification. While the delete part was easy enough - it just throws away the select and replaces it with a delete - the update and insert required a bit more. The concept used was to embed the UPDATE SET clause and the INSERT INTO [..] SELECT [..] in a projected SELECT clause.
becomes
By doing this, it is ensured that anything inside the clauses, such as subqueries, parameters etc., is going through the same rewriting process as everything else. It maybe feels like bit of a hack, and a full refactoring of the query modeling could open up for a more explicit path, but right now I think this is OK. Also, regarding the syntax. The reason the "fluent" stuff was needed was that without it...
would have to be
since the compiler can't infer just Dog, and if the base query contains a projection to an anonymous type, type inference is a must. Also, supporting both..
and
with the same method name is not possible, since the compiler can't distinguish between the two signatures when the lambda parameter is not used, unless the first one is qualified like this:
A minor nuisance maybe, but also something that could unnecessarily become a frequently asked question. The reason to use a fluent syntax for the Update and not only the Insert, which required it, was simply consistency. |
|
Any News to this Pull Req??? |
|
No, but any input you might have would be welcome. Especially regarding the syntax. I feel quite confident that the implementation itself is solid. |
|
I think the Syntax is quite good. We're using NHibernate quite long now in a Project. But not supporting Linq for Deletes and Updates, is a real show stopper. What are the conflicts wich are not yet resolved? |
|
Is there any active discussion to merge this? |
|
Sorry for leaving you hanging there. Not much discussion, since this was aimed for 5.0. |
|
Hi @gliljas, can you please enable Allow edits from maintainers feature? |
There was a problem hiding this comment.
I have locally rebased for having a look at this. It looks really great!
I have made some comments on the syntax as code review, along with some other suggestions.
Maybe adding Future version of executing methods would be nice, but it is not at all important I think.
There are some misses with white-spaces (spaces/tabs) I have fixed on my local branch.
Test cases sometime just do a "no op" for some tests, based on db support, maybe should they Assert.Ignore instead. A bunch of tests may have an additional teardown failure due to unclosed session if they fail, due to lack of using.
(Sorry for having messed up with single comments instead of "start review".)
src/NHibernate/Linq/UpdateSyntax.cs
Outdated
| /// <param name="assignments">The assignments.</param> | ||
| /// <param name="versioned">if set to <c>true</c> [versioned].</param> | ||
| /// <returns></returns> | ||
| public int Assign(Action<Assignments<T, T>> assignments, bool versioned = false) |
There was a problem hiding this comment.
Shouldn't it be versioned by default for entity having versioning, if possible? (I guess there are some hurdles indeed.)
The xml doc should maybe be written : <c>true</c> for having the update increments the entities version, <c>false</c> otherwise.
Instead of Assign, maybe Values would do.
| using NHibernate.Hql.Ast.ANTLR; | ||
| using NHibernate.Util; | ||
|
|
||
| namespace NHibernate.Test.NHSpecificTest.NH3438 |
There was a problem hiding this comment.
Typo, it should be 3488. All files and the older of the test cases for NH3488 have this typo. (Already renamed on my local branch.)
src/NHibernate/Linq/Assignments.cs
Outdated
| public Assignments<TInput, TOutput> Set<TProp>(Expression<Func<TOutput, TProp>> property, Expression<Func<TInput, TProp>> expression) | ||
| { | ||
| ArgumentUtility.CheckNotNull("property", property); | ||
| ArgumentUtility.CheckNotNull("expression", expression); |
There was a problem hiding this comment.
This utility seems to be no more available after rebasing, I have re-coded it "old style" on my local branch.
src/NHibernate/Linq/InsertSyntax.cs
Outdated
| /// <typeparam name="TOutput">The type of the output.</typeparam> | ||
| /// <param name="assignmentActions">The assignments.</param> | ||
| /// <returns></returns> | ||
| public int Into<TOutput>(Action<Assignments<TInput, TOutput>> assignmentActions) |
There was a problem hiding this comment.
Maybe renaming everywhere TInput and TOutput as TSource and TTarget would be more explicit. I have also thought about TSelected and TInserted but well, I am not sure.
And xml doc for TTarget could be "The type of the entities to insert."
Returns: "Number of inserted rows/entities"? What about insert into entities dispatched into join tables?
There was a problem hiding this comment.
Good points. I agree fully.
| /// <typeparam name="T"></typeparam> | ||
| /// <param name="query">The query.</param> | ||
| /// <returns></returns> | ||
| public static int Delete<T>(this IQueryable<T> query) |
There was a problem hiding this comment.
Contrary to Update and Insert, this one causes immediate execution. Maybe should we require an additional Execute call for aligning with Update and Insert.
There was a problem hiding this comment.
Possibly. I think that Delete().Execute() would look a bit strange, with a double verb.
There was a problem hiding this comment.
Yes, maybe would it be too "noisy" just for consistency.
|
"Allow edits from maintainers" has been enabled. |
|
May I force push my rebase? |
|
Yes! |
fredericDelaporte
force-pushed
the
NH3488
branch
from
f3d2b8d to
ae397c2
Compare
|
The tests are in fact the old Either we add them to the list of failings tests, or we fix them for being ignored on unsupported dialects. Some attempts at that were present, such as a test on I am in the process of modernizing those Linq tests and fixing their conditions. |
fredericDelaporte
force-pushed
the
NH3488
branch
from
492c6d6 to
b996884
Compare
| @@ -866,6 +910,11 @@ public void UpdateSetNullOnJoinedSubclass() | |||
| [Test] | |||
| public void DeleteWithSubquery() | |||
| { | |||
| if (Dialect is MsSqlCeDialect) | |||
| { | |||
| Assert.Ignore("Test failing on Ms SQL CE."); | |||
There was a problem hiding this comment.
Fails too in original BulkManipulationFixture, with a slightly different SQL query, but same error message. As this DB is deprecated since 2013, I do not intend to investigate more what is going on for this case.
|
Under Firebird And the db size endlessly grows... It looks like this kind of query under Firebird causes a recursive insert, as if newly inserted rows where taken into account in the Under SqlServerCe, there is a TeamCity build failure I do not understand. The log is huge due to the numerous known failing tests (700+), and at the end we can find: |
|
I have changed some xml comments according to earlier discussion, with some minor code cleanup. And made a third try at fixing the tests for accounting dialect unsupported features. |
|
SqlServerCe TeamCity build still failing, but looks like a test tooling failure. |
|
No, it is not. It means that the comparison produced the result file which has new failing tests.
|
|
Check the Comparison.txt file in build artifacts
|
|
The comparision.txt here to do that for us. |
|
Ok, I was not knowing we had this file for finding out which test causes trouble. |
fredericDelaporte
force-pushed
the
NH3488
branch
from
d359555 to
b04153e
Compare
|
I have updated the Linq reference documentation for documenting this new feature. |
fredericDelaporte
force-pushed
the
NH3488
branch
from
b04153e to
2fe62a4
Compare
fredericDelaporte
force-pushed
the
NH3488
branch
from
1aaec80 to
63d659e
Compare
Great for whose who do not wish to see that everywhere they use queryables and Nhibernate.Linq, but quite bad for discover-ability.
|
|
The linq2db alike insert syntax with individual setter was not working, I have updated my comparison comment. We are back to initial issue with inserts indeed. |
fredericDelaporte
force-pushed
the
NH3488
branch
from
32f6dd3 to
5538fb7
Compare
|
I have pushed an alternate syntax proposal. Commit to be dropped if preferring the previous one. InsertOld syntaxBy single selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Insert()
.As(c => new Pickup { Id = c.Id, Vin = c.Vin, Owner = c.Owner });By single anonymous selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Insert()
.As<Pickup>(c => new { Id = c.Id, Vin = c.Vin, Owner = c.Owner });By individual setterssession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Insert()
.Into<Pickup>(
a => a // assignments object
.Set(p => p.Id, c => c.Id)
.Set(p => p.Vin, c => c.Vin)
.Set(p => p.Owner, "The owner"));New linq2db alikeBy single selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.InsertInto(c => new Pickup { Id = c.Id, Vin = c.Vin, Owner = c.Owner });By single anonymous selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
// Sadly, have to re-specify the source type.
.InsertInto<Car, Pickup>(c => new { Id = c.Id, Vin = c.Vin, Owner = c.Owner });By individual setterssession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
// Value are not yielding a queryable anyway, so better be explicit
// with the fact we are toggling to a builder mode. Allows not re-specifying source type.
.InsertBuilder().Into<Pickup>()
.Value(p => p.Id, c => c.Id)
.Value(p => p.Vin, c => c.Vin)
.Value(p => p.Owner, "The owner")
.Insert();UpdateOld syntaxBy single selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Update() // .UpdateVersioned() available too
.As(c => new Car { Vin = c.Vin + "b", Owner = c.Owner }); By single anonymous selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Update() // .UpdateVersioned() available too
.As(c => new { Vin = c.Vin + "b", Owner = c.Owner }); By individual setterssession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Update() // .UpdateVersioned() available too
.Assign(
a => a // assignments object
.Set(c => c.Vin, c => c.Vin + "b")
.Set(c => c.Owner, "The owner"));New linq2db alikeBy single selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Update(c => new Car { Vin = c.Vin + "b", Owner = c.Owner }); // Or UpdateVersionedBy single anonymous selectorsession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
.Update(c => new { Vin = c.Vin + "b", Owner = c.Owner }); // Or UpdateVersionedBy individual setterssession
.Query<Car>()
.Where(c => c.UnitsInStock == 0)
// For consistency with insert case, and for telling explicitly we are switching away from
// queryable to a builder mode.
.UpdateBuilder()
.Set(c => c.Vin, c => c.Vin + "b")
.Set(c => c.Owner, "The owner")
.Update(); // Or UpdateVersionedDeleteSame with both. |
|
I think in linq2db you could use some set-functions, and after that you could add an additional where before you update (i've not tested, but i think it will work). |
|
No, we no more have a queryable from the point we start using one of these new methods. And that looks to be the same with linq2b: calling Anyway, maybe this would look more natural from a SQL standpoint to be able to put the |
* C#7 * White-spaces * Use of new ReflectionCache * ...
fredericDelaporte
force-pushed
the
NH3488
branch
from
e57f965 to
18134d7
Compare
|
Rebased, bulk manipulation fixture (non linq) refactoring dropped. |
hazzik
changed the title
|
This stuff will make NH live in enterprise much longer. Thanks! |
https://nhibernate.jira.com/browse/NH-3488
The bulk of the code in the commit is the unit tests, which is a copy of the tests in Hql/Ast/BulkManipulation.cs.
It would have been nice to reuse the same code but almost all the mappings in the original have casing problems, i.e they refer to lower case properties, but the properties are not lower cased. The HQL tests work, since they refer to the mapped properties, not the real ones. One solution would have been to check against the mapping, but I found the existing behavior strange and other parts of the Linq implementation doesn't do this, so I'll leave it as is for now.
The goal was to provide the new functionality by deriving from the existing Linq system, but that was easier said than done. Lots of code in NHibernate is very hard to derive from, with static calls and massive amounts of logic in the constructors. I decided to create derived types of NHLinqExpression, and handle most of the initialization in the constructor call. I could have created a derived QueryModelVisitor and IntermediateHqlTree too, but opted not to.
The syntax for the update and insert is worth discussing, but I wanted to support both a member initialization version and an explicit one. They could not be implemented with the same method name, since the compiler then sometimes will not be able to distinguish between the overloads.
For inserts we have:
session.Query<Cat>().Insert().As(x=>new Dog{Name=x.Name});or
session.Query<Cat>().Insert().Into<Dog>(x=>x.Set(y=>y.Name,y=>y.Name));For updates we have:
session.Query<Cat>().Update().As(x=>new Cat{Name=x.Name + " the cat"});or
session.Query<Cat>().Update().Assign(x=>x.Set(y=>y.Name,y=>y.Name + " the cat"));A better name than "Assign" is welcome. Or maybe for "Set".