OData+Swagger - $filter properties hidden if all properties are allowed

[spaasis]

Hi, and thanks (again) for your great work!

I ran into something that is working as intended but is an ill fit for my use case.

By default the api-versioning does not display e.q. OData $filter property names if every property is can be used as a filter: https://github.com/microsoft/aspnet-api-versioning/blob/e16b579d240574053ebd8e7ee38c8686beaee174/src/Common.OData.ApiExplorer/AspNet.OData/Builder/ODataAttributeVisitor.cs#L303

I hit a snag with a class with only two properties, both of which can be used as $filter. It took me a while to find out why this particular endpoint didn't list the properties while all others did.

I like to explicitly show all the available properties (for easier copy-pasting for example), even though the lists can admittedly grow quite long:

Now, I think the default behavior is absolutely sensible. What do you think about adding a configuration to toggle this behavior or only perform the cleaning if there are e.g. >5 properties listed?

[commonsensesoftware]

If I'm understanding correctly, things are working, but not displaying the descriptions the way you'd like.

As I recall, making description shorter (and succinct as possible) was a goal, but it had to be weighed against clarity and meaning. By listing no properties, it was intended that all properties are allowed, but I suppose that the description could say as much. At least in the case of OpenAPI/Swagger, the corresponding data model is also listed so my thought was that would be the thing that indicates all the possible properties.

I haven't seen many people customizing this area, so there are likely a number of places of improvement. You should be able to customize things today through IODataQueryOptionsConvention and IODataQueryOptionDescriptionProvider; however, there are likely some opportunities to improve that experience. I'm all for improving the functionality, but I'm definitely apprehensive about adding a bunch of configuration values. Adding more knobs may be confusing and tedious There should be a sensible set of defaults, possibly with some adjustments, and then developers can go off and customize/extend things to their heart's content.

[spaasis]

Yeah, you got the gist of the issue.
I totally agree that maybe a configuration flag for this would not be the best approach. I haven't run into those two interfaces before - I'll see how far they get me.

But yeah, maybe instead of an empty string to indicate that all properties can be used, the list could use the same way it now says that for $filter: The allowed functions are: allfunctions (should that actually be all functions...) ?

And while we're on the topic - as far as I can tell there's zero indication to the user what the actual $filter functions are - we basically need to point to some other documentation for that. I know that we shouldn't dump the entire OData syntax to Swagger, at least not in every endpoint that allows $filter, but it's something to think about.

If there's some direction you would like to move here, I would be happy to try my hand in making a PR

[commonsensesoftware]

The starting point is effectively ODataQueryOptionsConventionBuilder. You may have to make an undesirable number of customizations, but they should all be simple. You then update the default ODataApiExplorerOptions.QueryOptions with your new implementation. All the pieces you need should be public. If you run into any roadblocks, do share and we'll come up with a way to enhance things.

In the incarnations before the days of Swagger/OpenAPI, I had some fairly robust descriptions with the details as bulleted lists. All that's technically still possible to to do, it doesn't really work out for most documentation. If I remember right, any CRLF is removed from the text. In addition, the text can get really, really long. I feel like remember that there is a point where the text is eventually truncated or otherwise clipped from view; that's also bad. Ideally, it'd be nice to include some additional metadata such as a link to the documentation - as you suggested. To my knowledge, there is no way to make an OpenAPI document encode or otherwise utilize that. It might be possible via extension points, but it's likely a lot more work than people are willing to do. You might even be able to do that today. It doesn't make sense for API Versioning to hardcode such links and if it provides a way for you to define it, but it's not utilized without you doing more work on your end, then it's not super useful.

Is there really a spot where it says allfunctions? That doesn't seem right. I was looking some of the code and resource strings, but I couldn't figure out where that's coming from. The path where AllowedFunctions.All comes into play should be skipped. If you interested in helping fix some text or rendering thereof, I'll support a PR for that. The only caveat is that all PRs must provide the same changes for ASP.NET Core and ASP.NET Web API flavors of OData. In terms of fundamental design changes, I'm open to that, but I want to queue that up in a holding pattern - for now. I'm trying my best to burn down the long list of current issues and it's getting close. After the next release (patch or whatever), then I'll be ready to consider or changing features. It's best to queue that up in a new issue for tracking purposes.

[spaasis]

Thank you very much for the detailed response!

We have added links to odata.org for our API descriptions, and that at least is very simple to maintain. It's a shame that my trusted cheat sheet was recently taken down...

I'll see about fixing the allFunctions - at least it repros on every one of our APIs.

Let's keep this issue open a bit longer while I test the builders and query options for the problem described in the opening post.

[spaasis]

Hmm, undesirable amount of customizations seems like the correct estimation 😅
Here's my thought process:

    public class ODataQueryOptionsConventionBuilderOverrides : ODataQueryOptionsConventionBuilder {
        public ODataQueryOptionsConventionBuilderOverrides() {
            //replace the existing Convention in Conventions list by our overridden instance
            //could maybe override ApplyTo to directly inject our instance but seems risky
        }
    }

    public class ODataValidationSettingsConventionOverrides : ODataValidationSettingsConvention {

        public ODataValidationSettingsConventionOverrides(ODataValidationSettings validationSettings, ODataQueryOptionSettings settings) :
            base(validationSettings, settings) {
        }

        public override void ApplyTo(ApiDescription apiDescription) {
            //implement identical functionality to the base class with a custom visitor

        }
    }

    public class ODataAttributeVisitorOverrides : ODataAttributeVisitor {
        //implement the Seal-methods so that they do not skip when all properties are allowed
    }

The main issue with this is that the ODataAttributeVisitor is internal and it cannot be overridden. In order for this approach to work I would need to copy-paste the entire implementation of the Visitor - probably not worth it, all things considered.

Making ODataValidationSettingsConvention accept the Visitor as a parameter would, at a glance, make this quite simple to override.

[commonsensesoftware]

Although the ODataAttributeVisitor is internal, it merely visits query options. As I understand it, the results are correct. The expectation for extension here was that you would override the protected members for each query option.

At most, you should only have to change NewFilterParameter:

protected override ApiParameterDescription NewFilterParameter( ODataQueryOptionDescriptionContext descriptionContext )
{
    // TODO: new implementation
}

Honestly, you probably don't even have to do that. IODataQueryOptionDescriptionProvider.Describe is where the detailed description comes from. You can likely just extend DefaultODataQueryOptionDescriptionProvider and override:

protected override string DescribeFilter( ODataQueryOptionDescriptionContext context )
{
   // TODO: new implementation for describing $filter
}

At this point there might be some forking or duplication, but it should be minimal. It will depend on how you want the description to come out. You could call the base implementation and append text, update the context before calling the base implementation and then append text, and so on.

With that in place, you just have to replace the IODataQueryOptionDescriptionProvider in Options.QueryOptions.DescriptionProvider.

[spaasis]

That's so clean! Thanks, that's at least one step further. I might still need hand holding for a while though... :)

This works and outputs the specified string for every filter parameter. Useful by itself, no doubt.

    public class ODataQueryOptionsDescriptionProviderOverrides : DefaultODataQueryOptionDescriptionProvider {
        protected override string DescribeFilter(ODataQueryOptionDescriptionContext context) {
            context.AllowedFilterProperties.Add("Testing testing");
            return base.DescribeFilter(context);
        }
    }

    services
       .AddODataApiExplorer(options => {
             options.QueryOptions.DescriptionProvider = new ODataQueryOptionsDescriptionProviderOverrides();
       });

However, via just this override I have no access to the underlying ModelBoundQuerySettings which I think I need in order to do the following (basically copy the Visitor functionality):

• check if no properties have been explicitly defined
• if so, add every property name to context.AllowedFilterProperties in my DescribeFilter override

From this override I don't seem to have access to the current type, meaning that I don't see a way to get to the model definition.

[commonsensesoftware]

I'm not sure where this ever landed. Sorry, I didn't realize or mean to leave you hanging. It looks like the thread wasn't actually done. I've been revisiting all the open issues and burning everything down. Looking at this again, it seems want would probably help is adding some additional information to the context such as the ApiDescription and resolving IEdmStructuredType, then I think that would expose all the other information given to the ODataAttributeVisitor without exposing it.

I would plan to release this in the forthcoming 6.0 release. Let me know if you have any other thoughts.

[spaasis]

No worries! That sounds like a good approach which could enable all sorts of nice tweaks besides this $filter issue 👌