Skip to content

Commit

Permalink
spot more problems
Browse files Browse the repository at this point in the history
  • Loading branch information
mgravell committed Sep 15, 2023
1 parent bad0efd commit e76a10c
Show file tree
Hide file tree
Showing 11 changed files with 590 additions and 138 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,8 @@ static string BuildParameterMap(in GeneratorSyntaxContext ctx, IInvocationOperat
SqlAnalysis.TSqlProcessor.ModeFlags modeFlags = SqlAnalysis.TSqlProcessor.ModeFlags.None;
if (caseSensitive) modeFlags |= SqlAnalysis.TSqlProcessor.ModeFlags.CaseSensitive;
if ((flags & OperationFlags.BindResultsByName) != 0) modeFlags |= SqlAnalysis.TSqlProcessor.ModeFlags.ValidateSelectNames;
if ((flags & OperationFlags.SingleRow) != 0) modeFlags |= SqlAnalysis.TSqlProcessor.ModeFlags.SingleRow;
if ((flags & OperationFlags.AtMostOne) != 0) modeFlags |= SqlAnalysis.TSqlProcessor.ModeFlags.AtMostOne;

var proc = new DiagnosticTSqlProcessor(parameterType, modeFlags, diagnostics, loc, sqlSyntax);
try
Expand Down
19 changes: 16 additions & 3 deletions src/Dapper.AOT.Analyzers/CodeAnalysis/Diagnostics.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
using Microsoft.CodeAnalysis;
using System.Collections.Generic;
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -144,8 +144,21 @@ internal static readonly DiagnosticDescriptor
"SELECT statement assigns variable and performs reads (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true),
DeleteWithoutWhere = new("DAP223", "DELETE without WHERE",
"DELETE statement lacks WHERE clause (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true),
UpdateWithoutWhere = new ("DAP224", "UPDATE without WHERE",
"UPDATE statement lacks WHERE clause (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true);
UpdateWithoutWhere = new("DAP224", "UPDATE without WHERE",
"UPDATE statement lacks WHERE clause (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true),

FromMultiTableMissingAlias = new("DAP225", "UPDATE without WHERE",
"FROM expressions with multiple elements should use aliases (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true),
NonIntegerTop = new("DAP226", "UPDATE without WHERE",
"TOP literals should be integers (L{0} C{1})", Category.Sql, DiagnosticSeverity.Error, true),
NonPositiveTop = new("DAP227", "UPDATE without WHERE",
"TOP literals should be positive (L{0} C{1})", Category.Sql, DiagnosticSeverity.Error, true),
SelectFirstTopError = new("DAP228", "UPDATE without WHERE",
"SELECT for First* should use TOP 1 (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true),
SelectSingleTopError = new("DAP229", "UPDATE without WHERE",
"SELECT for Single* should use TOP 2; if you do not need to test over-read, use First* (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true),
SelectSingleRowWithoutWhere = new("DAP230", "UPDATE without WHERE",
"SELECT for single row without WHERE or (TOP and ORDER BY) (L{0} C{1})", Category.Sql, DiagnosticSeverity.Warning, true);


// be careful moving this because of static field initialization order
Expand Down
21 changes: 18 additions & 3 deletions src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.SqlServer.TransactSql.ScriptDom;
using System;
using System.Data;
using System.Diagnostics;
using static Dapper.SqlAnalysis.TSqlProcessor;

namespace Dapper.Internal;
Expand Down Expand Up @@ -46,14 +47,15 @@ public DiagnosticTSqlProcessor(ITypeSymbol? parameterType, ModeFlags flags, obje
return _location;
}



private void AddDiagnostic(DiagnosticDescriptor diagnostic, in Location location, params object?[]? args)
{
Diagnostics.Add(ref _diagnostics, Diagnostic.Create(diagnostic, GetLocation(location), args));
}
protected override void OnError(string error, in Location location)
=> AddDiagnostic(Diagnostics.SqlError, location, error, location.Line, location.Column);
{
Debug.Fail("unhandled error: " + error);
AddDiagnostic(Diagnostics.SqlError, location, error, location.Line, location.Column);
}

protected override void OnAdditionalBatch(Location location)
=> AddDiagnostic(Diagnostics.MultipleBatches, location, location.Line, location.Column);
Expand Down Expand Up @@ -115,6 +117,19 @@ protected override void OnUpdateWithoutWhere(Location location)
protected override void OnDeleteWithoutWhere(Location location)
=> AddDiagnostic(Diagnostics.DeleteWithoutWhere, location, location.Line, location.Column);

protected override void OnFromMultiTableMissingAlias(Location location)
=> AddDiagnostic(Diagnostics.FromMultiTableMissingAlias, location, location.Line, location.Column);
protected override void OnNonIntegerTop(Location location)
=> AddDiagnostic(Diagnostics.NonIntegerTop, location, location.Line, location.Column);
protected override void OnNonPositiveTop(Location location)
=> AddDiagnostic(Diagnostics.NonPositiveTop, location, location.Line, location.Column);
protected override void OnSelectFirstTopError(Location location)
=> AddDiagnostic(Diagnostics.SelectFirstTopError, location, location.Line, location.Column);
protected override void OnSelectSingleRowWithoutWhere(Location location)
=> AddDiagnostic(Diagnostics.SelectSingleRowWithoutWhere, location, location.Line, location.Column);
protected override void OnSelectSingleTopError(Location location)
=> AddDiagnostic(Diagnostics.SelectSingleTopError, location, location.Line, location.Column);

protected override bool TryGetParameter(string name, out ParameterDirection direction)
{
// we have knowledge of the type system; use it
Expand Down
123 changes: 117 additions & 6 deletions src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Data;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Text.RegularExpressions;
Expand All @@ -20,6 +21,8 @@ internal enum ModeFlags
None = 0,
CaseSensitive = 1 << 0,
ValidateSelectNames = 1 << 1,
SingleRow = 1 << 2,
AtMostOne = 1 << 3,
}
[Flags]
internal enum VariableFlags
Expand Down Expand Up @@ -184,6 +187,19 @@ protected virtual void OnDeleteWithoutWhere(Location location)
protected virtual void OnUpdateWithoutWhere(Location location)
=> OnError($"UPDATE statement without WHERE clause", location);

protected virtual void OnSelectSingleTopError(Location location)
=> OnError($"SELECT for Single* should use TOP 2; if you do not need to test over-read, use First*", location);
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 OnNonPositiveTop(Location location)
=> OnError($"TOP literals should be positive", location);
protected virtual void OnNonIntegerTop(Location location)
=> OnError($"TOP literals should be integers", location);
protected virtual void OnFromMultiTableMissingAlias(Location location)
=> OnError($"FROM expressions with multiple elements should use aliases", location);


internal readonly struct Location
{
Expand Down Expand Up @@ -327,6 +343,8 @@ public VariableTrackingVisitor(ModeFlags flags, TSqlProcessor parser)

public bool CaseSensitive => (_flags & ModeFlags.CaseSensitive) != 0;
public bool ValidateSelectNames => (_flags & ModeFlags.ValidateSelectNames) != 0;
public bool SingleRow => (_flags & ModeFlags.SingleRow) != 0;
public bool AtMostOne => (_flags & ModeFlags.AtMostOne) != 0;

private readonly Dictionary<string, Variable> variables;
private int batchCount;
Expand Down Expand Up @@ -505,17 +523,18 @@ public override void Visit(ExecuteStatement node)
base.Visit(node);
}

private void AddQuery()
private bool AddQuery() // returns true if the first
{
switch (parser.Flags & (ParseFlags.Query | ParseFlags.Queries))
{
case ParseFlags.None:
parser.Flags |= ParseFlags.Query;
break;
return true;
case ParseFlags.Query:
parser.Flags |= ParseFlags.Queries;
break;
}
return false;
}
public override void Visit(SelectStatement node)
{
Expand Down Expand Up @@ -548,7 +567,7 @@ public override void Visit(SelectStatement node)
if (name is null && scalar.Expression is ColumnReferenceExpression col)
{
var ids = col.MultiPartIdentifier.Identifiers;
if(ids.Count != 0)
if (ids.Count != 0)
{
name = ids[ids.Count - 1].Value;
}
Expand All @@ -572,27 +591,119 @@ public override void Visit(SelectStatement node)
}
if (reads != 0)
{
AddQuery();
if (sets != 0)
{
parser.OnSelectAssignAndRead(new(spec));
}
bool firstQuery = AddQuery();
if (firstQuery && SingleRow)
{
bool haveTop = false;
if (spec.TopRowFilter is { Percent: false, Expression: IntegerLiteral top }
&& ParseInt32(top, out int i))
{
haveTop = true;
if (AtMostOne) // Single[OrDefault][Async]
{
if (i != 2)
{
parser.OnSelectSingleTopError(new(node));
}
}
else // First[OrDefault][Async]
{
if (i != 1)
{
parser.OnSelectFirstTopError(new(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 (haveTop && spec.OrderByClause is not null) { } // fine
else
{
parser.OnSelectSingleRowWithoutWhere(new(node));
}
}

}

}

base.Visit(node);
}

static bool ParseInt32(IntegerLiteral node, out int value) => int.TryParse(node.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out value);
static bool ParseNumeric(NumericLiteral node, out decimal value) => decimal.TryParse(node.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out value);
public override void Visit(TopRowFilter node)
{
if (node.Expression is IntegerLiteral iLit && ParseInt32(iLit, out var i) && i <= 0)
{
parser.OnNonPositiveTop(new(node));
}
else if (node.Expression is NumericLiteral nLit)
{
if (!node.Percent)
{
parser.OnNonIntegerTop(new(node));
}
if (ParseNumeric(nLit, out var n) && n <= 0)
{ // don't expect to see this; parser rejects them
parser.OnNonPositiveTop(new(node));
}
}
base.Visit(node);
}

public override void Visit(OutputClause node)
{
AddQuery(); // works like a query
base.Visit(node);
}

private bool IsUnfiltered(FromClause from, WhereClause where)
{
if (where is not null) return false;
return !IsMultiTable(from); // treat multi-table as filtered
}
private bool IsMultiTable(FromClause? from)
{
var tables = from?.TableReferences;
// treat multiple tables as filtered (let's not discuss outer joins etc)
if (tables is null || tables.Count == 0) return false;
return tables.Count > 1 || tables[0] is JoinTableReference;
}

private bool _fromDemandAlias;
public override void ExplicitVisit(FromClause node)
{
bool oldDemandAlias = _fromDemandAlias;
try
{
// set ambient state so we can complain more as we walk the nodes
_fromDemandAlias = IsMultiTable(node);
base.ExplicitVisit(node);
}
finally
{
_fromDemandAlias = oldDemandAlias;
}
}
public override void Visit(TableReferenceWithAlias node)
{
if (_fromDemandAlias && string.IsNullOrWhiteSpace(node.Alias?.Value))
{
parser.OnFromMultiTableMissingAlias(new(node));
}
base.Visit(node);
}

public override void Visit(DeleteSpecification node)
{
if (node.WhereClause is null)
if (IsUnfiltered(node.FromClause, node.WhereClause))
{
parser.OnDeleteWithoutWhere(new(node));
}
Expand All @@ -601,7 +712,7 @@ public override void Visit(DeleteSpecification node)

public override void Visit(UpdateSpecification node)
{
if (node.WhereClause is null)
if (IsUnfiltered(node.FromClause, node.WhereClause))
{
parser.OnUpdateWithoutWhere(new(node));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
Generator produced 5 diagnostics:
Generator produced 6 diagnostics:

Hidden DAP000 L1 C1
Dapper.AOT handled 11 of 11 enabled call-sites using 7 interceptors, 1 commands and 1 readers

Warning DAP025 Interceptors/QueryDetection.input.cs L12 C12
The command has a query that will be ignored

Warning DAP230 Interceptors/QueryDetection.input.cs L17 C35
SELECT for single row without WHERE or (TOP and ORDER BY) (L1 C1)

Warning DAP219 Interceptors/QueryDetection.input.cs L17 C42
SELECT columns should be specified explicitly (L1 C8)

Expand Down
5 changes: 4 additions & 1 deletion test/Dapper.AOT.Test/Interceptors/QueryDetection.output.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
Generator produced 5 diagnostics:
Generator produced 6 diagnostics:

Hidden DAP000 L1 C1
Dapper.AOT handled 11 of 11 enabled call-sites using 7 interceptors, 1 commands and 1 readers

Warning DAP025 Interceptors/QueryDetection.input.cs L12 C12
The command has a query that will be ignored

Warning DAP230 Interceptors/QueryDetection.input.cs L17 C35
SELECT for single row without WHERE or (TOP and ORDER BY) (L1 C1)

Warning DAP219 Interceptors/QueryDetection.input.cs L17 C42
SELECT columns should be specified explicitly (L1 C8)

Expand Down
53 changes: 53 additions & 0 deletions test/Dapper.AOT.Test/Interceptors/TsqlTips.input.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,47 @@
[module: DapperAot]
public class Foo
{
public void TopChecks(System.Data.SqlClient.SqlConnection connection)
{
// fine
connection.Query("select A, B from SomeTable");
connection.Query("select TOP 10 A, B from SomeTable");
// non-positive top
connection.Query("select TOP 0 A, B from SomeTable");
// non integer top
connection.Query("select TOP 5.0 A, B from SomeTable");
// non integer percent top (fine)
connection.Query("select TOP 5.0 PERCENT A, B from SomeTable");
connection.Query("select TOP 5.1 PERCENT A, B from SomeTable");
// non integer negative percent top (not fine)
connection.Query("select TOP -5.1 PERCENT A, B from SomeTable");

// first/single with where: fine
connection.QueryFirst("select A, B from SomeTable where Id=42");
connection.QuerySingle("select A, B from SomeTable where Id=42");
// first/single with top and order-by: fine
connection.QueryFirst("select top 1 A, B from SomeTable order by B");
connection.QuerySingle("select top 2 A, B from SomeTable order by B");
// first/single with wrong top
connection.QueryFirst("select top 2 A, B from SomeTable order by B");
connection.QuerySingle("select top 1 A, B from SomeTable order by B");
// first/single with nothing
connection.QueryFirst("select A, B from SomeTable");
connection.QuerySingle("select A, B from SomeTable");
// first/single with order-by but no top, or top but no order-by
connection.QueryFirst("select A, B from SomeTable order by B");
connection.QuerySingle("select A, B from SomeTable order by B");
connection.QueryFirst("select top 1 A, B from SomeTable");
connection.QuerySingle("select top 2 A, B from SomeTable");

// first/single with join; we'll allow join to function as a filter
connection.QueryFirst("select A, B from SomeTable x inner join Foo y on x.Id = y.Id");

// check for aliases
connection.QueryFirst("select A, B from SomeTable x inner join Foo on x.Id = FromSomewhere");

}

public void Things(System.Data.SqlClient.SqlConnection connection)
{
var args = new { a = 123, b = "abc" };
Expand Down Expand Up @@ -104,10 +145,22 @@ update SomeTable
// delete/update with filter: fine
connection.Execute("delete from Customers where Id=12");
connection.Execute("update Customers set Name='New name' where Id=12");
// allow join to function like filter
connection.Execute("delete c from Customers c inner join Foo y on y.Id = c.Id");

// delete/update without filter: warn
connection.Execute("delete from Customers");
connection.Execute("update Customers set Name='New name'");
// allow join to function like filter
connection.Execute("update c set c.Name='New name' from Customers c inner join Foo y on y.Id = c.Id");

connection.QuerySingle<Customer>("select Id from Customers where Id=42");
connection.QuerySingle<Customer>("select top 1 Id from Customers");
connection.QuerySingle<Customer>("select top 2 Id from Customers");
// > 2 is bad for Single etc (2 makes sense to check for dups)
connection.QuerySingle<Customer>("select top 3 Id from Customers");
// no where
connection.QuerySingle<Customer>("select Id from Customers");
}

public class Customer
Expand Down
Loading

0 comments on commit e76a10c

Please sign in to comment.