-
Notifications
You must be signed in to change notification settings - Fork 709
How to use AllowedFilterProperties and AllowedExpandProperties under ODataQueryOptionDescriptionContext #928
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
Comments
For an example of how to use the builder.Services
.AddApiVersioning()
.AddOData()
.AddODataApiExplorer(options => options.DescriptionProvider = new MyProvider()); However, I think you are asking how do you set or otherwise populate A key goal is to minimize the amount of configuration required and use what you've already defined to populate the query options. The cases where you can explicitly set conventions are only meant to cover the documentation gaps where you can't otherwise specify the information. As an example, if you used The way these values are specified is via Model Bound settings. They are configured as attributes on your model classes or set For example, you can apply: [Expand("NoExpand", ExpandType = SelectExpandType.Disabled)] To disable expand on an entire type, enable selective members, and set maximum depth. If you don't want to use attributes, you can use the fluent API a la: builder.EntitySet<Order>("Orders")
.EntityType.Expand(6)
.Expand(SelectExpandType.Disabled, "NoExpand"); No matter which approach you choose, the API Versioning extensions should detect it. If there's something I've missed or there's some other method you're using that I'm not aware of, please. share. |
The problem for us is our service want to just use a set of OData query ability rather than being an fully OData service, so we don't create EDM model and register when service is up. By looking at the source code, I think the easiest way is to extend DefaultODataQueryOptionDescriptionProvider and base on the ApiDescription under ODataQueryOptionDescriptionContext to set limit properties for each controller and action, otherwise need to rewrite entire ODataControllerQueryOptionsConventionBuilder and ODataActionQueryOptionsConventionBuilder(with T also). |
I see. You probably should have led with that. 😉 Things work a little different with some OData. There are still a few options:
The Admittedly, there is limited support for some OData. This is an area could use some additional enhancements. |
For the options you mentioned:
The other approaches which I can think: |
I haven't tried it E2E, but something like this should work: Provide some way to define allowed properties. Attributes are one way. [AttributeUsage( AttributeTargets.Method, AllowMultiple = false, Inherited = true )]
internal sealed class AllowedPropertiesAttribute : Attribute
{
public AllowedPropertiesAttribute( string property, params string[] otherProperties )
{
var properties = new string[otherProperties.Length + 1];
properties[0] = property;
if ( otherProperties.Length > 0 )
{
Array.Copy( otherProperties, 0, properties, 1, otherProperties.Length );
}
Properties = properties;
}
public IReadOnlyList Properties { get; }
} There's a few ways this might be used. Something like: [EnableQuery(AllowedQueryOptions = AllowQueryOptions.Expand)]
[AllowedProperties("customer", "lineItems")]
public IActionResult Get(int id) => Ok(); Now you can create a convention that looks like: internal sealed class AllowedPropertiesConvention : IODataQueryOptionsConvention
{
private readonly ODataQueryOptionSettings settings;
public AllowedPropertiesConvention(
IODataQueryOptionDescriptionProvider descriptionProvider,
IModelMetadataProvider modelMetadataProvider,
ODataOptions option )
{
settings = new()
{
NoDollarPrefix = true,
DescriptionProvider = descriptionProvider,
DefaultQuerySettings = options.QuerySettings,
ModelMetadataProvider = modelMetadataProvider,
}
}
public void ApplyTo( ApiDescription apiDescription )
{
// applying the convention as endpoint metadata is probably a better way to do this
// but it requires more initial setup
var method = ((ControllerActionDescriptor) apiDescription.ActionDescriptor).MethodInfo;
var enableQuery = method.GetCustomAttribute<EnableQueryAttribute>();
var allowed = method.GetCustomAttribute<AllowedPropertiesAttribute>();
if ( enableQuery == null || allowed == null )
{
return;
}
var queryOption = enableQuery.AllowedQueryOptions;
switch ( enableQuery.AllowedQueryOptions )
{
case AllowedQueryOptions.Expand:
case AllowedQueryOptions.Filter:
break;
default:
return;
}
var context = new ODataQueryOptionDescriptionContext( apiDescription );
for ( var i = 0; i < allowed.Properties.Count; i++ )
{
context.AllowedProperties.Add( allowed.Properites[i] );
}
var name = ( settings.NoDollarPrefix ? string.Empty : "$" ) +
option.ToString().ToLowerInvariant();
var type = typeof( string );
var description = settings.DescriptionProvider.Describe( queryOption, context );
var parameter = new ApiParameterDescription()
{
IsRequired = false,
ModelMetadata = new ODataQueryOptionModelMetadata(
settings.ModelMetadataProvider,
type,
description ),
Name = name,
ParameterDescriptor = new()
{
Name = name,
ParameterType = type,
},
Source = BindingSource.Query,
Type = type,
};
apiDescription.Parameters.Add( parameter );
}
} Finally, since DI is needed to get some of the services to build out the convention, we need public sealed class ODataConventionSetup : IConfigureOptions<ODataApiExplorerOptions>
{
private readonly IModelMetadataProvider modelMetadataProvider;
private readonly IOptions<ODataOptions> odataOptions;
public ODataConventionSetup(
IModelMetadataProvider modelMetadataProvider,
IOptions<ODataOptions> odataOptions )
{
this.modelMetadataProvider = modelMetadataProvider;
this.odataOptions = odataOptions;
}
public void Configure( ODataApiExplorerOptions options )
{
var convention = new AllowedPropertiesConvention(
options.DescriptionProvider,
modelMetadataProvider,
odataOptions );
options.QueryOptions.Add( convention );
}
} This might seem like a lot of ceremony, but the challenge is trying to add a convention where no convention exists. As I mentioned the existing functionality for the fluent conventions API is to have parity with Another possible option is to use the current, albeit limited, support for Hopefully that give you a few ideas. |
Yes, you could create a EDM just for the purposes of metadata. It would have no effect on the surface area of the API. You need to have the OData services and features enabled change the wire protocol. I haven't tried it, but it should be possible to use the EDM just to provide this documentation. |
Thanks very much @commonsensesoftware, I tried to extend DefaultODataQueryOptionDescriptionProvider with attribute approach, it works perfectly, I will try to use this approach also, but one question though, since we are adding convention, so both the default validation convention and new convention we added will be invoked via Apply function, are we adding same parameter info twice into api description? |
I'm interested in hearing about the challenges and extension points you need[ed] when you get to a working state when you're done. Supporting partial OData features has been an area I've been wanting to continue improving. There used to be virtually no support at all. Since there isn't an EDM or even necessarily attributes for metadata to discover things from, I suspect it will all have to be by custom conventions. I'd support enhancing what's possible today to make it easier for folks, like yourself. |
I just had conversation with Sam Xu, he suggested we can use SelectorModel to build an EDM and register into its EndpointMetadata as different metadata (Maybe called FlightODataModelMetadata), so when ODataQueryOptionDescriptionContext try to get model, it can get this metadata from the actionDescriptor's EndpointMetadata. But in order to achieve this, we need to build our own ODataQueryOptionDescriptionContext and write a convention by ourself like the sample you mentioned above, is my assumption correct or there is better way to customize this. |
That sounds like something Sam would suggest. That approach could be made to work. API Versioning doesn't specifically care about the
You add the metadata via a internal static void AddEndpointMetadata( this ActionModel action, object metadata )
{
var selectors = action.Selectors;
if ( selectors.Count == 0 )
{
selectors.Add( new() );
}
for ( var i = 0; i < selectors.Count; i++ )
{
selectors[i].EndpointMetadata.Add( metadata );
}
} Which is how API Versioning adds If you're able to connect the pieces this way and then apply the conventions via |
It's pretty yucky, but here's a working baseline that you should be able to build off of. I was able to get this to show a restricted This approach uses You have to do the following:
There may be some other oddities in the mix somewhere. The Metadata and Service Document endpoints have special handling. You have construct routing info that will not match that. It can be virtually anything (as seen below). Register it with: builder.Services.TryAddEnumerable(
ServiceDescriptor.Transient<IApiDescriptionProvider, AdHocModelBoundSettingsProvider>() ); The magic 🧙🏽 ... using Asp.Versioning;
using Asp.Versioning.ApiExplorer;
using Asp.Versioning.OData;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Mvc.ApiExplorer;
using Microsoft.AspNetCore.Mvc.ApplicationModels;
using Microsoft.AspNetCore.OData.Routing;
using Microsoft.AspNetCore.OData.Routing.Template;
using Microsoft.Extensions.Options;
using Microsoft.OData.Edm;
using Microsoft.OData.UriParser;
using static Asp.Versioning.ApiVersionMapping;
public class AdHocModelBoundSettingsProvider : IApiDescriptionProvider
{
private const int Last = int.MaxValue;
private readonly VersionedODataModelBuilder builder;
public AdHocModelBoundSettingsProvider(
IEnumerable<IApiVersionMetadataCollationProvider> providers,
IOptions<ApiVersioningOptions> options )
{
// a single, simple, inline configuration for all versions.
// a separate function or IModelConfiguration registrations
// are probably better here
builder = new(
CollateApiVersions( providers.ToArray(), options ),
Enumerable.Empty<IModelConfiguration>() )
{
DefaultModelConfiguration = ( modelBuilder, apiVersion, routePrefix ) =>
{
modelBuilder.EntitySet<Book>( "Books" )
.EntityType.Filter( "author" );
}
};
}
public int Order => Last;
public void OnProvidersExecuting( ApiDescriptionProviderContext context )
{
// 1. build edm for each api version (handled by builder)
// 2. for each action and edm:
// a. get action api version metadata
// b. get edm api version
// c. if edm api version applies to action, add it
var models = builder.GetEdmModels();
var actions = context.Actions;
for ( var i = 0; i < actions.Count; i++ )
{
var action = actions[i];
var metadata = action.GetApiVersionMetadata();
for ( var j = 0; j < models.Count; j++ )
{
var model = models[j];
var version = model.GetAnnotationValue<ApiVersionAnnotation>( model )?.ApiVersion;
if ( metadata.IsMappedTo( version ) )
{
action.EndpointMetadata.Add( new EdmMetadata( model ) );
}
}
}
}
public void OnProvidersExecuted( ApiDescriptionProviderContext context )
{
// clear/remove temporary metadata we added for
// exploration (e.g. no chance of affecting routing)
var actions = context.Actions;
for ( var i = 0; i < actions.Count; i++ )
{
var metadata = actions[i].EndpointMetadata;
for ( var j = metadata.Count - 1; j >= 0; j-- )
{
if ( metadata[j] is IODataRoutingMetadata )
{
metadata.Remove( j );
}
}
}
}
private static ODataApiVersionCollectionProvider CollateApiVersions(
IApiVersionMetadataCollationProvider[] providers,
IOptions<ApiVersioningOptions> options )
{
var context = new ApiVersionMetadataCollationContext();
for ( var i = 0; i < providers.Length; i++ )
{
providers[i].Execute( context );
}
var results = context.Results;
var versions = new SortedSet<ApiVersion>();
for ( var i = 0; i < results.Count; i++ )
{
var model = results[i].Map( Explicit | Implicit );
var declared = model.DeclaredApiVersions;
for ( var j = 0; j < declared.Count; j++ )
{
versions.Add( declared[j] );
}
}
if ( versions.Count == 0 )
{
versions.Add( options.Value.DefaultApiVersion );
}
return new() { ApiVersions = versions.ToArray() };
}
private static void ApplyEdmMetadata( IList<ControllerModel> controllers, IReadOnlyList<IEdmModel> models )
{
for ( var i = 0; i < controllers.Count; i++ )
{
var actions = controllers[i].Actions;
for ( var j = 0; j < actions.Count; j++ )
{
var action = actions[j];
var metadata = action.Selectors
.SelectMany( s => s.EndpointMetadata )
.OfType<ApiVersionMetadata>()
.FirstOrDefault();
if ( metadata == null )
{
continue;
}
for ( var k = 0; k < models.Count; k++ )
{
var model = models[k];
var version = model.GetAnnotationValue<ApiVersionAnnotation>( model )?.ApiVersion;
if ( !metadata.IsMappedTo( version ) )
{
continue;
}
var selectors = action.Selectors;
for ( var l = 0; l < selectors.Count; l++ )
{
selectors[l].EndpointMetadata.Add( new EdmMetadata( model ) );
}
}
}
}
}
private sealed class ODataApiVersionCollectionProvider : IODataApiVersionCollectionProvider
{
public IReadOnlyList<ApiVersion> ApiVersions { get; set; }
}
private sealed class EdmMetadata : IODataRoutingMetadata
{
// metadata (~/$metadata) and service (~/) doc have special handling.
// make sure we don't match the service doc
private readonly static ODataPathTemplate FakeODataTemplate =
new( new DynamicSegmentTemplate( new DynamicPathSegment( "fake" ) ) );
public EdmMetadata( IEdmModel model ) => Model = model;
public string Prefix => string.Empty;
public IEdmModel Model { get; }
public ODataPathTemplate Template => FakeODataTemplate;
public bool IsConventional => false;
}
} |
Having looked at it a couple of times, I think this is what a generic, comprehensive feature for partial OData uses would look like. I would have to detect if you registered Another edge case to be careful of would be a mixture of some full OData and some partial OData. I think it may be possible for these concepts to coexist. I already have some bits in there to detect if an action is OData-like, which is how/why exploration of OData query options for non-OData actions works at all. I need to cogitate on this some more, but this is likely the general approach. This example illustrates how to apply the configuration, but how would a developer configure the EDM? Normally, this would be done via |
@commonsensesoftware, just want to confirm I added AddOData. I am currently trying to approach Sam Xu provided, I will check your approach also. Hopefully this weekend I can give you a summary of what I tried and give your suggestions. |
Cool. To be clear, I made it work without |
@commonsensesoftware, one thing we figured out when looking at the source code for IApiVersioningBuilder.AddOData, underlying it will replace the origin IOptions with VersionedODataOptions. |
Not exactly, but kinda... There's no other way to get in front of OData and the design simply isn't extensible - unfortunately. Since you didn't use Line 147 in 0d1b9fa
By using I'm not sure off-hand, but there's a reasonable chance you'll have to go through Good update. I hope that helps steer you in the right direction. |
It was definitely involved to make the solution generic, but I was able to get something working over the weekend. I was able to polish things up last night. You can have a look and provide any feedback in PR #933. In short, you won't have to do anything special to make the Model Bound settings get picked up. Any use of the defined attributes will be picked up automatically. No additional registration will be required. It's magic 🧙🏽♂️. If you don't want use the Model Bound attributes, then you can use pure conventions a la: builder.Services
.AddApiVersioning()
.AddODataApiExplorer(
options.AdHocModelBuilder.DefaultModelConfiguration =
( b, v, p ) => b.EntitySet<Book>( "Books" ).EntityType.Filter( "title", "published" ) ); If you don't want your configuration to be inline, you can create It turns out that there really isn't a need to know if you're configuring a normal entity or an ad hoc one. More than one configuration for the same CLR type and API version combination doesn't make sense. Finally, ad hoc models will honor other things you configure in the EDM. Just like with normal OData, if the EDM doesn't exactly match the corresponding CLR type, then the model is substituted with a dynamically generated surrogate type. For example, you can exclude a specific property with: if ( apiVersion == new ApiVersion( 1.0 ) )
{
builder.EntitySet<Book>( "Books" ).EntityType.Ignore( book => book.NumberOfPages );
} Let me know if you have any other questions or feedback. I was even able to get this to work with OData in ASP.NET Web API too! 😄 . I expect this to land in |
@commonsensesoftware, this is awesome, update from my side, I was doing the approach you mentioned earlier and it worked perfectly. For the PR, I will take a look once I get some time and may get a branch and do some testing. |
I integrated all of this into the final release. |
@commonsensesoftware, with continuing doing my testing, find another thing, not sure whether it is bug or not. |
Correct. The following will not work without specifying the response type:
There might be a few I missed. The following will work:
provided that |
@commonsensesoftware , today when use EnableQueryAttributes, like [EnableQuery(AllowedQueryOptions = AllowedQueryOptions.Expand | AllowedQueryOptions.Filter | AllowedQueryOptions.Skip | AllowedQueryOptions.Top)], the ApiVersion library will read the enable query attribute and try to show the settings in the swagger. |
Yes... and no. 😛 Yes, you do have to use conventions. There simply is no other way to advertise this information. Without an attribute, it doesn't exist in the code and can't be interrogated. Since it also does not exist in the EDM, there is nothing to probe there either. The OData query options Conventions API is flexible. I provide a fluent API out-of-the-box, but you do not have to use it. You can also use the non-generic flavor of A full solution is a bit involved, but a custom convention would be something like this: public sealed class ODataQueryOptionsAttribute : Attribute
{
public ODataQueryOptionsAttribute(AllowedQueryOptions allowed) => Allowed = allowed;
public AllowedQueryOptions Allowed { get; }
} You can technically use using static Microsoft.AspNetCore.OData.Query.AllowedQueryOptions;
public sealed class MyCustomConvention : ODataValidationSettingsConvention
{
// you'd likely accept ODataOptions.QuerySettings, but you don't have to
public MyCustomConvention( ODataQueryOptionSettings? settings = default )
: base( DefaultValidationSettings(), settings ?? new() ) { }
private static ODataValidationSettings DefaultValidationSettings() => new();
public virtual void ApplyTo( ApiDescription apiDescription )
{
var controllerAttributes = controllerAction.ControllerTypeInfo.GetCustomAttributes<ODataQueryOptionsAttribute>( inherit: true );
var actionAttributes = controllerAction.MethodInfo.GetCustomAttributes<ODataQueryOptionsAttribute>( inherit: true );
var attributes = controllerAttributes.Concat( actionAttributes ).ToArray();
if ( attributes.Length == 0 )
{
return;
}
var context = new ODataQueryOptionDescriptionContext( apiDescription, ValidationSettings );
// TODO: merge options from the attributes
var options = ?;
var parameterDescriptions = apiDescription.ParameterDescriptions;
// example usage:
if ( options.HasFlag( Top ) )
{
parameterDescriptions.Add( NewTopParameter( context ) );
}
if ( options.HasFlag( Skip ) )
{
parameterDescriptions.Add( NewSkipParameter( context ) );
}
// ...
}
} ODataValidationSettingsConvention.cs can provide additional details on how you would implement it. Then you just add it to the configuration: services.AddApiVersioning().AddODataApiExplorer( options => options.QueryOptions.Add( new MyCustomConvention() ) ); |
Nice design pattern, I was looking at how EnableQueryAttribute got consumed via ODataAttributeVisitor, Will take your suggestion and give it a try since EnableQueryAttribute is too heavy and did so many things :) |
@commonsensesoftware , I tried the approach above, the new ODataQueryOptionDescriptionContext( apiDescription, ValidationSettings ) is internal constructor, don't think I can use it. Also for how to initialize the convention, I am thinking of using ConfigureOptions approach Also the following code was invoked twice, but by looking at the code we register as singleton. |
As I mentioned, you will likely need a What you are observing appears to be a behavior of Swashbuckle and not something I can control. You can see at: that a container scope and |
@commonsensesoftware , I landup use the first option and took IModelMetadataProvider and IOptions as DI dependency. It looks pretty good. However once I added my convention, each odata query parameters api supported showed up as twice, by looking at the code, I think the reason is I am using the model configuration for showing the limited properties our api can expand and filter, ODataValidationSettingsConvention will visit the model configuration bounding and try to set parameter description, and then my custom attribute was added again via my convention. In this case, I think I should use merge rather than add. |
That sounds about right. I'm leading you down the path, but how you want to implement the solution is at your discretion. The point of the Honestly, the default implementation could have broken out the entry point to adding all the parameters because what you need should be exactly the same as the default implementation. This is an enhancement I may consider in the next release. Not a lot of people have gone down this rabbit hole of customization so I don't have a ton of feedback in this area. Regardless, it might be a little bit more verbose, but you should have all the necessary pieces to replicate the process of adding all the parameters. If there's certain things you don't want to support or you know you can optimize, you have full control over that. |
@commonsensesoftware , it feels likr a bug, but want to discuss about it. I use model configuration to set max top and default page size But for one api, we don't need pagination, so I use EnableQueryAttribute and didn't set skip and top In your code it directly set skip and top if we set top and page via model configuration. aspnet-api-versioning/src/Common/src/Common.OData.ApiExplorer/Conventions/ODataAttributeVisitor.cs Line 231 in 3857a33
For the OData Order, based on the document, it doesn't explicitly mention that model configuration should override EnableQueryAttributes, and I get the confirmation from Sam Xu. Also did test if I pass $skip and $top in the request, I get bad request due to I didn't set them in EnableQueryAttributes. Which makes feel that override rule is based on whether Skip and Top operation is set in EnableQueryAttribute or not. |
If that is Sam's text, it's not all that clarifying; however, now that we've had a lengthy chat and I've spelunked a lot of the OData source code, I have a better understanding of how the code works. I think, the visitation logic is correct, but it may be backward. We know that Model Bound Settings are validated first and that Let me reverse the logic and make sure it doesn't break anything. You've presented a very valid use case that I can add to the test suite as well. If all goes well, I'll accept/create a bug for it and publish a patch. I'll get back to you ASAP. |
Thanks @commonsensesoftware, the statement below actually is coming from https://learn.microsoft.com/en-us/odata/webapi/modelbound-attribute#overall-query-setting-precedence. |
…Bound Settings. Related to #928
@SamGuoMsft the change seems simple. I've created PR #973, which you can take a 👀 at. All I did was reorder visitation so that Model Bound settings are processed first. If there is a subsequent Let me know if you see any other scenario or configuration that I might have missed. I still need to cherry pick this commit and backport it to the .NET 6 package for you. While it's not absolutely necessary, it would be nice to more 👀 on the changes. I should be able to have the patch out for you by Monday if not over the weekend. |
…Bound Settings. Related to #928
Thanks @commonsensesoftware , I am going to make the same changes in my local branch and test my scenario, will post here. |
@commonsensesoftware , the changes look good. Ship it :) |
Fantastic! I'll work on getting the PRs merged and the packages published so you can pick it up by the AM. Stay tuned. 😄 |
@commonsense7software , thanks very much, would you mind to publish this to both asp.net 6 and 7. We plan to move to asp.net 7 in middle of next month. |
…Bound Settings. Related to #928
…Bound Settings. Related to #928
|
Checking back in. @SamGuoMsft did the patch finally solve your issue? |
@commonsensesoftware, we updated the packages to this latest version and got an exception when requesting the openapi.json. I didn't change anything else... Happens with both
Here is my reproduction: https://github.com/dnperfors/eLibrary/blob/main/src/eLibrary.WebApi.OData.Versioning/Program.cs -- edit -- |
@dnperfors The issue you are describing is related to #980. You can track what's happening there as well as the deep link over to the OData repo. |
@commonsensesoftware, sorry for later reply. The issue is solved, thanks very much for your help on this. |
Thanks for confirming. I wanted to wait on closing it out until you confirmed it was resolved. Thanks. You helped make things better. |
Is there an existing issue for this?
Describe the bug
Not sure it is a bug or not. But I have scenario that we only have limit properties can be used as expand and filter and would like to show as description in Swagger. I saw ODataQueryOptionDescriptionContext exposes AllowedFilterProperties and AllowedExpandProperties, since we only can set max depth and max node.
options.QueryOptions.Controller()
.Action( c => c.Get( default ) )
.Allow( Filter | Expand )
.AllowExpand( 2 )
.AllowFilter( 4 );
Would you mind to give any suggestions how I can use these two properties?
Expected Behavior
Having a way to use AllowedFilterProperties and AllowedExpandProperties.
Steps To Reproduce
No response
Exceptions (if any)
No response
.NET Version
7.0.100
Anything else?
No response
The text was updated successfully, but these errors were encountered: