From b62269873672e15d2a4e7f95805ba7fc7922211a Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 17 Nov 2023 07:58:03 +0000 Subject: [PATCH 1/4] fix #79 --- .../SqlAnalysis/TSqlProcessor.cs | 43 +++++++++++-------- test/Dapper.AOT.Test/Verifiers/DAP025.cs | 13 ++++++ test/Dapper.AOT.Test/Verifiers/DAP026.cs | 13 ++++++ test/Dapper.AOT.Test/Verifiers/Verifier.cs | 6 ++- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs index 32e74ab0..a6cc36c7 100644 --- a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs @@ -755,33 +755,37 @@ public override void Visit(SelectStatement node) } index++; } + if (reads != 0) { if (sets != 0) { parser.OnSelectAssignAndRead(spec); } - bool firstQuery = AddQuery(); - if (firstQuery && SingleRow // optionally enforce single-row validation - && spec.FromClause is not null) // no "from" is things like 'select @id, @name' - always one row + if (node.Into is null) // otherwise not actually a query { - bool haveTopOrFetch = false; - if (spec.TopRowFilter is { Percent: false, Expression: ScalarExpression top }) + bool firstQuery = AddQuery(); + if (firstQuery && SingleRow // optionally enforce single-row validation + && spec.FromClause is not null) // no "from" is things like 'select @id, @name' - always one row { - haveTopOrFetch = EnforceTop(top); - } - else if (spec.OffsetClause is { FetchExpression: ScalarExpression fetch }) - { - haveTopOrFetch = EnforceTop(fetch); - } + bool haveTopOrFetch = false; + if (spec.TopRowFilter is { Percent: false, Expression: ScalarExpression top }) + { + haveTopOrFetch = EnforceTop(top); + } + else if (spec.OffsetClause is { FetchExpression: ScalarExpression fetch }) + { + haveTopOrFetch = EnforceTop(fetch); + } - // we want *either* a WHERE (which we will allow with/without a TOP), - // or a TOP + ORDER BY - if (!IsUnfiltered(spec.FromClause, spec.WhereClause)) { } // fine - else if (haveTopOrFetch && spec.OrderByClause is not null) { } // fine - else - { - parser.OnSelectSingleRowWithoutWhere(node); + // we want *either* a WHERE (which we will allow with/without a TOP), + // or a TOP + ORDER BY + if (!IsUnfiltered(spec.FromClause, spec.WhereClause)) { } // fine + else if (haveTopOrFetch && spec.OrderByClause is not null) { } // fine + else + { + parser.OnSelectSingleRowWithoutWhere(node); + } } } } @@ -1340,6 +1344,9 @@ public override void ExplicitVisit(BinaryExpression node) public override void Visit(OutputClause node) { + // note that this doesn't handle OUTPUT INTO, + // which is via OutputIntoClause + AddQuery(); // works like a query base.Visit(node); } diff --git a/test/Dapper.AOT.Test/Verifiers/DAP025.cs b/test/Dapper.AOT.Test/Verifiers/DAP025.cs index 51a5ac09..2746fb75 100644 --- a/test/Dapper.AOT.Test/Verifiers/DAP025.cs +++ b/test/Dapper.AOT.Test/Verifiers/DAP025.cs @@ -46,4 +46,17 @@ exec SomeLookupFetch Diagnostic(Diagnostics.ExecuteCommandWithQuery).WithLocation(0), ]); + [Fact] + public Task ReportWhenNoQueryExpected() => SqlVerifyAsync(""" + SELECT [Test] + FROM MyTable + """, SqlAnalysis.SqlParseInputFlags.ExpectNoQuery, + Diagnostic(Diagnostics.ExecuteCommandWithQuery).WithLocation(Execute)); + + [Fact] // false positive scenario, https://github.com/DapperLib/DapperAOT/issues/79 + public Task DoNotReportSelectInto() => SqlVerifyAsync(""" + SELECT [Test] + INTO #MyTemporaryTable FROM MyTable + """, SqlAnalysis.SqlParseInputFlags.ExpectNoQuery); + } \ No newline at end of file diff --git a/test/Dapper.AOT.Test/Verifiers/DAP026.cs b/test/Dapper.AOT.Test/Verifiers/DAP026.cs index 5df09bf6..a1c4746c 100644 --- a/test/Dapper.AOT.Test/Verifiers/DAP026.cs +++ b/test/Dapper.AOT.Test/Verifiers/DAP026.cs @@ -47,4 +47,17 @@ exec SomeLookupInsert 'North' Diagnostic(Diagnostics.QueryCommandMissingQuery).WithLocation(0), ]); + [Fact] + public Task DoNotReportWhenQueryExpected() => SqlVerifyAsync(""" + SELECT [Test] + FROM MyTable + """, SqlAnalysis.SqlParseInputFlags.ExpectQuery); + + [Fact] + public Task ReportSelectInto() => SqlVerifyAsync(""" + SELECT [Test] + INTO #MyTemporaryTable FROM MyTable + """, SqlAnalysis.SqlParseInputFlags.ExpectQuery, + Diagnostic(Diagnostics.QueryCommandMissingQuery).WithLocation(Execute)); + } \ No newline at end of file diff --git a/test/Dapper.AOT.Test/Verifiers/Verifier.cs b/test/Dapper.AOT.Test/Verifiers/Verifier.cs index 6b718bcb..7bc115b0 100644 --- a/test/Dapper.AOT.Test/Verifiers/Verifier.cs +++ b/test/Dapper.AOT.Test/Verifiers/Verifier.cs @@ -172,7 +172,9 @@ protected static Func WithVisualBasicParseOptions { internal Task SqlVerifyAsync(string sql, params DiagnosticResult[] expected) => SqlVerifyAsync(sql, SqlParseInputFlags.None, expected); - + + public const int Execute = 9999; + internal Task SqlVerifyAsync(string sql, SqlParseInputFlags sqlParseInputFlags, params DiagnosticResult[] expected) { var cs = $$""" @@ -182,7 +184,7 @@ internal Task SqlVerifyAsync(string sql, SqlParseInputFlags sqlParseInputFlags, [DapperAot(false)] class SomeCode { - public void Foo(DbConnection conn) => conn.Execute(@"{{sql.Replace("\"", "\"\"")}}"); + public void Foo(DbConnection conn) => conn.{|#{{Execute}}:Execute|}(@"{{sql.Replace("\"", "\"\"")}}"); } """; return CSVerifyAsync(cs, DefaultConfig, expected, SqlSyntax.SqlServer, sqlParseInputFlags | SqlParseInputFlags.DebugMode); From 217ddc85bd5c7fed35b5ae2a31857e60aa2e87e5 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 17 Nov 2023 09:00:39 +0000 Subject: [PATCH 2/4] fix #80; add new rule 243 for invalid datepart expressions --- docs/rules/DAP243.md | 12 ++++ .../DapperAnalyzer.Diagnostics.cs | 3 +- .../Internal/DiagnosticTSqlProcessor.cs | 7 +- .../SqlAnalysis/TSqlProcessor.cs | 72 ++++++++++++++++++- test/Dapper.AOT.Test/Verifiers/DAP226.cs | 9 ++- test/Dapper.AOT.Test/Verifiers/DAP243.cs | 29 ++++++++ 6 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 docs/rules/DAP243.md create mode 100644 test/Dapper.AOT.Test/Verifiers/DAP243.cs diff --git a/docs/rules/DAP243.md b/docs/rules/DAP243.md new file mode 100644 index 00000000..030bfc65 --- /dev/null +++ b/docs/rules/DAP243.md @@ -0,0 +1,12 @@ +# DAP243 + +Date functions like `DATEADD`, `DATEPART` expect a *datepart* token as the first argument; for example in + +``` sql +DATEPART(year, GETDATE()) +``` + +the `year` is the *datepart* token. These values cannot be parameterized or take a value +from a column - they are a special kind of literal value. + +For the list of valid tokens, see [here](https://learn.microsoft.com/sql/t-sql/functions/dateadd-transact-sql). \ No newline at end of file diff --git a/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs b/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs index c56a44e2..54290931 100644 --- a/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs +++ b/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs @@ -93,6 +93,7 @@ public static readonly DiagnosticDescriptor InvalidNullExpression = SqlWarning("DAP239", "Invalid null expression", "Operation requires a non-null operand"), VariableParameterConflict = SqlError("DAP240", "Parameter/variable conflict", "The declaration of variable '{0}' conflicts with a parameter"), InterpolatedStringSqlExpression = SqlWarning("DAP241", "Interpolated string usage", "Data values should not be interpolated into SQL string - use parameters instead"), - ConcatenatedStringSqlExpression = SqlWarning("DAP242", "Concatenated string usage", "Data values should not be concatenated into SQL string - use parameters instead"); + ConcatenatedStringSqlExpression = SqlWarning("DAP242", "Concatenated string usage", "Data values should not be concatenated into SQL string - use parameters instead"), + InvalidDatepartToken = SqlWarning("DAP243", "Valid datepart token expected", "Date functions require a recognized datepart argument"); } } diff --git a/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs b/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs index 9e51806f..e9302d26 100644 --- a/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs @@ -4,10 +4,7 @@ using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.SqlServer.TransactSql.ScriptDom; -using System; -using System.Data; using System.Diagnostics; -using static Dapper.SqlAnalysis.TSqlProcessor; namespace Dapper.Internal; @@ -126,6 +123,10 @@ protected override void OnFromMultiTableMissingAlias(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.FromMultiTableMissingAlias, location); protected override void OnFromMultiTableUnqualifiedColumn(Location location, string name) => OnDiagnostic(DapperAnalyzer.Diagnostics.FromMultiTableUnqualifiedColumn, location, name); + + protected override void OnInvalidDatepartToken(Location location) + => OnDiagnostic(DapperAnalyzer.Diagnostics.InvalidDatepartToken, location); + protected override void OnNonIntegerTop(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.NonIntegerTop, location); protected override void OnNonPositiveTop(Location location) diff --git a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs index a6cc36c7..843d72b6 100644 --- a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs @@ -250,6 +250,9 @@ protected virtual void OnFromMultiTableMissingAlias(Location location) => OnError($"FROM expressions with multiple elements should use aliases", location); protected virtual void OnFromMultiTableUnqualifiedColumn(Location location, string name) => OnError($"FROM expressions with multiple elements should qualify all columns; it is unclear where '{name}' is located", location); + + protected virtual void OnInvalidDatepartToken(Location location) + => OnError($"Valid datepart token expected", location); protected virtual void OnTopWithOffset(Location location) => OnError($"TOP cannot be used when OFFSET is specified", location); @@ -702,6 +705,66 @@ public override void Visit(QuerySpecification node) base.Visit(node); } + public override void ExplicitVisit(FunctionCall node) + { + ScalarExpression? ignore = null; + if (node.Parameters.Count != 0 && IsSpecialDateFunction(node.FunctionName.Value)) + { + ValidateDateArg(ignore = node.Parameters[0]); + } + var oldIgnore = _demandAliases.IgnoreNode; // stash + _demandAliases.IgnoreNode = ignore; + base.ExplicitVisit(node); // dive + _demandAliases.IgnoreNode = oldIgnore; // restore + + static bool IsSpecialDateFunction(string name) + => name.StartsWith("DATE", StringComparison.OrdinalIgnoreCase) + && IsAnyCaseInsensitive(name, DateTokenFunctions); + } + + static bool IsAnyCaseInsensitive(string value, string[] options) + { + if (value is not null) + { + foreach (var option in options) + { + if (string.Equals(option, value, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + } + return false; + } + // arg0 has special meaning - not a column/etc + static readonly string[] DateTokenFunctions = ["DATE_BUCKET", "DATEADD", "DATEDIFF", "DATEDIFF_BIG", "DATENAME", "DATEPART", "DATETRUNC"]; + static readonly string[] DateTokens = [ + "year", "yy", "yyyy", + "quarter", "qq", "q", + "month", "mm", "m", + "dayofyear", "dy", "y", + "day", "dd", "d", + "week", "wk", "ww", + "weekday", "dw", "w", + "hour", "hh", + "minute", "mi", "n", + "second", "ss", "s", + "millisecond", "ms", + "microsecond", "mcs", + "nanosecond", "ns" + ]; + + private void ValidateDateArg(ScalarExpression value) + { + if (!(value is ColumnReferenceExpression col + && col.MultiPartIdentifier.Count == 1 && IsAnyCaseInsensitive( + col.MultiPartIdentifier[0].Value, DateTokens ))) + { + parser.OnInvalidDatepartToken(value); + } + + } + public override void Visit(SelectStatement node) { if (node.QueryExpression is QuerySpecification spec) @@ -755,7 +818,7 @@ public override void Visit(SelectStatement node) } index++; } - + if (reads != 0) { if (sets != 0) @@ -1295,7 +1358,7 @@ bool TrySimplifyExpression(ScalarExpression node) { if (_expressionAlreadyEvaluated) return true; - if (node is UnaryExpression { UnaryExpressionType: UnaryExpressionType.Negative, Expression: IntegerLiteral or NumericLiteral }) + if (node is UnaryExpression { UnaryExpressionType: UnaryExpressionType.Negative, Expression: IntegerLiteral or NumericLiteral }) { // just "-4" or similar; don't warn the user to simplify that! _expressionAlreadyEvaluated = true; @@ -1416,6 +1479,8 @@ public DemandAliasesState(bool active, TableReference? amnesty) public readonly bool Active; public readonly TableReference? Amnesty; // we can't validate the target until too late public bool AmnestyNodeIsAlias; + + public ScalarExpression? IgnoreNode { get; set; } } private DemandAliasesState _demandAliases; @@ -1442,7 +1507,8 @@ public override void Visit(TableReferenceWithAlias node) } public override void Visit(ColumnReferenceExpression node) { - if (_demandAliases.Active && node.MultiPartIdentifier.Count == 1) + if (_demandAliases.Active && node.MultiPartIdentifier.Count == 1 + && !ReferenceEquals(node, _demandAliases.IgnoreNode)) { parser.OnFromMultiTableUnqualifiedColumn(node, node.MultiPartIdentifier[0].Value); } diff --git a/test/Dapper.AOT.Test/Verifiers/DAP226.cs b/test/Dapper.AOT.Test/Verifiers/DAP226.cs index 0ef1cbfa..62b78516 100644 --- a/test/Dapper.AOT.Test/Verifiers/DAP226.cs +++ b/test/Dapper.AOT.Test/Verifiers/DAP226.cs @@ -16,5 +16,12 @@ from A a from A a inner join B b on b.X = a.Id """, Diagnostic(Diagnostics.FromMultiTableUnqualifiedColumn).WithLocation(0).WithArguments("Name")); - + + [Fact] // false positive: https://github.com/DapperLib/DapperAOT/issues/80 + public Task DoNotReportDateAdd() => SqlVerifyAsync(""" + SELECT DATEADD(YEAR, 1, t.Year) AS NextYear + FROM MyTable t + JOIN MyOtherTable o ON o.Id = t.Id + """); + } \ No newline at end of file diff --git a/test/Dapper.AOT.Test/Verifiers/DAP243.cs b/test/Dapper.AOT.Test/Verifiers/DAP243.cs new file mode 100644 index 00000000..66197cd6 --- /dev/null +++ b/test/Dapper.AOT.Test/Verifiers/DAP243.cs @@ -0,0 +1,29 @@ +using Dapper.CodeAnalysis; +using System.Threading.Tasks; +using Xunit; +using Diagnostics = Dapper.CodeAnalysis.DapperAnalyzer.Diagnostics; +namespace Dapper.AOT.Test.Verifiers; + +public class DAP243 : Verifier +{ + [Fact] + public Task Valid() => SqlVerifyAsync(""" + SELECT DATEPART(Year, GETUTCDATE()) + """); + + [Fact] + public Task UnknownValue() => SqlVerifyAsync(""" + SELECT DATEPART({|#0:NotValid|}, GETUTCDATE()) + """, Diagnostic(Diagnostics.InvalidDatepartToken).WithLocation(0)); + + [Fact] + public Task Parameter() => SqlVerifyAsync(""" + SELECT DATEPART({|#0:@Foo|}, GETUTCDATE()) + """, Diagnostic(Diagnostics.InvalidDatepartToken).WithLocation(0)); + + [Fact] + public Task Literal() => SqlVerifyAsync(""" + SELECT DATEPART({|#0:42|}, GETUTCDATE()) + """, Diagnostic(Diagnostics.InvalidDatepartToken).WithLocation(0)); + +} \ No newline at end of file From 039fb24118de0060a230fb396d8d104885e8726d Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 17 Nov 2023 09:41:28 +0000 Subject: [PATCH 3/4] groundwork for aggregate --- .../DapperAnalyzer.Diagnostics.cs | 3 ++- .../Internal/DiagnosticTSqlProcessor.cs | 2 ++ .../SqlAnalysis/TSqlProcessor.cs | 27 +++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs b/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs index 54290931..1e4929d3 100644 --- a/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs +++ b/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs @@ -94,6 +94,7 @@ public static readonly DiagnosticDescriptor VariableParameterConflict = SqlError("DAP240", "Parameter/variable conflict", "The declaration of variable '{0}' conflicts with a parameter"), InterpolatedStringSqlExpression = SqlWarning("DAP241", "Interpolated string usage", "Data values should not be interpolated into SQL string - use parameters instead"), ConcatenatedStringSqlExpression = SqlWarning("DAP242", "Concatenated string usage", "Data values should not be concatenated into SQL string - use parameters instead"), - InvalidDatepartToken = SqlWarning("DAP243", "Valid datepart token expected", "Date functions require a recognized datepart argument"); + InvalidDatepartToken = SqlWarning("DAP243", "Valid datepart token expected", "Date functions require a recognized datepart argument"), + OnSelectAggregateAndNonAggregate = SqlWarning("DAP244", "SELECT aggregate mismatch", "SELECT has mixture of aggregate and non-aggregate expressions"); } } diff --git a/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs b/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs index e9302d26..bdc1f318 100644 --- a/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs @@ -139,6 +139,8 @@ protected override void OnSelectFirstTopError(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectFirstTopError, location); protected override void OnSelectSingleRowWithoutWhere(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleRowWithoutWhere, location); + protected override void OnSelectAggregateAndNonAggregate(Location location) + => OnDiagnostic(DapperAnalyzer.Diagnostics.OnSelectAggregateAndNonAggregate, location); protected override void OnSelectSingleTopError(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleTopError, location); diff --git a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs index 843d72b6..7d5042a8 100644 --- a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs @@ -238,6 +238,8 @@ protected virtual void OnSelectFirstTopError(Location location) => OnError($"SELECT for First* should use TOP 1", location); protected virtual void OnSelectSingleRowWithoutWhere(Location location) => OnError($"SELECT for single row without WHERE or (TOP and ORDER BY)", location); + protected virtual void OnSelectAggregateAndNonAggregate(Location location) + => OnError($"SELECT has mixture of aggregate and non-aggregate expressions", location); protected virtual void OnNonPositiveTop(Location location) => OnError($"TOP literals should be positive", location); protected virtual void OnNonPositiveFetch(Location location) @@ -780,6 +782,7 @@ public override void Visit(SelectStatement node) var checkNames = ValidateSelectNames; HashSet names = checkNames ? new HashSet(StringComparer.InvariantCultureIgnoreCase) : null!; + var aggregate = AggregateFlags.None; int index = 0; foreach (var el in spec.SelectElements) { @@ -787,6 +790,7 @@ public override void Visit(SelectStatement node) { case SelectStarExpression: parser.OnSelectStar(el); + aggregate |= AggregateFlags.HaveNonAggregate; reads++; break; case SelectScalarExpression scalar: @@ -810,6 +814,7 @@ public override void Visit(SelectStatement node) parser.OnSelectDuplicateColumnName(scalar, name!); } } + aggregate |= IsAggregate(scalar.Expression); reads++; break; case SelectSetVariable: @@ -819,6 +824,11 @@ public override void Visit(SelectStatement node) index++; } + if (aggregate == (AggregateFlags.HaveAggregate | AggregateFlags.HaveNonAggregate)) + { + parser.OnSelectAggregateAndNonAggregate(spec); + } + if (reads != 0) { if (sets != 0) @@ -845,6 +855,7 @@ public override void Visit(SelectStatement node) // or a TOP + ORDER BY if (!IsUnfiltered(spec.FromClause, spec.WhereClause)) { } // fine else if (haveTopOrFetch && spec.OrderByClause is not null) { } // fine + else if ((aggregate & (AggregateFlags.HaveAggregate | AggregateFlags.Uncertain)) != 0) { } // fine else { parser.OnSelectSingleRowWithoutWhere(node); @@ -857,6 +868,22 @@ public override void Visit(SelectStatement node) base.Visit(node); } + enum AggregateFlags + { + None = 0, + HaveAggregate = 1 << 0, + HaveNonAggregate = 1 << 1, + Uncertain = 1 << 2, + } + private AggregateFlags IsAggregate(ScalarExpression expression) + { + // any use of an aggregate function contributes HaveAggregate + // - there could be unary/binary operations on that aggregate function + // - column references inside the aggregate expression or a sub-query are fine, + // otherwise they contribute HaveNonAggregate + return AggregateFlags.Uncertain; + } + private bool EnforceTop(ScalarExpression expr) { if (IsInt32(expr, out var i) == TryEvaluateResult.SuccessConstant && i.HasValue) From aaf1361d5e39095904bedda1aaa81f98b6bfbf04 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 17 Nov 2023 10:20:56 +0000 Subject: [PATCH 4/4] fix #81; add new rule DAP244 for mixing aggregate and non-aggregate data --- docs/rules/DAP244.md | 28 +++++++++++++++ .../DapperAnalyzer.Diagnostics.cs | 2 +- .../Internal/DiagnosticTSqlProcessor.cs | 2 +- .../SqlAnalysis/TSqlProcessor.cs | 36 ++++++++++++++++--- test/Dapper.AOT.Test/Verifiers/DAP231.cs | 10 ++++++ test/Dapper.AOT.Test/Verifiers/DAP244.cs | 27 ++++++++++++++ 6 files changed, 99 insertions(+), 6 deletions(-) create mode 100644 docs/rules/DAP244.md create mode 100644 test/Dapper.AOT.Test/Verifiers/DAP244.cs diff --git a/docs/rules/DAP244.md b/docs/rules/DAP244.md new file mode 100644 index 00000000..5bfd33ff --- /dev/null +++ b/docs/rules/DAP244.md @@ -0,0 +1,28 @@ +# DAP244 + +A `SELECT` expression cannot mix aggregate and non-aggregate results; + +Fine: + +``` sql +select Name +from SomeTable +``` + +or + +``` sql +select COUNT(1) +from SomeTable +``` + +Invalid: + +or + +``` sql +select Name, COUNT(1) +from SomeTable +``` + +Aggregate functions are [listed here](https://learn.microsoft.com/sql/t-sql/functions/aggregate-functions-transact-sql). \ No newline at end of file diff --git a/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs b/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs index 1e4929d3..a47041b1 100644 --- a/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs +++ b/src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs @@ -95,6 +95,6 @@ public static readonly DiagnosticDescriptor InterpolatedStringSqlExpression = SqlWarning("DAP241", "Interpolated string usage", "Data values should not be interpolated into SQL string - use parameters instead"), ConcatenatedStringSqlExpression = SqlWarning("DAP242", "Concatenated string usage", "Data values should not be concatenated into SQL string - use parameters instead"), InvalidDatepartToken = SqlWarning("DAP243", "Valid datepart token expected", "Date functions require a recognized datepart argument"), - OnSelectAggregateAndNonAggregate = SqlWarning("DAP244", "SELECT aggregate mismatch", "SELECT has mixture of aggregate and non-aggregate expressions"); + SelectAggregateMismatch = SqlWarning("DAP244", "SELECT aggregate mismatch", "SELECT has mixture of aggregate and non-aggregate expressions"); } } diff --git a/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs b/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs index bdc1f318..bb6356ea 100644 --- a/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs @@ -140,7 +140,7 @@ protected override void OnSelectFirstTopError(Location location) protected override void OnSelectSingleRowWithoutWhere(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleRowWithoutWhere, location); protected override void OnSelectAggregateAndNonAggregate(Location location) - => OnDiagnostic(DapperAnalyzer.Diagnostics.OnSelectAggregateAndNonAggregate, location); + => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectAggregateMismatch, location); protected override void OnSelectSingleTopError(Location location) => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleTopError, location); diff --git a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs index 7d5042a8..875fa09c 100644 --- a/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs +++ b/src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs @@ -760,7 +760,7 @@ private void ValidateDateArg(ScalarExpression value) { if (!(value is ColumnReferenceExpression col && col.MultiPartIdentifier.Count == 1 && IsAnyCaseInsensitive( - col.MultiPartIdentifier[0].Value, DateTokens ))) + col.MultiPartIdentifier[0].Value, DateTokens))) { parser.OnInvalidDatepartToken(value); } @@ -873,17 +873,45 @@ enum AggregateFlags None = 0, HaveAggregate = 1 << 0, HaveNonAggregate = 1 << 1, - Uncertain = 1 << 2, + Uncertain = 1 << 2, } + private AggregateFlags IsAggregate(ScalarExpression expression) { // any use of an aggregate function contributes HaveAggregate // - there could be unary/binary operations on that aggregate function - // - column references inside the aggregate expression or a sub-query are fine, + // - column references etc inside an aggregate expression, // otherwise they contribute HaveNonAggregate - return AggregateFlags.Uncertain; + switch (expression) + { + case Literal: + return AggregateFlags.None; + case UnaryExpression ue: + return IsAggregate(ue.Expression); + case BinaryExpression be: + return IsAggregate(be.FirstExpression) + | IsAggregate(be.SecondExpression); + case FunctionCall func when IsAggregateFunction(func.FunctionName.Value): + return AggregateFlags.HaveAggregate; // don't need to look inside + case ScalarSubquery sq: + throw new NotSupportedException(); + case IdentityFunctionCall: + case PrimaryExpression: + return AggregateFlags.HaveNonAggregate; + default: + return AggregateFlags.Uncertain; + } + + static bool IsAggregateFunction(string name) + => IsAnyCaseInsensitive(name, AggregateFunctions); } + static readonly string[] AggregateFunctions = [ + "APPROX_COUNT_DISTINCT", "AVG","CHECKSUM_AGG", "COUNT", "COUNT_BIG", + "GROUPING", "GROUPING_ID", "MAX", "MIN", "STDEV", + "STDEVP", "STRING_AGG", "SUM", "VAR", "VARP", + ]; + private bool EnforceTop(ScalarExpression expr) { if (IsInt32(expr, out var i) == TryEvaluateResult.SuccessConstant && i.HasValue) diff --git a/test/Dapper.AOT.Test/Verifiers/DAP231.cs b/test/Dapper.AOT.Test/Verifiers/DAP231.cs index fbaf4f28..21aad486 100644 --- a/test/Dapper.AOT.Test/Verifiers/DAP231.cs +++ b/test/Dapper.AOT.Test/Verifiers/DAP231.cs @@ -37,4 +37,14 @@ public Task SelectSingleRowWithoutFrom() => SqlVerifyAsync(""" select HasRecords = case when exists (select top 1 1 from MyTable) then 1 else 0 end """, SqlParseInputFlags.SingleRow); + [Fact] // false positive https://github.com/DapperLib/DapperAOT/issues/81 + public Task DoNotReportForAggregate() => SqlVerifyAsync(""" + SELECT MAX(SomeColumn) FROM MyTable + """, SqlParseInputFlags.SingleRow); + + [Fact] + public Task DoNotReportForMultipleAggregatesAndExpressions() => SqlVerifyAsync(""" + SELECT -MAX(SomeColumn), COUNT(1) + 32 FROM MyTable + """, SqlParseInputFlags.SingleRow); + } \ No newline at end of file diff --git a/test/Dapper.AOT.Test/Verifiers/DAP244.cs b/test/Dapper.AOT.Test/Verifiers/DAP244.cs new file mode 100644 index 00000000..ac4552fa --- /dev/null +++ b/test/Dapper.AOT.Test/Verifiers/DAP244.cs @@ -0,0 +1,27 @@ +using Dapper.CodeAnalysis; +using System.Threading.Tasks; +using Xunit; +using Diagnostics = Dapper.CodeAnalysis.DapperAnalyzer.Diagnostics; +namespace Dapper.AOT.Test.Verifiers; + +public class DAP244 : Verifier +{ + [Fact] + public Task ValidNonAggregate() => SqlVerifyAsync(""" + select Name + from SomeTable + """); + + [Fact] + public Task ValidAggregate() => SqlVerifyAsync(""" + select COUNT(1), COUNT(DISTINCT Name) + from SomeTable + """); + + [Fact] + public Task InvalidMix() => SqlVerifyAsync(""" + {|#0:select COUNT(1), COUNT(DISTINCT Name), Name + from SomeTable|} + """, Diagnostic(Diagnostics.SelectAggregateMismatch).WithLocation(0)); + +} \ No newline at end of file