Support ReadItem for no-tracking queries #33927
Conversation
ajcvickers
force-pushed
the
PrettyLittleKeys
branch
from
fe4e2b4
to
8c4f823
Compare
src/EFCore.Cosmos/Metadata/Conventions/CosmosRuntimeModelConvention.cs
Outdated
Show resolved
Hide resolved
| @@ -92,8 +93,7 @@ public class CosmosQueryableMethodTranslatingExpressionVisitor : QueryableMethod | |||
| [return: NotNullIfNotNull(nameof(expression))] | |||
| public override Expression? Visit(Expression? expression) | |||
| { | |||
| if (_queryCompilationContext.QueryTrackingBehavior == QueryTrackingBehavior.TrackAll // Issue #33893 | |||
| && expression is MethodCallExpression | |||
| if (expression is MethodCallExpression | |||
There was a problem hiding this comment.
(not quite related to this PR)
This being in Visit() means that it's done recursively for every node - although we should only pattern-match on the top-level node. In other words, if the whole ReadItem-compatible fragment is composed upon with something, we probably incorrectly convert (and have a bug). To fix this, we should override QueryableMethodTranslatingExpressionVisitor.Translate() and have it there.
As a nit, I'd also move the logic to a dedicated function (TryTransformToReadItem) just to make it extra clear what's being done here.
There was a problem hiding this comment.
@ajcvickers noticed this again while working in an area of the code; opened #34085 to make sure we don't forget it.
src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs
Show resolved
Hide resolved
...mos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.ReadItemQueryingEnumerable.cs
Outdated
Show resolved
Hide resolved
| } | ||
| else | ||
| { | ||
| Check.DebugFail("Parameters should cover all properties or we should not be using ReadItem."); |
There was a problem hiding this comment.
nit: I've switched to throwing UnreachableException in these cases, since Check.DebugFail is for DEBUG-only, i.e. to get the assertion to trip in actual released bits (but not super important).
|
@AndriySvyryd @roji There is a meta point that probably should be discussed here. In current versions of the Cosmos provider, a user can put their own value generator on the |
|
@ajcvickers yeah, understood. We should maybe have a discussion on this in design... |
|
@ajcvickers Users can still use their implementation of |
ajcvickers
force-pushed
the
PrettyLittleKeys
branch
from
8c4f823
to
bf2d7a8
Compare
|
@AndriySvyryd @roji New version up based on Andriy's suggestion. |
| protected override void InitializeModel(IModel model, bool designTime, bool prevalidation) | ||
| { | ||
| base.InitializeModel(model, designTime, prevalidation); | ||
| model.SetRuntimeAnnotation(CosmosAnnotationNames.ModelDependencies, CosmosDependencies); |
There was a problem hiding this comment.
Only set this if prevalidation is true or false
There was a problem hiding this comment.
Only set this if prevalidation is true or false
prevalidation is a non-nullable bool...
There was a problem hiding this comment.
Only set it when it's true xor false.
Basically, only do it once.
There was a problem hiding this comment.
@AndriySvyryd I'm obviously missing something fundamental here. prevalidation is a single bool value. What am I xoring that with?
There was a problem hiding this comment.
C# is more precise than English. Add this after the first line:
if (!prevalidation)
{
return;
}
src/EFCore.Cosmos/Metadata/Internal/CosmosEntityTypeExtensions.cs
Outdated
Show resolved
Hide resolved
Part of #20693 Part of #33893 There is a lot left to do here, but I'm making a break here to get reviews before it goes too far. Major changes here are: - Discover and record properties used to form the JSON `id` in one place. - Use this to generate ate `id` values without tracking an instance. (Makes no-tracking work, needed for Reload.) - Be better at detecting only detecting patterns we can later translate. Next up: be better at detecting non-Find query patterns that we can translate.
ajcvickers
force-pushed
the
PrettyLittleKeys
branch
from
bf2d7a8
to
ba59905
Compare
Part of #20693
Part of #33893
There is a lot left to do here, but I'm making a break here to get reviews before it goes too far.
Major changes here are:
idin one place.idvalues without tracking an instance. (Makes no-tracking work, needed for Reload.)Next up: be better at detecting non-Find query patterns that we can translate.