Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ArgumentException in ProjectPlanCompiler caused by missing materializerContext argument in dynamic calls #2535

Merged
merged 5 commits into from
Oct 24, 2022
Merged

Fix ArgumentException in ProjectPlanCompiler caused by missing materializerContext argument in dynamic calls #2535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f2a34e3
Add failing test
habbes Oct 21, 2022
6880242
Fix issue
habbes Oct 21, 2022
8df3946
Use nameof instead of literals for dynamic method names
habbes Oct 21, 2022
d6b86c3
Refactoring
habbes Oct 21, 2022
b5dd111
Use base.MaterializerCache consistently
habbes Oct 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Microsoft.OData.Client/AtomMaterializerLog.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ internal class AtomMaterializerLog
/// <summary>
/// The materializer context.
/// </summary>
private IODataMaterializerContext materializerContext;
private readonly IODataMaterializerContext materializerContext;

#endregion Private fields

Expand All @@ -69,6 +69,7 @@ internal AtomMaterializerLog(MergeOption mergeOption, ClientEdmModel model, Enti
{
Debug.Assert(model != null, "model != null");
Debug.Assert(entityTracker != null, "entityTracker != null");
Debug.Assert(materializerContext != null, "materializerContext != null");

this.appendOnlyEntries = new Dictionary<Uri, ODataResource>(EqualityComparer<Uri>.Default);
this.mergeOption = mergeOption;
Expand Down
10 changes: 5 additions & 5 deletions src/Microsoft.OData.Client/BaseAsyncResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ internal abstract class BaseAsyncResult : IAsyncResult
/// <summary>wrapped request</summary>
protected PerRequest perRequest;

/// <summary>
/// Cache used to store temporary metadata used for materialization of OData items.
/// </summary>
protected MaterializerCache materializerCache = new MaterializerCache();

/// <summary>
/// The int equivalent for true.
/// </summary>
Expand Down Expand Up @@ -94,6 +89,11 @@ internal BaseAsyncResult(object source, string method, AsyncCallback callback, o
this.userState = state;
}

/// <summary>
/// Cache used to store temporary metadata used for materialization of OData items.
/// </summary>
protected MaterializerCache MaterializerCache { get; } = new MaterializerCache();

/// <summary>
/// This delegate exists to workaround limitations in the WP7 runtime.
/// When limitations on the number of parameters to Func&lt;&gt; are resolved, this can be subsumed by the following:
Expand Down
4 changes: 2 additions & 2 deletions src/Microsoft.OData.Client/BatchSaveResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ protected override MaterializeAtom GetMaterializer(EntityDescriptor entityDescri
/*projectionPlan*/ null,
this.currentOperationResponse.CreateResponseMessage(),
ODataPayloadKind.Resource,
this.materializerCache);
base.MaterializerCache);
}

/// <summary>
Expand Down Expand Up @@ -690,7 +690,7 @@ private IEnumerable<OperationResponse> HandleBatchResponse(ODataBatchReader batc
this.currentOperationResponse.Headers.GetHeader(XmlConstants.HttpContentType),
this.currentOperationResponse.CreateResponseMessage(),
query.PayloadKind,
this.materializerCache);
base.MaterializerCache);
qresponse = QueryOperationResponse.GetInstance(query.ElementType, this.currentOperationResponse.Headers, query, materializer);
}
}
Expand Down
9 changes: 7 additions & 2 deletions src/Microsoft.OData.Client/GroupByProjectionPlanCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,12 @@ private Expression CallValueForPath(Expression entry, Expression entryType, Proj
Debug.Assert(entry != null, "entry != null");
Debug.Assert(path != null, "path != null");

Expression result = CallMaterializer("ProjectionValueForPath", this.materializerExpression, entry, entryType, Expression.Constant(path, typeof(object)));
Expression result = CallMaterializer(
nameof(ODataEntityMaterializerInvoker.ProjectionValueForPath),
this.materializerExpression,
entry,
entryType,
Expression.Constant(path, typeof(object)));
this.annotations.Add(result, new ExpressionAnnotation() { Segment = path[path.Count - 1] });
return result;
}
Expand Down Expand Up @@ -523,7 +528,7 @@ private Expression RebindMethodCallForAggregationMethod(MethodCallExpression met
annotation.Segment.StartPath.Add(memberSegment);

Expression value = CallMaterializer(
"ProjectionDynamicValueForPath",
nameof(ODataEntityMaterializerInvoker.ProjectionDynamicValueForPath),
this.materializerExpression,
annotation.Segment.StartPath.RootEntry,
Expression.Constant(targetType, typeof(Type)),
Expand Down
32 changes: 22 additions & 10 deletions src/Microsoft.OData.Client/ProjectionPlanCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ internal class ProjectionPlanCompiler : ALinqExpressionVisitor
/// <param name="materializerContext">The materializer context.</param>
private ProjectionPlanCompiler(Dictionary<Expression, Expression> normalizerRewrites, IODataMaterializerContext materializerContext)
{
Debug.Assert(materializerContext != null, "materializerContext != null");

this.annotations = new Dictionary<Expression, ExpressionAnnotation>(ReferenceEqualityComparer<Expression>.Instance);
this.materializerExpression = Expression.Parameter(typeof(object), "mat");
this.normalizerRewrites = normalizerRewrites;
Expand Down Expand Up @@ -504,7 +506,12 @@ private static Expression RebindConstructor(ConstructorInfo info, params Express
/// <returns>A new expression with the call instance.</returns>
private Expression CallCheckValueForPathIsNull(Expression entry, Expression entryType, ProjectionPath path)
{
Expression result = CallMaterializer("ProjectionCheckValueForPathIsNull", entry, entryType, Expression.Constant(path, typeof(object)));
Expression result = CallMaterializer(
nameof(ODataEntityMaterializerInvoker.ProjectionCheckValueForPathIsNull),
entry,
entryType,
Expression.Constant(path, typeof(object)),
Expression.Constant(this.materializerContext));
this.annotations.Add(result, new ExpressionAnnotation() { Segment = path[path.Count - 1] });
return result;
}
Expand All @@ -519,7 +526,12 @@ private Expression CallValueForPath(Expression entry, Expression entryType, Proj
Debug.Assert(entry != null, "entry != null");
Debug.Assert(path != null, "path != null");

Expression result = CallMaterializer("ProjectionValueForPath", this.materializerExpression, entry, entryType, Expression.Constant(path, typeof(object)));
Expression result = CallMaterializer(
nameof(ODataEntityMaterializerInvoker.ProjectionValueForPath),
this.materializerExpression,
entry,
entryType,
Expression.Constant(path, typeof(object)));
this.annotations.Add(result, new ExpressionAnnotation() { Segment = path[path.Count - 1] });
return result;
}
Expand Down Expand Up @@ -671,7 +683,7 @@ private Expression RebindEntityMemberInit(MemberInitExpression init)
assignment.Expression.NodeType == ExpressionType.MemberInit))
{
Expression nestedEntry = CallMaterializer(
"ProjectionGetEntry",
nameof(ODataEntityMaterializerInvoker.ProjectionGetEntry),
entryParameterAtMemberInit,
Expression.Constant(assignment.Member.Name, typeof(string)),
Expression.Constant(this.materializerContext));
Expand Down Expand Up @@ -771,7 +783,7 @@ private Expression RebindEntityMemberInit(MemberInitExpression init)
}

Expression reboundExpression = CallMaterializer(
"ProjectionInitializeEntity",
nameof(ODataEntityMaterializerInvoker.ProjectionInitializeEntity),
this.materializerExpression,
entryToInitValue,
expectedParamValue,
Expand All @@ -797,7 +809,7 @@ private Expression GetDeepestEntry(Expression[] path)
do
{
result = CallMaterializer(
"ProjectionGetEntry",
nameof(ODataEntityMaterializerInvoker.ProjectionGetEntry),
result ?? this.pathBuilder.ParameterEntryInScope,
Expression.Constant(((MemberExpression)path[pathIndex]).Member.Name, typeof(string)));
pathIndex++;
Expand Down Expand Up @@ -1051,7 +1063,7 @@ private Expression RebindMethodCallForMemberSelect(MethodCallExpression call)
// helpers, eg to preserve paging information.
Type returnElementType = call.Method.ReturnType.GetGenericArguments()[0];
result = CallMaterializer(
"ProjectionSelect",
nameof(ODataEntityMaterializerInvoker.ProjectionSelect),
this.materializerExpression,
this.pathBuilder.ParameterEntryInScope,
this.pathBuilder.ExpectedParamTypeInScope,
Expand All @@ -1060,7 +1072,7 @@ private Expression RebindMethodCallForMemberSelect(MethodCallExpression call)
selectorExpression);
this.annotations.Add(result, annotation);
result = CallMaterializerWithType(
"EnumerateAsElementType",
nameof(ODataEntityMaterializerInvoker.EnumerateAsElementType),
new Type[] { returnElementType },
result);
this.annotations.Add(result, annotation);
Expand Down Expand Up @@ -1150,7 +1162,7 @@ private Expression RebindMethodCallForNewSequence(MethodCallExpression call)
// {(mat, entry1, type1) => Convert(ProjectionValueForPath(mat, entry1, type1, p->*.FirstName))}
Type returnElementType = call.Method.ReturnType.GetGenericArguments()[0];
result = CallMaterializer(
"ProjectionSelect",
nameof(ODataEntityMaterializerInvoker.ProjectionSelect),
this.materializerExpression,
this.pathBuilder.ParameterEntryInScope,
this.pathBuilder.ExpectedParamTypeInScope,
Expand All @@ -1159,7 +1171,7 @@ private Expression RebindMethodCallForNewSequence(MethodCallExpression call)
selectorExpression);
this.annotations.Add(result, annotation);
result = CallMaterializerWithType(
"EnumerateAsElementType",
nameof(ODataEntityMaterializerInvoker.EnumerateAsElementType),
new Type[] { returnElementType },
result);
this.annotations.Add(result, annotation);
Expand Down Expand Up @@ -1209,7 +1221,7 @@ private Expression TypedEnumerableToList(Expression source, Type targetType)

// Return the annotated expression.
Expression result = CallMaterializerWithType(
"ListAsElementType",
nameof(ODataEntityMaterializer.ListAsElementType),
new Type[] { enumeratedType, listElementType },
this.materializerExpression,
source);
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.OData.Client/QueryResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ private MaterializeAtom CreateMaterializer(ProjectionPlan plan, ODataPayloadKind
this.ContentType,
responseMessageWrapper,
payloadKind,
this.materializerCache);
base.MaterializerCache);
}
}
}
4 changes: 2 additions & 2 deletions src/Microsoft.OData.Client/SaveResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ protected override MaterializeAtom GetMaterializer(EntityDescriptor entityDescri
{
Debug.Assert(this.cachedResponse.Exception == null && this.cachedResponse.MaterializerEntry != null, "this.cachedResponse.Exception == null && this.cachedResponse.Entry != null");
ODataResource entry = this.cachedResponse.MaterializerEntry == null ? null : this.cachedResponse.MaterializerEntry.Entry;
return new MaterializeAtom(responseInfo, new[] { entry }, entityDescriptor.Entity.GetType(), this.cachedResponse.MaterializerEntry.Format, this.materializerCache);
return new MaterializeAtom(responseInfo, new[] { entry }, entityDescriptor.Entity.GetType(), this.cachedResponse.MaterializerEntry.Format, base.MaterializerCache);
}

/// <summary>
Expand Down Expand Up @@ -865,7 +865,7 @@ private void HandleOperationResponseData(IODataResponseMessage responseMsg, Stre
responseMsg.StatusCode,
() => responseStream);

ODataMaterializerContext materializerContext = new ODataMaterializerContext(responseInfo, this.materializerCache);
ODataMaterializerContext materializerContext = new ODataMaterializerContext(responseInfo, base.MaterializerCache);
entry = ODataReaderEntityMaterializer.ParseSingleEntityPayload(responseMessageWrapper, responseInfo, entityDescriptor.Entity.GetType(), materializerContext);
entityDescriptor.TransientEntityDescriptor = entry.EntityDescriptor;
}
Expand Down
19 changes: 19 additions & 0 deletions test/EndToEndTests/Tests/Client/AsynchronousTests/AsynchronousQueryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ namespace Microsoft.Test.OData.Tests.Client.AsynchronousTests
using System.Linq;
using System.Net;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.OData.Client;
using Microsoft.Test.OData.Services.TestServices;
using Microsoft.Test.OData.Services.TestServices.AstoriaDefaultServiceReference;
Expand Down Expand Up @@ -788,6 +789,24 @@ public void Linq_ProjectPropertiesFromEntityandExpandedEntity()
this.EnqueueTestComplete();
}

[Fact]
public async Task Linq_ProjectPropertiesFromEntityWithConditionalNullCheckOnExpandedEntity()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this test be the one that fails without your other changes? The LINQ query wasn't generating an expression with the materializer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was failing without my changes. It was throwing an ArgumentException because the ProjectPlanCompiler was generating a dynamic call to ProjectionCheckValueForPathIsNull without passing the required materializerContext argument.

{
var context = this.CreateWrappedContext<DefaultContainer>().Context;
var query = context.Computer.Where(c => c.ComputerId == -10)
.Select(c => new Computer
{
ComputerId = c.ComputerId,
ComputerDetail = c.ComputerDetail == null ? null : c.ComputerDetail,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is interesting, what does the ternary operator give us here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ternary operator here is used to force the ProjectPlanCompiler to call the method ProjectionCheckValueForPathIsNull, which is the one that had the bug. In this case, the result of that expression will be the value of c.ComputerDetail since it's not null. But the key thing here I believe is the null check against a nested EntityType.

}) as DataServiceQuery<Computer>;

var result = await query.ExecuteAsync();

var computer = result.First();
Assert.Equal(-10, computer.ComputerId);
Assert.Equal(-10, computer.ComputerDetail.ComputerDetailId);
}

/// <summary>
/// LINQ query Project Name Stream Property
/// </summary>
Expand Down
10 changes: 5 additions & 5 deletions ...Tests/Client.TDD.Tests/Tests/Materialization/CollectionValueMaterializationPolicyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void AddingCollectionToComplexCollectionShouldFail()
[Fact]
public void DataServicCollectionOfTAsCollectionTypeShouldFailForPrimitiveOrComplexCollections()
{
var testContext = new TestMaterializerContext(new MaterializerCache());
var testContext = new TestMaterializerContext();
var edmType = testContext.Model.GetOrCreateEdmType(typeof(MyInfo));
var clientTypeAnnotation = new ClientTypeAnnotation(edmType, typeof(MyInfo), "MyInfo", testContext.Model);

Expand All @@ -141,7 +141,7 @@ public void DataServicCollectionOfTAsCollectionTypeShouldFailForPrimitiveOrCompl
[Fact]
public void CreateCollectionInstanceShouldFailOnTypeWithNoParametersLessConstructors()
{
var testContext = new TestMaterializerContext(new MaterializerCache());
var testContext = new TestMaterializerContext();
var edmType = testContext.Model.GetOrCreateEdmType(typeof(ListWithNoEmptyConstructors));
var clientTypeAnnotation = new ClientTypeAnnotation(edmType, typeof(ListWithNoEmptyConstructors), "Points", testContext.Model);

Expand All @@ -153,7 +153,7 @@ public void CreateCollectionInstanceShouldFailOnTypeWithNoParametersLessConstruc
public void CreateCollectionPropertyInstanceShouldFailOnTypeWithNoParametersLessConstructors()
{
var odataProperty = new ODataProperty() { Name = "foo", Value = new ODataCollectionValue() { TypeName = "Points" } };
var testContext = new TestMaterializerContext(new MaterializerCache());
var testContext = new TestMaterializerContext();
testContext.ResolveTypeForMaterializationOverrideFunc = (Type type, string name) =>
{
var edmType = testContext.Model.GetOrCreateEdmType(typeof(ListWithNoEmptyConstructors));
Expand All @@ -167,7 +167,7 @@ public void CreateCollectionPropertyInstanceShouldFailOnTypeWithNoParametersLess
[Fact]
public void NonMissingMethodExceptionOnCreateInstanceShouldNotBeCaught()
{
var testContext = new TestMaterializerContext(new MaterializerCache());
var testContext = new TestMaterializerContext();
var edmType = testContext.Model.GetOrCreateEdmType(typeof(ListWithNoEmptyConstructors));
var clientTypeAnnotation = new ClientTypeAnnotation(edmType, typeof(ErrorThrowingList), "Points", testContext.Model);

Expand All @@ -181,7 +181,7 @@ public void NonMissingMethodExceptionOnCreateInstanceShouldNotBeCaught()

internal CollectionValueMaterializationPolicy CreateCollectionValueMaterializationPolicy()
{
return CreateCollectionValueMaterializationPolicy(new TestMaterializerContext(new MaterializerCache()));
return CreateCollectionValueMaterializationPolicy(new TestMaterializerContext());
}

internal CollectionValueMaterializationPolicy CreateCollectionValueMaterializationPolicy(IODataMaterializerContext materializerContext)
Expand Down
Loading