-
Notifications
You must be signed in to change notification settings - Fork 161
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
Added support for custom query option implementation with a simple extension handler #617
base: release-8.x
Are you sure you want to change the base?
Conversation
…en the service collection is null
/// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param> | ||
/// <param name="extensions">The <see cref="IODataQueryOptionsBindingExtension"/> to add to the service collection.</param> | ||
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns> | ||
public static IServiceCollection AddMultipleDataQueryOptionsBindingExtension(this IServiceCollection services, Action<IList<IODataQueryOptionsBindingExtension>> extensions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand why extensions
is a delegate instead of just the extensions themselves. What's the reasoning behind this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I like it more this way because I think it looks better in the configuration :). But it can also be changed if you don't like this approach.
.AddRouteComponents("v1", model1, (services) =>
{
services.AddMultipleODataQueryOptionsBindingExtension((extensions) =>
{
extensions.Add(new ExampleQueryOptionsBindingExtension());
extensions.Add(new Example1QueryOptionsBindingExtension());
extensions.Add(new Example2QueryOptionsBindingExtension());
extensions.Add(new Example3QueryOptionsBindingExtension());
});
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure yet if I'm nitpicking or not :), but I think then wouldn't the delegate be Func<IEnumerable<IODataQueryOptionsBindingExtension>, IEnumerable<IODataQueryOptionsBindingExtension>> extensions
so that you could write:
.AddRouteComponents("v1", model1, (services) =>
{
services.AddMultipleODataQueryOptionsBindingExtension((extensions) =>
{
extensions
.Append(new ExampleQueryOptionsBindingExtension())
.Append(new Example1QueryOptionsBindingExtension())
.Append(new Example2QueryOptionsBindingExtension())
.Append(new Example3QueryOptionsBindingExtension());
});
})
In fact, though, before I waste too much of your time, @xuzhg are you alright with the structure of this change? If so, I'll go ahead and start reviewing more, but I want to make sure we are heading in the right direction first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@corranrogue9 Your solution is also fine, both should work :)
|
||
using System.Linq; | ||
|
||
namespace Microsoft.AspNetCore.OData.Query.Extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add some tests for this new feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some tests.
…named AddDataQueryOptionsBindingExtension to AddODataQueryOptionsBindingExtension
I think this is a great feature being introduced. Given that this introduces a new API, I think I'd like to see more discussions/thoughts on design considerations. @xuzhg @corranrogue9 @gathogojr any thoughts on this. @DevKrieger is this PR linked to an existing issue? If so, could you link the issue? If not, could you create an issue that describes what the problem being solved, that feature being requested? Before reading the code, I'd like to understand what the expected behavior, what the specification for this feature is. e.g. at what point of the query transformation pipeline is this customer handler invoked, and where does its result get applied? Is the extension expected to be able to modify the query settings and query options objects? If so, what's the impact, if not, should do we do anything to prevent that from happening? |
|
||
namespace ODataRoutingSample | ||
{ | ||
public class ExampleQueryOptionsBindingExtension : IODataQueryOptionsBindingExtension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide some documentation for Public classes, functions and methods.
{ | ||
public class ExampleQueryOptionsBindingExtension : IODataQueryOptionsBindingExtension | ||
{ | ||
public IQueryable ApplyTo(IQueryable query, ODataQueryOptions queryOptions, ODataQuerySettings querySettings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these params used for ?
{ | ||
result = extension.ApplyTo(query, queryOptions, querySettings); | ||
} | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intention here to return the last result and ignore the rest ?
{ | ||
if(Context.RequestContainer != null) | ||
{ | ||
IODataQueryOptionsBindingExtension extension = Context.RequestContainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to use var for consistency.
public void ODataQueryOptions_ApplyCustom_RequestContainerIsNull() | ||
{ | ||
// Arrange | ||
HttpRequest request = RequestFactory.Create(HttpMethods.Get, "http://server/service/Customers"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to use var for consistency.
This adds a simple
IODataQueryOptionsBindingExtension
interface that allows third-party libraries to extend theApplyTo
method in theODataQueryOptions
class.Example:
Add a custom handler and extend the queryable.
Add the extension handler to a service collection of a route component.