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.

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

Fixes #766, Support in memory filtering after ODataQueryOptions.ApplyTo #824

Open
wants to merge 1 commit into
base: release-8.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System;
using System.Collections.Concurrent;
using System.Diagnostics.Contracts;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.OData.Common;
using Microsoft.AspNetCore.OData.Edm;
Expand Down Expand Up @@ -108,14 +109,32 @@ internal static Func<object, object> GetOrCreatePropertyGetter(

private static Func<object, object> CreatePropertyGetter(Type type, string propertyName)
{
PropertyInfo property = type.GetProperty(propertyName);
PropertyInfo propertyInfo = null;
var properties = type.GetProperties().Where(p => p.Name == propertyName).ToArray();
if (properties.Length <= 0)
{
propertyInfo = null;
}
else if (properties.Length == 1)
{
propertyInfo = properties[0];
}
else
{
// resolve 'new' modifier
propertyInfo = properties.FirstOrDefault(p => p.DeclaringType == type);
if (propertyInfo == null)
{
propertyInfo = properties[0];
}
}
Comment on lines +112 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very convoluted to me. Wouldn't this result in the exact same behavior?

Suggested change
PropertyInfo propertyInfo = null;
var properties = type.GetProperties().Where(p => p.Name == propertyName).ToArray();
if (properties.Length <= 0)
{
propertyInfo = null;
}
else if (properties.Length == 1)
{
propertyInfo = properties[0];
}
else
{
// resolve 'new' modifier
propertyInfo = properties.FirstOrDefault(p => p.DeclaringType == type);
if (propertyInfo == null)
{
propertyInfo = properties[0];
}
}
var propertyInfo = type
.GetProperties()
.FirstOrDefault(p => p.Name == propertyName && p.DeclaringType == type);

Also, I wonder if it isn't possible to use one of the GetProperty overloads with BindingFlags to get the same behavior in an even cleaner way without having to go through every single property on the type.

Did you check that possibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did use BindingFlags, but it seems it can't work. (Maybe what I tried is not enough).


if (property == null)
if (propertyInfo == null)
{
return null;
}

var helper = new PropertyHelper(property);
var helper = new PropertyHelper(propertyInfo);

return helper.GetValue;
}
Expand Down
52 changes: 52 additions & 0 deletions src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml
Original file line number Diff line number Diff line change
Expand Up @@ -9618,6 +9618,48 @@
<param name="request">A http request containing the query options.</param>
<returns>A string representing the query options part of an OData URL.</returns>
</member>
<member name="T:Microsoft.AspNetCore.OData.Query.ODataCastOptions">
<summary>
The cast options.
</summary>
</member>
<member name="P:Microsoft.AspNetCore.OData.Query.ODataCastOptions.MapProvider">
<summary>
Gets/sets the map provider.
</summary>
</member>
<member name="T:Microsoft.AspNetCore.OData.Query.IQueryableODataExtension">
<summary>
Provides a set of static methods for querying data structures that implement <see cref="T:System.Linq.IQueryable"/>
</summary>
</member>
<member name="M:Microsoft.AspNetCore.OData.Query.IQueryableODataExtension.OCast``1(System.Object,Microsoft.AspNetCore.OData.Query.ODataCastOptions)">
<summary>
Converts the source to the specified type.
</summary>
<typeparam name="TResult">The type to convert the source to.</typeparam>
<param name="source">The source.</param>
<param name="options">The cast options.</param>
<returns>The converted object if it's OData object. Otherwise, return same source or the default.</returns>
</member>
<member name="M:Microsoft.AspNetCore.OData.Query.IQueryableODataExtension.OCast``1(System.Linq.IQueryable,Microsoft.AspNetCore.OData.Query.ODataCastOptions)">
<summary>
Converts the elements of an <see cref="T:System.Linq.IQueryable"/> to the specified type.
</summary>
<typeparam name="TResult">The type to convert the elements of source to.</typeparam>
<param name="source">The <see cref="T:System.Linq.IQueryable"/> that contains the elements to be converted.</param>
<param name="options">The cast options.</param>
<returns>Contains each element of the source sequence converted to the specified type.</returns>
</member>
<member name="M:Microsoft.AspNetCore.OData.Query.IQueryableODataExtension.CreateInstance(Microsoft.AspNetCore.OData.Query.Wrapper.SelectExpandWrapper,System.Type,Microsoft.AspNetCore.OData.Query.ODataCastOptions)">
<summary>
Create the object based on <see cref="T:Microsoft.AspNetCore.OData.Query.Wrapper.SelectExpandWrapper"/>
</summary>
<param name="wrapper">The select expand wrapper.</param>
<param name="resultType">The created type.</param>
<param name="options">The options.</param>
<returns>The created object.</returns>
</member>
<member name="T:Microsoft.AspNetCore.OData.Query.ModelBoundQuerySettingsExtensions">
<summary>
This class describes the model bound settings to use during query composition.
Expand Down Expand Up @@ -12240,6 +12282,11 @@
Indicates whether the underlying instance can be used to obtain property values.
</summary>
</member>
<member name="P:Microsoft.AspNetCore.OData.Query.Wrapper.SelectExpandWrapper.InstanceValue">
<summary>
Gets the instance value.
</summary>
</member>
<member name="M:Microsoft.AspNetCore.OData.Query.Wrapper.SelectExpandWrapper.GetEdmType">
<inheritdoc />
</member>
Expand Down Expand Up @@ -12277,6 +12324,11 @@
Gets or sets the instance of the element being selected and expanded.
</summary>
</member>
<member name="P:Microsoft.AspNetCore.OData.Query.Wrapper.SelectExpandWrapper`1.InstanceValue">
<summary>
Gets the instance value.
</summary>
</member>
<member name="T:Microsoft.AspNetCore.OData.Results.BadRequestODataResult">
<summary>
Represents a result that when executed will produce a Bad Request (400) response.
Expand Down
7 changes: 7 additions & 0 deletions src/Microsoft.AspNetCore.OData/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,11 @@ Microsoft.AspNetCore.OData.Query.HttpRequestODataQueryExtensions
Microsoft.AspNetCore.OData.Query.IODataQueryRequestParser
Microsoft.AspNetCore.OData.Query.IODataQueryRequestParser.CanParse(Microsoft.AspNetCore.Http.HttpRequest request) -> bool
Microsoft.AspNetCore.OData.Query.IODataQueryRequestParser.ParseAsync(Microsoft.AspNetCore.Http.HttpRequest request) -> System.Threading.Tasks.Task<string>
Microsoft.AspNetCore.OData.Query.IQueryableODataExtension
Microsoft.AspNetCore.OData.Query.ODataCastOptions
Microsoft.AspNetCore.OData.Query.ODataCastOptions.MapProvider.get -> System.Func<Microsoft.OData.Edm.IEdmModel, Microsoft.OData.Edm.IEdmStructuredType, Microsoft.AspNetCore.OData.Query.Container.IPropertyMapper>
Microsoft.AspNetCore.OData.Query.ODataCastOptions.MapProvider.set -> void
Microsoft.AspNetCore.OData.Query.ODataCastOptions.ODataCastOptions() -> void
Microsoft.AspNetCore.OData.Query.ODataQueryContext
Microsoft.AspNetCore.OData.Query.ODataQueryContext.DefaultQuerySettings.get -> Microsoft.OData.ModelBuilder.Config.DefaultQuerySettings
Microsoft.AspNetCore.OData.Query.ODataQueryContext.ElementClrType.get -> System.Type
Expand Down Expand Up @@ -1709,6 +1714,8 @@ static Microsoft.AspNetCore.OData.Query.Expressions.QueryBinder.ApplyNullPropaga
static Microsoft.AspNetCore.OData.Query.Expressions.QueryBinder.GetDynamicPropertyContainer(Microsoft.OData.UriParser.SingleValueOpenPropertyAccessNode openNode, Microsoft.AspNetCore.OData.Query.Expressions.QueryBinderContext context) -> System.Reflection.PropertyInfo
static Microsoft.AspNetCore.OData.Query.HttpRequestODataQueryExtensions.GetETag(this Microsoft.AspNetCore.Http.HttpRequest request, Microsoft.Net.Http.Headers.EntityTagHeaderValue entityTagHeaderValue) -> Microsoft.AspNetCore.OData.Query.ETag
static Microsoft.AspNetCore.OData.Query.HttpRequestODataQueryExtensions.GetETag<TEntity>(this Microsoft.AspNetCore.Http.HttpRequest request, Microsoft.Net.Http.Headers.EntityTagHeaderValue entityTagHeaderValue) -> Microsoft.AspNetCore.OData.Query.ETag<TEntity>
static Microsoft.AspNetCore.OData.Query.IQueryableODataExtension.OCast<TResult>(this object source, Microsoft.AspNetCore.OData.Query.ODataCastOptions options = null) -> TResult
static Microsoft.AspNetCore.OData.Query.IQueryableODataExtension.OCast<TResult>(this System.Linq.IQueryable source, Microsoft.AspNetCore.OData.Query.ODataCastOptions options = null) -> System.Collections.Generic.IEnumerable<TResult>
static Microsoft.AspNetCore.OData.Query.ODataQueryOptions.IsSystemQueryOption(string queryOptionName) -> bool
static Microsoft.AspNetCore.OData.Query.ODataQueryOptions.IsSystemQueryOption(string queryOptionName, bool isDollarSignOptional) -> bool
static Microsoft.AspNetCore.OData.Query.ODataQueryOptions.LimitResults<T>(System.Linq.IQueryable<T> queryable, int limit, bool parameterize, out bool resultsLimited) -> System.Linq.IQueryable<T>
Expand Down
218 changes: 218 additions & 0 deletions src/Microsoft.AspNetCore.OData/Query/IQueryableODataExtension.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
//-----------------------------------------------------------------------------
// <copyright file="IQueryableODataExtension.cs" company=".NET Foundation">
// Copyright (c) .NET Foundation and Contributors. All rights reserved.
// See License.txt in the project root for license information.
// </copyright>
//------------------------------------------------------------------------------

using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.OData.Common;
using Microsoft.AspNetCore.OData.Edm;
using Microsoft.AspNetCore.OData.Formatter.Deserialization;
using Microsoft.AspNetCore.OData.Query.Container;
using Microsoft.AspNetCore.OData.Query.Wrapper;
using Microsoft.OData;
using Microsoft.OData.Edm;

namespace Microsoft.AspNetCore.OData.Query
{
/// <summary>
/// The cast options.
/// </summary>
public class ODataCastOptions
{
/// <summary>
/// Gets/sets the map provider.
/// </summary>
public Func<IEdmModel, IEdmStructuredType, IPropertyMapper> MapProvider { get; set; }
}
Comment on lines +25 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move each class to its own file?


/// <summary>
/// Provides a set of static methods for querying data structures that implement <see cref="IQueryable"/>
/// </summary>
public static class IQueryableODataExtension
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, extension classes on interfaces do not have the I in the name:

Suggested change
public static class IQueryableODataExtension
public static class QueryableODataExtension

{
/// <summary>
/// Converts the source to the specified type.
/// </summary>
/// <typeparam name="TResult">The type to convert the source to.</typeparam>
/// <param name="source">The source.</param>
/// <param name="options">The cast options.</param>
/// <returns>The converted object if it's OData object. Otherwise, return same source or the default.</returns>
[SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>")]
Copy link
Contributor

Choose a reason for hiding this comment

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

If you really want to suppress this, we should add the justification there.

public static TResult OCast<TResult>(this object source, ODataCastOptions options = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "OCast" is a really bad name that is not intuitive/semantic at all...

I don't have an immediate suggestion, but I'd strongly suggest reconsidering this name.

{
if (source is null)
{
return default;
}

Type sourceType = source.GetType();
if (source is SelectExpandWrapper wrapper)
{
return (TResult)CreateInstance(wrapper, sourceType, options);
}

if (typeof(TResult) == sourceType)
Copy link
Contributor

Choose a reason for hiding this comment

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

This check looks weird to me as it is. Would recommend swapping the values:

Suggested change
if (typeof(TResult) == sourceType)
if (sourceType == typeof(TResult))

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just do:

Suggested change
if (typeof(TResult) == sourceType)
if (source is TResult resultSource)

?

{
return (TResult)source;
}

if (typeof(TResult).IsAssignableFrom(sourceType))
{
return (TResult)Convert.ChangeType(source, typeof(TResult), CultureInfo.InvariantCulture);
}

return default;
}

/// <summary>
/// Converts the elements of an <see cref="IQueryable"/> to the specified type.
/// </summary>
/// <typeparam name="TResult">The type to convert the elements of source to.</typeparam>
/// <param name="source">The <see cref="IQueryable"/> that contains the elements to be converted.</param>
/// <param name="options">The cast options.</param>
/// <returns>Contains each element of the source sequence converted to the specified type.</returns>
[SuppressMessage("ApiDesign", "RS0026:Do not add multiple public overloads with optional parameters", Justification = "<Pending>")]
public static IEnumerable<TResult> OCast<TResult>(this IQueryable source, ODataCastOptions options = null)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we cast to iqueryable and then do the operations? in cases where the queryable is not a selectexpand, what's written here will result in executing the query on the server just to find out that the method should have been a no-op

if (source is null)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

this won't throw until enumeration; use .select instead

throw Error.ArgumentNull(nameof(source));
}

foreach (var item in source)
{
yield return OCast<TResult>(item, options);
}
}

/// <summary>
/// Create the object based on <see cref="SelectExpandWrapper"/>
/// </summary>
/// <param name="wrapper">The select expand wrapper.</param>
/// <param name="resultType">The created type.</param>
/// <param name="options">The options.</param>
/// <returns>The created object.</returns>
private static object CreateInstance(SelectExpandWrapper wrapper, Type resultType, ODataCastOptions options)
{
object instance;
Type instanceType = resultType;
IEdmModel model = wrapper.Model;
if (wrapper.UseInstanceForProperties)
{
instance = wrapper.InstanceValue;
instanceType = instance.GetType();
}
else
{
if (wrapper.InstanceType != null && wrapper.InstanceType != resultType.FullName)
{
// inheritance
IEdmTypeReference typeReference = wrapper.GetEdmType();
instanceType = model.GetClrType(typeReference); // inheritance type
}

instance = Activator.CreateInstance(instanceType);
}

IDictionary<string, object> properties;
if (options != null && options.MapProvider != null)
{
properties = wrapper.ToDictionary(options.MapProvider);
}
Comment on lines +126 to +129
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer pattern matching for this type of check+extract:

Suggested change
if (options != null && options.MapProvider != null)
{
properties = wrapper.ToDictionary(options.MapProvider);
}
if (options is { MapProvider: not null and var provider})
{
properties = wrapper.ToDictionary(provider);
}

else
{
properties = wrapper.ToDictionary();
}

foreach (var property in properties)
{
string propertyName = property.Key;
object propertyValue = property.Value;
Comment on lines +135 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

Since property is a KeyValuePair:

Suggested change
foreach (var property in properties)
{
string propertyName = property.Key;
object propertyValue = property.Value;
foreach (var (propertyName, propertyValue) in properties)
{

if (propertyValue == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency in new code.

Suggested change
if (propertyValue == null)
if (propertyValue is null)

{
// If it's null, we don't need to do anything.
continue;
}

PropertyInfo propertyInfo = GetPropertyInfo(instanceType, propertyName);
if (propertyInfo == null)
{
throw new ODataException(Error.Format(SRResources.PropertyNotFound, instanceType.FullName, propertyName));
}

bool isCollection = TypeHelper.IsCollection(propertyInfo.PropertyType, out Type elementType);

if (isCollection)
{
IList<object> collection = new List<object>();
IEnumerable collectionPropertyValue = propertyValue as IEnumerable;
foreach (var item in collectionPropertyValue)
Comment on lines +156 to +157
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a mistake. as should not be used when the semantic is "the value must be of the type", since it can return null and this will cause a NullReferenceExeception in the foreach.

Suggested change
IEnumerable collectionPropertyValue = propertyValue as IEnumerable;
foreach (var item in collectionPropertyValue)
IEnumerable collectionPropertyValue = (IEnumerable)propertyValue;
foreach (var item in collectionPropertyValue)

{
object itemValue = CreateValue(item, elementType, options);
collection.Add(itemValue);
}

IEdmTypeReference typeRef = model.GetEdmTypeReference(propertyInfo.PropertyType);
IEdmCollectionTypeReference collectionTypeRef = typeRef.AsCollection();

DeserializationHelpers.SetCollectionProperty(instance, propertyName, collectionTypeRef, collection, true, null);
}
else
{
object itemValue = CreateValue(propertyValue, elementType, options);
propertyInfo.SetValue(instance, itemValue);
}
}

return instance;
}

private static object CreateValue(object value, Type elementType, ODataCastOptions options)
{
Type valueType = value.GetType();

if (typeof(ISelectExpandWrapper).IsAssignableFrom(valueType))
{
SelectExpandWrapper subWrapper = value as SelectExpandWrapper;
Comment on lines +182 to +184
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks dangerous to me. We are checking for a match with the interface ISelectExpandWrapper, but then casting to the concrete type.

It could be possible that the value implements the interface but is not the concrete SelectExpandWrapper, which would cause this to fail.

return CreateInstance(subWrapper, elementType, options);
}
else
{
return value;
}
}

private static PropertyInfo GetPropertyInfo(Type type, string propertyName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this could be extracted to its own separate extension method. To me this is such a generic concern that doesn't need to be inside the OData-specific extension class.

{
PropertyInfo propertyInfo = null;
var properties = type.GetProperties().Where(p => p.Name == propertyName).ToArray();
if (properties.Length <= 0)
{
propertyInfo = null;
}
else if (properties.Length == 1)
{
propertyInfo = properties[0];
}
else
{
// resolve 'new' modifier
propertyInfo = properties.FirstOrDefault(p => p.DeclaringType == type);
Comment on lines +195 to +208
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated code.

if (propertyInfo == null)
{
throw new ODataException(Error.Format(SRResources.AmbiguousPropertyNameFound, propertyName));
}
}

return propertyInfo;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ internal abstract class SelectExpandWrapper : IEdmEntityObject, ISelectExpandWra
/// </summary>
public bool UseInstanceForProperties { get; set; }

/// <summary>
/// Gets the instance value.
/// </summary>
public abstract object InstanceValue { get; }

/// <inheritdoc />
public IEdmTypeReference GetEdmType()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public TElement Instance
set { UntypedInstance = value; }
}

/// <summary>
/// Gets the instance value.
/// </summary>
public override object InstanceValue => Instance;

protected override Type GetElementType()
{
return UntypedInstance == null ? typeof(TElement) : UntypedInstance.GetType();
Expand Down
Loading