Skip to content
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

Generating Parameterized SQL #185

Closed
wants to merge 22 commits into from
Closed

Conversation

sspekinc
Copy link

Converting constant expressions to the wrapped ones will enable the generated SQL is to be parameterized as proposed in the following blog post.

https://github.com/graeme-hill/gblog/blob/master/source_content/articles/2014.139_entity-framework-dynamic-queries-and-parameterization.mkd

Converting constant expressions to the wrapped ones will enable the generated SQL is to be parameterized as proposed in the following blog post.

https://github.com/graeme-hill/gblog/blob/master/source_content/articles/2014.139_entity-framework-dynamic-queries-and-parameterization.mkd
Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some comments, please take a look.

}
}

public static MemberExpression WrappedConstant<TValue>(TValue value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be private

}
}

public static MemberExpression WrappedConstant<TValue>(TValue value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment here how this is working. (Maybe add the link to that article, or maybe better to also add the article as file to this git.

@StefH StefH changed the title Generating Paratemeterized SQL Generating Parameterized SQL Jul 23, 2018
@Jo1nes
Copy link

Jo1nes commented Oct 17, 2018

Hi, I have the same requirement and I very glad there is already a fix. Many thanks! Can you tell me if this will be included in the next release?

@StefH
Copy link
Collaborator

StefH commented Oct 23, 2018

Hello @sspekinc and @Jo1nes,

There are some code review comments and 3 unit-tests fail.
See https://ci.appveyor.com/project/StefH/system-linq-dynamic-core/build/1.0.8.576 for more details.

@sspekinc
Copy link
Author

sspekinc commented Oct 23, 2018 via email

{
expression = WrappedConstant((string)constantExpression.Value);
}
else if (constantExpression.Type == typeof(Int64))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should also be a check for unsigned long, int and short here.

Also char, bool, byte, sbyte, float, decimal and double.

Also DateTimeOffset, TimeSpan.

Double check the file PredefinedTypesHelper, this file has some lists of types which you can use.

StefH
StefH previously requested changes Oct 24, 2018
@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #185 into master will decrease coverage by 0.4%.
The diff coverage is 63.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
- Coverage   82.49%   82.09%   -0.41%     
==========================================
  Files          37       39       +2     
  Lines        3702     3809     +107     
  Branches      508      529      +21     
==========================================
+ Hits         3054     3127      +73     
- Misses        501      535      +34     
  Partials      147      147
Impacted Files Coverage Δ
...rc/System.Linq.Dynamic.Core/Parser/WrappedValue.cs 100% <100%> (ø)
...ystem.Linq.Dynamic.Core/Parser/ExpressionHelper.cs 79.57% <100%> (+1.88%) ⬆️
...q.Dynamic.Core/Parser/ConstantExpressionWrapper.cs 55.55% <55.55%> (ø)
...ystem.Linq.Dynamic.Core/Parser/ExpressionParser.cs 78.59% <0%> (+0.16%) ⬆️
src/System.Linq.Dynamic.Core/Parser/TypeHelper.cs 87.26% <0%> (+0.47%) ⬆️
...c/System.Linq.Dynamic.Core/Tokenizer/TextParser.cs 98.03% <0%> (+0.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a6bf6c...48c7b2d. Read the comment docs.

@StefH
Copy link
Collaborator

StefH commented Oct 24, 2018

I've refactored some code.
When I run the ConsoleApp_net452_EF6 example, the SQL profiler shows this trace for all 3 queries:

exec sp_executesql N'SELECT TOP (1) 
    [Extent1].[Id] AS [Id], 
    [Extent1].[EmployeeNumber] AS [EmployeeNumber], 
    [Extent1].[FirstName] AS [FirstName], 
    [Extent1].[LastName] AS [LastName], 
    [Extent1].[Email] AS [Email], 
    [Extent1].[HireDate] AS [HireDate], 
    [Extent1].[CompanyId] AS [CompanyId], 
    [Extent1].[CountryId] AS [CountryId], 
    [Extent1].[FunctionId] AS [FunctionId], 
    [Extent1].[SubFunctionId] AS [SubFunctionId], 
    [Extent1].[Assigned] AS [Assigned]
    FROM [dbo].[KendoGrid_Employee] AS [Extent1]
    WHERE [Extent1].[EmployeeNumber] > @p__linq__0',N'@p__linq__0 int',@p__linq__0=1000
exec sp_executesql N'SELECT TOP (1) 
    [Extent1].[Id] AS [Id], 
    [Extent1].[EmployeeNumber] AS [EmployeeNumber], 
    [Extent1].[FirstName] AS [FirstName], 
    [Extent1].[LastName] AS [LastName], 
    [Extent1].[Email] AS [Email], 
    [Extent1].[HireDate] AS [HireDate], 
    [Extent1].[CompanyId] AS [CompanyId], 
    [Extent1].[CountryId] AS [CountryId], 
    [Extent1].[FunctionId] AS [FunctionId], 
    [Extent1].[SubFunctionId] AS [SubFunctionId], 
    [Extent1].[Assigned] AS [Assigned]
    FROM [dbo].[KendoGrid_Employee] AS [Extent1]
    WHERE [Extent1].[EmployeeNumber] > @p__linq__0',N'@p__linq__0 int',@p__linq__0=1001
exec sp_executesql N'SELECT TOP (1) 
    [Extent1].[Id] AS [Id], 
    [Extent1].[EmployeeNumber] AS [EmployeeNumber], 
    [Extent1].[FirstName] AS [FirstName], 
    [Extent1].[LastName] AS [LastName], 
    [Extent1].[Email] AS [Email], 
    [Extent1].[HireDate] AS [HireDate], 
    [Extent1].[CompanyId] AS [CompanyId], 
    [Extent1].[CountryId] AS [CountryId], 
    [Extent1].[FunctionId] AS [FunctionId], 
    [Extent1].[SubFunctionId] AS [SubFunctionId], 
    [Extent1].[Assigned] AS [Assigned]
    FROM [dbo].[KendoGrid_Employee] AS [Extent1]
    WHERE [Extent1].[EmployeeNumber] > @p__linq__0',N'@p__linq__0 int',@p__linq__0=1002

So it seems the code is not working ?

Refactor and move some classes
@sspekinc
Copy link
Author

Seems good. What's next?

@StefH
Copy link
Collaborator

StefH commented Oct 25, 2018

@sspekinc Looks good.

one thing I was thinking : do we want this to be default behaviour? Or make this configurable using settings?

@sspekinc
Copy link
Author

I think it should be the default behavior but it would be good to be able to turn it off through ParsingConfig.

@StefH
Copy link
Collaborator

StefH commented Oct 27, 2018

Added config setting.

Closing this PR ; somehow I need to use my own PR for merging...

@StefH StefH closed this Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants