Skip to content

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

Closed
1 task done
SamGuoMsft opened this issue Nov 29, 2022 · 72 comments

Comments

@SamGuoMsft
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

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

@commonsensesoftware
Copy link
Collaborator

For an example of how to use the ODataQueryOptionDescriptionContext, you can have a look at: DefaultODataQueryOptionDescriptionProvider. The default provider can be extended or replaced and configured via:

builder.Services
       .AddApiVersioning()
       .AddOData()
       .AddODataApiExplorer(options => options.DescriptionProvider = new MyProvider());

However, I think you are asking how do you set or otherwise populate AllowedFilterProperties and AllowedExpandProperties. This is a good question and it clearly is not all that obvious (but it's supposed to be).

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 ODataQuerySettings and ODataValidationSettings directly in a controller action, there is no other way to surface this information. These classes do not have a way to limit $filter or $expand property names, which is why you can't find a built-in extension method convention for them.

The way these values are specified is via Model Bound settings. They are configured as attributes on your model classes or set
using the fluent conventions when you configure the EDM. This is required for OData to enforce the policies. The API Versioning extensions are of aware of this and will pick them up and document them for you. It might seem clunky, but it would be weird (and bad) if a limited set of properties were specified in the documentation, but they weren't actually limited by OData. Most settings can be configured multiple ways, but this happens to be some that can only be done this way (unless I've made a mistake or things have changed).

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.

@SamGuoMsft
Copy link
Author

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

@commonsensesoftware
Copy link
Collaborator

I see. You probably should have led with that. 😉 Things work a little different with some OData.

There are still a few options:

  1. Create and add a custom IODataQueryOptionsConvention, which can be added to the default builder
  2. Extend/customize the default ODataQueryOptionsConventionBuilder
  3. Extend/customize the DefaultODataQueryOptionDescriptionProvider

The ODataQueryOptionDescriptionContext is ultimately just a monad to collect all the discovered information from standard OData attributes and settings. There isn't anything that special about it. If you have your own convention or you know what you're looking for (in your own code), then it's pretty straight forward to add the necessary ApiParameterDescription instances. For example, you could create your own attribute that you know to look for (ex: [AllowedProperties("...")]).

Admittedly, there is limited support for some OData. This is an area could use some additional enhancements.

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Nov 29, 2022

For the options you mentioned:

  1. I though about to build my own ODataValidationSettingsConvention which is inherited from IODataQueryOptionsConvention, but if I do that, I need to write my own ODataActionQueryOptionsConventionBuilder and override the Build method from base class ODataActionQueryOptionsConventionBuilderBase. Would you mind to let me know how to add to the default builder?
    2 and 3 we already discussed above, and for the AllowedProperties attribute, yeap, as I mentioned by using ApiDescription, I will add attribute and use attribute to control it.

The other approaches which I can think:
4. by using Swagger OperationFilter with parameter and custom attribute to edit description.
5. I looked at edm model again, based on the code , if I dynamically generate an EDM, can I set into ActionDescriptor EndpointMetadata and still keep service as standard restful? Since otherwise I can't use model API version in that case.

@commonsensesoftware
Copy link
Collaborator

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 IConfigureOptions<T>:

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 ODataQuerySettings and ODataValidationSettings. Neither of these have a way to specify the properties allowed with $expand or $filter. I'm not opposed to supporting it as an enhancement, but it's not there today.

Another possible option is to use the current, albeit limited, support for $expand and $filter conventions and update the description after the parameter is added with the additional properties. Of course, you could go down the Swashbuckle or OpenAPI extensions, but you'll have many of the same challenges.

Hopefully that give you a few ideas.

@commonsensesoftware
Copy link
Collaborator

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.

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Nov 29, 2022

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 IConfigureOptions<T>:

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 ODataQuerySettings and ODataValidationSettings. Neither of these have a way to specify the properties allowed with $expand or $filter. I'm not opposed to supporting it as an enhancement, but it's not there today.

Another possible option is to use the current, albeit limited, support for $expand and $filter conventions and update the description after the parameter is added with the additional properties. Of course, you could go down the Swashbuckle or OpenAPI extensions, but you'll have many of the same challenges.

Hopefully that give you a few ideas.

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?

@commonsensesoftware
Copy link
Collaborator

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.

@SamGuoMsft
Copy link
Author

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.

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.

@commonsensesoftware
Copy link
Collaborator

commonsensesoftware commented Nov 30, 2022

That sounds like something Sam would suggest.

That approach could be made to work. API Versioning doesn't specifically care about the SelectorModel. At the end of the day, you just need an EDM associated with the Endpoint (a la the metadata):

  1. API Versioning is looking for IODataRoutingMetadata in the endpoint metadata.
    a. This could be any implementation; it doesn't need to be an OData one
    b. Only the Model property is used/considered
  2. API Versioning uses and expects one EDM per API version
    a. That can be a little tedious, the VersionedODataModelBuilder may help there
    b. The constructed IEdmModel must have the ApiVersionAnnotation applied
    i. That's how API Versioning will match up the current API version with the correct EDM
    ii. Each EDM an only have this annotation applied once; hence, you need one EDM per API version
    iii. If you have endpoint that services more than one API version, then you need multiple IODataRoutingMetadata instances
  3. The metadata can be applied via:
    a. IApplicationModelProvider
    b. IControllerModelProvider
    c. IActionModelProvider

You add the metadata via a SelectModel, but you only need one. You might have extension that looks something like this:

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

If you're able to connect the pieces this way and then apply the conventions via ODataModelBuilder or Model Bound attributes (which get picked up in GetEdmModel()), then I think this approach should work.

@commonsensesoftware
Copy link
Collaborator

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 $filter property using the Some OData OpenAPI Example. This will require API Versioning 6.3.1+ or 7.0.0-rc1.+.

This approach uses IApiDescriptionProvider, which should only affect the API Explorer. The necessary metadata to document the OData query options are temporarily added prior to exploration and then subsequently removed. This should guarantee zero chance of affecting the routing process and also prevents frivolous allocated memory from hanging around for no reason.

You have to do the following:

  1. Collate all API versions (this is probably also possible through the IApiVersionDescriptionProvider service)
  2. Create a VersionedODataModelBuilder that will be based on each API version
    a. This sample uses a simple, default callback, but you probably want separate methods or use IModelConfiguration
  3. Before exploration, for every action where the API version of the EDM applies, add the EDM as metadata to the endpoint
  4. After exploration, remove the EDM metadata added; it's not needed for anything else

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;
    }
}

@commonsensesoftware
Copy link
Collaborator

commonsensesoftware commented Nov 30, 2022

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 AddODataApiExplorer without calling AddOData. IODataApiVersionCollectionProvider and VersionedODataModelBuilder aren't registed in DI otherwise. Furthermore, their registrations (as seen in the example) would be quite different from full OData.

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 VersionedODataModelBuilder or IModelConfiguration that is picked up or exposed by DI. If both OData and non-OData actions coexist, how do you tell them apart? It might be a non-issue, but I have to think that through. If I can solve that, then you can configure your OData query options in the EDM just like other Model Bound settings. The fake, temporary EDM can be handled for you behind the scenes. 😉

@SamGuoMsft
Copy link
Author

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 AddODataApiExplorer without calling AddOData. IODataApiVersionCollectionProvider and VersionedODataModelBuilder aren't registed in DI otherwise. Furthermore, their registrations (as seen in the example) would be quite different from full OData.

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 VersionedODataModelBuilder or IModelConfiguration that is picked up or exposed by DI. If both OData and non-OData actions coexist, how do you tell them apart? It might be a non-issue, but I have to think that through. If I can solve that, then you can configure your OData query options in the EDM just like other Model Bound settings. The fake, temporary EDM can be handled for you behind the scenes. 😉

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

@commonsensesoftware
Copy link
Collaborator

commonsensesoftware commented Nov 30, 2022

Cool. To be clear, I made it work without IApiVersioningBuilder.AddOData. Since my example doesn't resolve any of the other OData-specific services from DI, it should still work if you called it. I look forward to hearing/seeing your findings.

@SamGuoMsft
Copy link
Author

Cool. To be clear, I made it work without IApiVersioningBuilder.AddOData. Since my example doesn't resolve any of the other OData-specific services from DI, it should still work if you called it. I look forward to hearing/seeing your findings.

@commonsensesoftware, one thing we figured out when looking at the source code for IApiVersioningBuilder.AddOData, underlying it will replace the origin IOptions with VersionedODataOptions.
services.Replace( Singleton<IOptions>( sp => sp.GetRequiredService() ) );
So the global setting .AddControllers().AddOData(opt =>opt.Filter().Expand()) will be gone. So if we add IApiVersioningBuilder.AddOData, it will not work and I guess the reason why the global setting is gone is due to we don't register EDM.

@commonsensesoftware
Copy link
Collaborator

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 AddOData and you don't have any true OData endpoints, I'm guessing the process is skipped. What you are looking for is here:

By using IOptionsFactory<ODataOptions> you can create all the ODataOptions you need. The key will always be Options.DefaultName because OData options aren't named (but I guess you could register it that way). Creating an instance of ODataOptions this way will take you through the global callback in the original AddOData configuration. It's a bit strange, but I did it that way so you didn't end up having to configure global options differently per API version. Arguably, that wouldn't be global for the application anymore. Furthermore, they are unlikely to be different across API versions.

I'm not sure off-hand, but there's a reasonable chance you'll have to go through ODataOptions.AddRouteComponents with fake values that you'll never use. Add a minimal, you'd have to invoke the configuration callback. I'll have to look at the OData source again to see what it does.

Good update. I hope that helps steer you in the right direction.

@commonsensesoftware
Copy link
Collaborator

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 IModelConfiguration implementations and use them just like you would for normal OData configurations. You likely wouldn't need to, but if you need information prior to the configuration being built, you can also implement IODataQueryOptionsConvention. It acts as a visitor.

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 7.0, which is just around the corner.

@SamGuoMsft
Copy link
Author

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 IModelConfiguration implementations and use them just like you would for normal OData configurations. You likely wouldn't need to, but if you need information prior to the configuration being built, you can also implement IODataQueryOptionsConvention. It acts as a visitor.

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 7.0, which is just around the corner.

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

@commonsensesoftware
Copy link
Collaborator

I integrated all of this into the final release. 7.0.0 has been published and has this feature in it; credit to you. Try it out and if you run into any issues or limitations, we'll get them patched. 😄

@SamGuoMsft
Copy link
Author

@commonsensesoftware, with continuing doing my testing, find another thing, not sure whether it is bug or not.
If I change the action return from IQueryable to IActionResult, the swagger will only show the expand section. The filter, pagination sections are all gone. But once I put [ProducesResponseType(typeof(IQueryable), StatusCodes.Status200OK)],
then everything becomes normal. This behavior is making sense since library needs to know the return type so that can read filter and pagination info. But just want to confirm with you that it is expected and not a bug.

@commonsensesoftware
Copy link
Collaborator

Correct. The following will not work without specifying the response type:

  • void
  • Task
  • IActionResult
  • Task<IActionResult>
  • IEnumerable
  • IQueryable

There might be a few I missed. The following will work:

  • Task<T>
  • IEnumerable<T>
  • IQueryable<T>
  • IAsyncEnumerable<T>
  • ActionResult<T>
  • SingleResult<T>

provided that T is not one of the listed response types that will not work.

@SamGuoMsft
Copy link
Author

@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.
If I don't plan to use EnableQuery since it did so many things, the only other option is set it via AddODataApiExplorer, is my understand correct? If there are other options to do it, please let me know.
image

@commonsensesoftware
Copy link
Collaborator

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 options.QueryOptions.Controller(typeof(BooksController)), which might be easier in some cases. If you don't want to put all the logic directly in the start, you can always move it to your own IConfigureOptions<ODataApiExplorerOptions> implementation with DI. You are free to create your own conventions as well. Your convention might map by type or you could combine a convention with your own custom attribute to indicate the available options.

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 IODataQueryOptionsConvention, but it's probably simpler to start with ODataValidationSettingsConvention like this:

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() ) );

@SamGuoMsft
Copy link
Author

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 :)

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Jan 31, 2023

@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.
image

  1. First time being invoked here
    image
  2. When swagger UI showed up.

@commonsensesoftware
Copy link
Collaborator

ODataQueryOptionDescriptionContext has a public constructor. The use of ODataValidationSettings is a specific variant. I wasn't sure I wanted to open that up. You have 2 options:

  1. Use the normal public constructor and initialize the other properties outside of the constructor from ODataValidationSettings.
    a. The constructor logic is just a shortcut not a barrier
  2. The constructor is protected internal and the class is not sealed. You can extend ODataQueryOptionDescriptionContext to get access to the constructor if you really want it.

As I mentioned, you will likely need a IConfigureOptions<ODataApiExplorerOptions> implementation to wire your conventions. IOptions<T> is a singleton so your IConfigureOptions<T> should be transient as it should only ever be used once.

What you are observing appears to be a behavior of Swashbuckle and not something I can control. You can see at:

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/master/src/Swashbuckle.AspNetCore.SwaggerUI/SwaggerUIBuilderExtensions.cs#L33

that a container scope and IOptionsSnapshot<T> is used, which will almost certainly result in another instance being created. It's out of my hands, but I don't think it should be a problem either. Are you actually running into a problem or you just made an observation?

@SamGuoMsft
Copy link
Author

ODataQueryOptionDescriptionContext has a public constructor. The use of ODataValidationSettings is a specific variant. I wasn't sure I wanted to open that up. You have 2 options:

  1. Use the normal public constructor and initialize the other properties outside of the constructor from ODataValidationSettings.
    a. The constructor logic is just a shortcut not a barrier
  2. The constructor is protected internal and the class is not sealed. You can extend ODataQueryOptionDescriptionContext to get access to the constructor if you really want it.

As I mentioned, you will likely need a IConfigureOptions<ODataApiExplorerOptions> implementation to wire your conventions. IOptions<T> is a singleton so your IConfigureOptions<T> should be transient as it should only ever be used once.

What you are observing appears to be a behavior of Swashbuckle and not something I can control. You can see at:

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/master/src/Swashbuckle.AspNetCore.SwaggerUI/SwaggerUIBuilderExtensions.cs#L33

that a container scope and IOptionsSnapshot<T> is used, which will almost certainly result in another instance being created. It's out of my hands, but I don't think it should be a problem either. Are you actually running into a problem or you just made an observation?

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

@commonsensesoftware
Copy link
Collaborator

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 context is build up a set of initial configuration from global query options and some standard set of validation settings, if any. Then you will need to traverse (e.g. visit) your custom attributes to accumulate the settings which is logically a merge in most cases. Then, and only then, you can use the final set of options to build out the necessary parameters.

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.

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Mar 9, 2023

@commonsensesoftware , it feels likr a bug, but want to discuss about it. I use model configuration to set max top and default page size
image

But for one api, we don't need pagination, so I use EnableQueryAttribute and didn't set skip and top
image

In your code it directly set skip and top if we set top and page via model configuration.

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

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.

@commonsensesoftware
Copy link
Collaborator

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 EnableQueryAttribute is processed last because it's an IActionFilter. This means the model should be visited first and the attributes second. The explicit logic to $skip and $top as AllowedQueryOptions is correct when you use [Page] because there is no other way to configure this information when you use Model Bound Settings alone. However, if that was always implicitly set and then overridden by EnableQueryAttribute, you would get the correct behavior. The challenge in this feature has always been clearly knowing the correct order of evaluation. The fact that the two configuration approaches are incongruent doesn't help.

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.

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Mar 9, 2023

Thanks @commonsensesoftware, the statement below actually is coming from https://learn.microsoft.com/en-us/odata/webapi/modelbound-attribute#overall-query-setting-precedence.
image
Also I did tests on $expand and $filter too, it has the same pattern as $skip and $top: if I didn't set them in EnableQueryAttributes, but set Expand and Filter properties via ModelConfiguration, now I pass $expand or $filter in the request, I get bad request.

@commonsensesoftware
Copy link
Collaborator

@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 EnableQueryAttribute found, it will override those settings when applicable. All of the previous code continued working as expected. I added a new test that should represent your test case and it now behaves as you expect.

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.

@SamGuoMsft
Copy link
Author

@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 EnableQueryAttribute found, it will override those settings when applicable. All of the previous code continued working as expected. I added a new test that should represent your test case and it now behaves as you expect.

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.

Thanks @commonsensesoftware , I am going to make the same changes in my local branch and test my scenario, will post here.

@SamGuoMsft
Copy link
Author

SamGuoMsft commented Mar 15, 2023

@commonsensesoftware , the changes look good. Ship it :)

@commonsensesoftware
Copy link
Collaborator

Fantastic! I'll work on getting the PRs merged and the packages published so you can pick it up by the AM. Stay tuned. 😄

@SamGuoMsft
Copy link
Author

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

commonsensesoftware added a commit that referenced this issue Mar 16, 2023
commonsensesoftware added a commit that referenced this issue Mar 16, 2023
@commonsensesoftware
Copy link
Collaborator

6.4.1 and 7.0.2 have been published with the changes. Pick them up at your leisure.

@commonsensesoftware
Copy link
Collaborator

Checking back in. @SamGuoMsft did the patch finally solve your issue?

@dnperfors
Copy link

dnperfors commented Apr 5, 2023

@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 6.4.1 and 7.0.2:

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1]
      An unhandled exception has occurred while executing the request.
      System.MissingMethodException: Method not found: 'Microsoft.OData.ModelBuilder.Config.DefaultQuerySettings Microsoft.AspNetCore.OData.ODataOptions.get_QuerySettings()'.
         at Asp.Versioning.ApiExplorer.ODataApiDescriptionProvider.ExploreQueryOptions(IEnumerable`1 apiDescriptions)
         at Asp.Versioning.ApiExplorer.ODataApiDescriptionProvider.OnProvidersExecuted(ApiDescriptionProviderContext context)
         at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.GetCollection(ActionDescriptorCollection actionDescriptors)
         at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.get_ApiDescriptionGroups()
         at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerDocumentWithoutFilters(String documentName, String host, String basePath)
         at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerAsync(String documentName, String host, String basePath)
         at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
         at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
         at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

Here is my reproduction: https://github.com/dnperfors/eLibrary/blob/main/src/eLibrary.WebApi.OData.Versioning/Program.cs

-- edit --
Found out this isn't an issue with aspnet-api-versioning, but with OData itself, in the 8.0.12 release of OData this doesn't occur. You can ignore this comment on this issue :)

@commonsensesoftware
Copy link
Collaborator

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

@SamGuoMsft
Copy link
Author

Checking back in. @SamGuoMsft did the patch finally solve your issue?

@commonsensesoftware, sorry for later reply. The issue is solved, thanks very much for your help on this.

@commonsensesoftware
Copy link
Collaborator

Thanks for confirming. I wanted to wait on closing it out until you confirmed it was resolved. Thanks. You helped make things better.

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

No branches or pull requests

3 participants