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: main
Choose a base branch
from

Conversation

xuzhg
Copy link
Member

@xuzhg xuzhg commented Jan 26, 2023

Fixes #766.

If we have $select and $expand in the request query, the result is collection of ISelectExpandWrapper.

It's not easy for customers to do further. So Add 'Cast' Extension methods on IQueryable and Object.

Basic usage in the controller/action:

IQueryable query = options.ApplyTo(....);
IEnumerable<Customer> results = query.OCast<Customer>(query); // the new extension methods on IQueryable.

foreach (var a in results)
{
    // a is a partial object of Customer if we have $select and $expand.
}

Thanks.

@xuzhg
Copy link
Member Author

xuzhg commented Jan 26, 2023

@julealgon Please take a look and share your thoughts.

Comment on lines +112 to +130
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];
}
}
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).

Comment on lines +25 to +34
/// <summary>
/// The cast options.
/// </summary>
public class ODataCastOptions
{
/// <summary>
/// Gets/sets the map provider.
/// </summary>
public Func<IEdmModel, IEdmStructuredType, IPropertyMapper> MapProvider { get; set; }
}
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

/// <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>")]
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.

/// <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.

Comment on lines +156 to +157
IEnumerable collectionPropertyValue = propertyValue as IEnumerable;
foreach (var item in collectionPropertyValue)
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)

Comment on lines +182 to +184
if (typeof(ISelectExpandWrapper).IsAssignableFrom(valueType))
{
SelectExpandWrapper subWrapper = value as SelectExpandWrapper;
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.

Comment on lines +195 to +208
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);
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.

}
}

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.

Comment on lines +30 to +47
[Fact]
public void OCast_Works_ForSingleObject()
{
// Arrange & Act & Assert
object source = null;
Assert.Null(source.OCast<object>());

// Arrange & Act & Assert
QueryCustomer customer = new QueryCustomer
{
Name = "A"
};

Assert.Null(customer.OCast<string>());

var expectCustomer = customer.OCast<QueryCustomer>();
Assert.Same(customer, expectCustomer);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd strongly recommend breaking this into 3 separate tests either manually, or using data-driven tests.

@bogdan-patraucean
Copy link

We need this. Any updates??

@mikepizzo
Copy link
Member

We had a discussion on this today -- what is the expected behavior if there is a $select applied to the query on which OCast() is called? should we ignore the $select and set all of the properties, or should we only set the properties that are specified in $select and leave it to the developer to somehow understand which properties have been set and which have not?

@bogdan-patraucean
Copy link

bogdan-patraucean commented Oct 3, 2023

Awesome! I would expect the select to work. I can't use the select clause with IQueryable so I have to add the needed properties in my endpoint using the Select method.

From my understanding, this PR would fix the issue.

@julealgon
Copy link
Contributor

or should we only set the properties that are specified in $select

Wouldn't that clash with required properties? What if the user didn't select some properties that are marked as required in the original model? If you then try to recreate the same object but only set the selected properties, this will fail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature ask: support in memory filtering after ODataQueryOptions.ApplyTo()
4 participants