search

OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
855 stars 473 forks source link

PageSize on EnableQuery attr does not respect EnableConstantParameterization #1523

Open adzhiljano opened 6 years ago

adzhiljano commented 6 years ago

First of all, great lib ! : )

Now, when PageSize is set on the EnableQuery attribute and EnableConstantParameterization is set to true (the default value), there is no SQL parameter in the resulting query. Which might pollute the SQL Server query plan cache with unwanted query plans (containing constants).

Assemblies affected

OData WebApi lib 6.1.0

Reproduce steps

[EnableQuery(PageSize = 10)]
public IQueryable<Order> Get() { return db.Orders; }

Expected result

Notice the TOP (@plinq0)

SELECT TOP (@p__linq__0) 
    [Extent1].[Id] AS [Id], 
    [Extent1].[Name] AS [Name],
    ...
    FROM [dbo].[Orders] AS [Extent1]
    ORDER BY [Extent1].[Id] ASC

Actual result

Notice the TOP (11)

SELECT TOP (11) 
    [Extent1].[Id] AS [Id], 
    [Extent1].[Name] AS [Name],
    ... 
    FROM [dbo].[Orders] AS [Extent1]
    ORDER BY [Extent1].[Id] ASC

Proposed solution

The system query option $top respects the EnableConstantParameterization flag already, so maybe something similar should be done for the PageSize handling.

In the ODataQueryOptions.ApplyTo method where the query options are applied, pass down the querySettings or querySettings.EnableConstantParameterization to LimitResults and then to TruncatedCollection<T> and the constructor (all constructors accepting IQueryable) should now look like:

public TruncatedCollection(IQueryable<T> source, int pageSize, bool parameterize)
    : base(ExpressionHelpers.Take(source, checked(pageSize + 1), typeof(T), parameterize) as IQueryable<T>)
{
    Initialize(pageSize);
}

instead of the current:

public TruncatedCollection(IQueryable<T> source, int pageSize)
    : base(source.Take(checked(pageSize + 1)))
{
    Initialize(pageSize);
}

Tell me what you think about this solution, and I can make a PR.

Thanks!

biaol-odata commented 6 years ago

Hi @adzhiljano, WebApi v6.1 is in maintenance mode. The master branch supports WebApi v7 (for classic AspNet and the new AspNetCore). Can you check this solution is applicable to the master branch and port accordingly? Thanks.

adzhiljano commented 6 years ago

Hello @biaol-odata, does that mean WebApi 6.1 won't get any fixes/updates and there won't be a 6.2 version for example? If yes, is it safe to upgrade to 7.0?

I checked and yes, the solution is applicable. I will send a PR later today/tomorrow.

biaol-odata commented 6 years ago

@adzhiljano: Correct, we don't have plan for 6.2 release at this moment. Upgrading to 7.0 should be safe but, unfortunately, there will be some breaking changes along with the major version release. We have tried to reduce the breaking changes to as minimum as possible. Please refer to details here. Please also let me know when your PR on master is available.

adzhiljano commented 6 years ago

Hello @biaol-odata, Thanks for responding! I went through the breaking changes and I think we'll be able to upgrade to 7.0.

As for the PR, I'm running in some issues with building the solutions. When I tried to run the build.cmd at first I got Build WebApiOData.AspNet.sln FAILED. with lots of errors like this:

CSC : error CS8032: An instance of analyzer Xunit.Analyzers.TheoryMethodMustHaveTestData cannot be created from D:\Projects\My-OData-WebApi\sln\packages\xunit.analyzers.0.7.0\analyzers\dotnet\cs\xunit.analyzers.dll : Could not load file or assembly 'Microsoft.CodeAnalysis, Version=1.2.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The system cannot find the file specified.. [D:\Projects\My-OData-WebApi\test\UnitTest\Microsoft.AspNet.OData.Test\Microsoft.AspNet.OData.Test.csproj]

Here is the msbuild.log : https://gist.github.com/adzhiljano/395a582548bafa652d2624225bb8d901

I installed the Microsoft.CodeAnalysis 1.2.0 (just created a new sln) and then manually added that to the GAC like .\gacutil.exe -i C:\Users\Nasko\.nuget\packages\Microsoft.CodeAnalysis.Common\1.2.0\lib\net45\Microsoft.CodeAnalysis.dll (Not sure if this is the right approach to deal with this error)

Then I ran the build.cmd again and I got something similar:

CSC : error CS8032: An instance of analyzer Xunit.Analyzers.TheoryMethodMustHaveTestData cannot be created from D:\Projects\My-OData-WebApi\sln\packages\xunit.analyzers.0.7.0\analyzers\dotnet\cs\xunit.analyzers.dll : Could not load file or assembly 'System.Collections.Immutable, Version=1.1.37.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.. [D:\Projects\My-OData-WebApi\test\UnitTest\Microsoft.AspNet.OData.Test\Microsoft.AspNet.OData.Test.csproj]

Here is the msbuild.log : https://gist.github.com/adzhiljano/a3bcdbb60a851dc87b44a9de0a96f8e1

I installed the System.Collections.Immutable 1.1.37.0 (just created a new sln) and then manually added that to the GAC like .\gacutil.exe -i C:\Users\Nasko\.nuget\packages\system.collections.immutable\1.1.37\lib\dotnet\System.Collections.Immutable.dll (Not sure if this is the right approach to deal with this error)

Then I ran the build.cmd again and got the following:

CSC : error CS8033: The assembly D:\Projects\My-OData-WebApi\sln\packages\xunit.analyzers.0.7.0\analyzers\dotnet\cs\xunit.analyzers.dll does not contain any analyzers. [D:\Projects\My-OData-WebApi\test\UnitTest\Microsoft.AspNet.OData.Test\Microsoft.AspNet.OData.Test.csproj]

Here is the msbuild.log: https://gist.github.com/adzhiljano/706eb62d70f648094fc01b6bad9caed3

And this is where I'm stuck. I'm getting the feeling that I don't have the proper build tools. Do you have any ideas on what's wrong?

Nevertheless, I wrote my changes and successfully built the WebApiOData.AspNet.sln with VS 2017 (15.7.5), but I can't build the WebApiOData.AspNetCore.sln with the same VS 2017, because of Strong name validation failed errors. I'm not sure if running the Function SkipStrongName from the build script will have any affect on VS when building, but even if it does I have no output in the ...\bin\Debug\*\netstandard2.0. Actually tried that and all I got was: Failed to open metadata scope on D:\Projects\My-OData-WebApi\bin\Debug\netstandard2.0\Microsoft.AspNetCore.OData.dll -- The system cannot find the file specified. as expected.

My changes aren't that big and it should't break anything and I can submit the PR, but I would like everything to be checked and built, so that I don't have to blindly send PRs in the future.

Thanks!

adzhiljano commented 6 years ago

I managed to fix my issues by installing VS 2015 : ) I thought that the VS2015 build tools would be enough, but that was not the case. But still I had issues with building the AspNetCore project. With VS 2017 I got:

Error   CS8034  Unable to load Analyzer assembly C:\Users\Nasko\.nuget\packages\system.runtime.analyzers\1.0.0\analyzers\dotnet\cs\System.Runtime.Analyzers.dll : Could not load file or assembly 'System.Runtime.Analyzers, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. Strong name validation failed. (Exception from HRESULT: 0x8013141A)
Error   CS8034  Unable to load Analyzer assembly C:\Users\Nasko\.nuget\packages\system.runtime.analyzers\1.0.0\analyzers\dotnet\cs\System.Runtime.CSharp.Analyzers.dll : Could not load file or assembly 'System.Runtime.CSharp.Analyzers, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. Strong name validation failed. (Exception from HRESULT: 0x8013141A)
Error   CS8034  Unable to load Analyzer assembly C:\Users\Nasko\.nuget\packages\system.runtime.interopservices.analyzers\1.0.0\analyzers\dotnet\cs\System.Runtime.InteropServices.Analyzers.dll : Could not load file or assembly 'System.Runtime.InteropServices.Analyzers, Version=1.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. Strong name validation failed. (Exception from HRESULT: 0x8013141A)

and those errors led me to the following analyzer issue and then I temporarily upgraded the Microsoft.CodeAnalysis.FxCopAnalyzers package to 2.6.1 which led to a long list of analyzer errors in the code, so I decided to temporarily downgrade to 1.0.1 (contrary to what is recommended) and this is how I was able to build everything.

Despite the struggle created a PR for this issue : )