Throw for DML on filter #2020
Throw for DML on filter #2020
Conversation
| { | ||
| var a = await (s.Query<SimpleEntityWithAssociation>().Take(1).SingleOrDefaultAsync()); | ||
| var query = a.AssociatedEntities.AsQueryable(); | ||
| Assert.That(() => query.Delete(), Throws.InstanceOf<NotSupportedException>()); |
There was a problem hiding this comment.
@maca88 I've just noticed that async versions of DML extension methods are not used at all in async tests. Is it Async Generator bug?
There was a problem hiding this comment.
Yes it's a bug and I've created an issue for it.
There was a problem hiding this comment.
Published a new release 0.13.2 that has the bug fixed.
There was a problem hiding this comment.
I do not think we should upgrade the async generator on 5.2x (and even less on previous branches). So we will have to take care of regenerating async files as part of the merge to master I think, in order to take into account the fix now available in master.
|
Should probably go all the way down to 5.0.x |
bahusoid
force-pushed
the
throwFilterDml
branch
from
a500d85 to
0b1facd
Compare
bahusoid
force-pushed
the
throwFilterDml
branch
2 times, most recently
from
0b1facd to
8d21d3b
Compare
|
Too soon, @bahusoid. @fredericDelaporte your thoughts? |
This comment has been minimized.
This comment has been minimized.
|
Technically, I think we should anyway target first 5.2.x, then eventually backport (cherry-pick) to previous ones. So this PR here would go in 5.2.4, and #2011 should drop its release commit for letting it go in its own PR. |
bahusoid
force-pushed
the
throwFilterDml
branch
from
8d21d3b to
da20245
Compare
| @@ -282,6 +282,9 @@ public virtual void SetResultTransformerAndAdditionalCriteria(IQuery query, NhLi | |||
|
|
|||
| public int ExecuteDml<T>(QueryMode queryMode, Expression expression) | |||
| { | |||
| if (Collection != null) | |||
| throw new NotSupportedException("DML operations are not supported for filters."); | |||
There was a problem hiding this comment.
Should probably be InvalidOperationException. However, I'm not sure.
There was a problem hiding this comment.
As potentially it could be implemented I think NotSupported/NotImplemented exceptions are more appropriate.
There was a problem hiding this comment.
I do not think it deserves a critical label: this is about hard-failing instead of doing unexpected changes in database when attempting to use DML on collection filters (feature actually not available). So fixing it will not restore a critical feature, but "just" prevent some insufficiently tested mistake to corrupt data.
Still worth patching down to 5.0.x for avoiding bad surprises to users.
But it may be a possible breaking change, for those using it in its current state, eventually adding themselves the missing conditions, instead of using a queryable from the session. They may not have a reference to the session where they are doing that. I think we should do it anyway.
As otherwise DML is executed on the whole table.
Though it would be cool to support it.