When [Select] attribute on Entity without [Page(MaxTop = 100)] attribute, then MaxTop set to 0

[ptrushin]

Assemblies affected ASP.NET Core OData 8.x

Describe the bug When [Select] attribute on Entity without [Page(MaxTop = 100)] attribute, then MaxTop set to 0 For entity without [Select] attribute all ok, MaxTop set from global services.AddControllers().AddOData(opt => opt.Count().Filter().Expand().Select().OrderBy().SetMaxTop(100000).AddRouteComponents("odata", GetEdmModel()))

Reproduce steps 1 [Select(SelectType = SelectExpandType.Automatic)] [Select(nameof(Memo), SelectType = SelectExpandType.Allowed)] //[Page(MaxTop = 1000000)] public class Ware ...

2 try url /odata/Ware?$top=10

3 result is "The query specified in the URI is not valid. The limit of '0' for Top query has been exceeded. The value from the incoming request is '10'."

Data Model

EDM (CSDL) Model

Request/Response

Expected behavior When Page attribute absent, then MaxTop get from global.

Screenshots

Additional context

[julealgon]

Can you also share the code for the corresponding controller action?

[corranrogue9]

From triage: this should be straightforward to repro with one of our samples. Agree with @julealgon, please add the controller action. We did have a previous bug assigned to June recently around investigating the interaction between the SetMaxTop and the [Page(MaxTop = ...)] attribute. We should understand that investigation, and then address this but ensuring that we never set the max top to 0 as the default.

[ptrushin]

Can you also share the code for the corresponding controller action?

[HttpGet]
[EnableQuery]
public ActionResult<IQueryable<T>> Get()
{
        return Ok(Db.Set<Ware>().AsNoTracking());
}

[devlie]

I think the repro may be simpler. In my case:

1. I have this in Startup.cs:

   .AddOData(opt =>
   {
       opt.Filter().Select().Expand().SkipToken().SetMaxTop(100); 
       opt.EnableNoDollarQueryOptions = true;
   });
   

2. I do not have `Page[(MaxTop= ..._] in my EDM model.

3. I create the ODataValidationSettings myself:

   var validationSettings = new ODataValidationSettings
   {
       MaxTop = _config.MaxTop, // this is set to 100
       AllowedQueryOptions = AllowedQueryOptions.Expand | AllowedQueryOptions.Top | AllowedQueryOptions.SkipToken | AllowedQueryOptions.Filter,
       AllowedLogicalOperators = AllowedLogicalOperators.Equal,
       AllowedFunctions = AllowedFunctions.None,
       AllowedArithmeticOperators = AllowedArithmeticOperators.None,
   };
   

4. I called ODataQueryOptions<T>.Validate(), passing in the settings from step 3 and it throws at this location, complaning about $top exceeding 0.

I would expect the limit from step 1 to be use for undecorated EdmModels. The code above was working in v7.x

[commonsensesoftware]

Some additional notes on this bug.

• EnableQueryAttribute.MaxTop and EnableQueryAttribute.MaxSkip are incongruent
  • There is no global SetMaxSkip
  • Once enabled, $skip works without specifically setting MaxSkip
• If $top is not enabled anywhere, but is used, there is no error; it is simply ignored (which seems wrong since it's disallowed)
• If $skip is not enabled anywhere, but is used, there is no error; it is simply ignored (which seems wrong since it's disallowed)

As I recall, MaxTop is defined in one place as int? and another place as int. Since a value of 0 would have no meaning, null and 0 are equivalent, but I don't believe they are always compared or used that way.

[commonsensesoftware]

Ok, I believe I've found the primary culprit.

A Bit of Astoria

There is some historical context to MaxTop being int instead of int?. The main reason is that Nullable<T> didn't exist when the code was first written. This has changed quite bit between the different OData teams and owners over the many years. Suffice it to say, that even today, there is a mismatch between 0 and null, but they really mean the same thing. It's somewhat amusing that the int? implementation only allows >= 1 or null.

Analysis

So why does this happen? First, you have to look here:

https://github.com/OData/ModelBuilder/blob/5ebe57dba23bf0d9f341ca9e8a66bd12298198ad/src/Microsoft.OData.ModelBuilder/Config/ModelBoundQuerySettings.cs#L14

        private int? _maxTop = 0;

This makes no sense to me. If you have int? _maxTop, why initialize to 0? This may not be the only place that triggers the behavior, but this conflation of null and 0 is one of the primary cases.

So what are the side effects? Next, we have to look here:

https://github.com/OData/AspNetCoreOData/blob/4de92f52a346606a447ec4df96c5f3cd05642f50/src/Microsoft.AspNetCore.OData/Edm/EdmHelpers.cs#L375

This highlights that the DefaultQuerySettings are only considered if there are no ModelBoundQuerySettings. The catch is that if you use any model bound query settings by attribute or convention, you are on the hook to also specify whatever you want for $top (at least in the current implementation). If you configure things this way, then value specified through the SetMaxTop global configuration has no effect.

To further demonstrate how inconsistent this is, you need only look at:

https://github.com/OData/ModelBuilder/blob/5ebe57dba23bf0d9f341ca9e8a66bd12298198ad/src/Microsoft.OData.ModelBuilder/Attributes/PageAttribute.cs#L15

        private int _maxTop = -1;

Here MaxTop is not int? and defaults to -1 instead of 0 as seen in other places. This isn't even consistent with the equivalent fluent API on the builder:

https://github.com/OData/ModelBuilder/blob/5ebe57dba23bf0d9f341ca9e8a66bd12298198ad/src/Microsoft.OData.ModelBuilder/Types/StructuralTypeConfigurationOfTStructuralType.cs#L794

public StructuralTypeConfiguration<TStructuralType> Page(int? maxTopValue, int? pageSizeValue)

These behavior ultimately results in things falling here:

https://github.com/OData/AspNetCoreOData/blob/4de92f52a346606a447ec4df96c5f3cd05642f50/src/Microsoft.AspNetCore.OData/Edm/EdmHelpers.cs#L144

The other catch is that if you set [EnableQuery(MaxTop = 100)], it is enforced after model bound query setting validation.

Workaround

My understanding of OData's default behavior is supposed be that $top should be configurable and enabled through other means (say [EnableQuery]) as long as it is not explicitly set elsewhere. The current implementation is treating MaxTop as being explicitly set even if it hasn't been.

There are 3 possible workarounds:

• Do not use any model bound settings, set the global SetMaxTop, and apply it with [EnableQuery]
• Use [Page(MaxTop=42)] on your model
• Set it via the model builder a la builder.EntitySet<Order>("Orders").EntityType.Page(maxTopValue: 42, default)

The Fix

This has painfully happened to me so many times, that I wrote extensions methods to cover all the cases:

internal static class NullableExtensions
{
    public static bool Unset( this int? value ) => value.HasValue && value.Value == 0;
    public static bool NoLimitOrSome( this int? value ) => !value.HasValue || value.Value > 0;
    public static bool NoLimitOrNone( this int? value ) => !value.HasValue || value.Value <= 0;
}

That way I can have something like:

if (querySettings.MaxTop.Unset())
{
}

@xuzhg, this seems like pretty low hanging fruit. As there is seemingly no case where I've ever seen 0 be a valid value, can someone from the team add some additional checks that will universally treat null and 0 as unset? This would be great to go out in the next patch. I can't imagine it would take more than a 1-2 days to track down all the use cases.

[relair]

This is still a bug in version 8.2.0, is this library still maintained?

[julealgon]

...is this library still maintained?

Lol c'mon now... that's kinda mean 😅