Our website uses cookies to enhance your browsing experience.
When it comes to ORMs, most C# developers immediately think of the powerful Entity Framework Core or the lightweight Dapper. What about RepoDB and SqlSugar, though? While they may not be as well-known, these ORMs are actually being used in real-world projects and are still developing. Let's take a look at what issues we can uncover in their source code with the help of a static analyzer.
It's hard to imagine a modern application without an ORM: this technology helps developers by reducing the amount of code they need to write and by eliminating the need to work directly with SQL.
The ORM manages all interactions with the database, including processing read and write queries, transactions, and object-to-SQL mapping. Errors in such tools can lead to serious consequences: from reduced performance to application malfunctions and even security issues.
In the .NET world, Entity Framework Core and Dapper are the industry standards. However, many less popular tools are also used in real-world projects. RepoDB and SqlSugar are among these ORMs.
In this article, we'll look at them through the lens of PVS-Studio static analyzer. Instead of comparing API capabilities or performance, we'll check the projects' source code for potential bugs and suspicious constructs.
Let's move on to discussing the most exciting parts of the SqlSugar code. The source code comes from this commit.
public Task<int> ExecuteCommandAsync(string sql, object parameters,
CancellationToken cancellationToken)
{
this.CancellationToken = CancellationToken;
return ExecuteCommandAsync(sql,parameters);
}The PVS-Studio warning: V3005 The 'this.CancellationToken' variable is assigned to itself. AdoProvider.cs 1472
The CancellationToken auto-property is assigned its own value. Most likely, this property should be assigned the value of the cancellationToken parameter instead.
private static void AppColumns(SqlInfo result,
ISugarQueryable<object> queryable,
string columnName)
{
var selectPkName = queryable.SqlBuilder.GetTranslationColumnName(columnName);
if (result.IsSelectNav)
{
if ( result.SelectString != null
&& !result.SelectString
.ToLower()
.Contains($" {selectPkName.ToLower()}
AS {selectPkName.ToLower()}")) // <=
{
result.SelectString = result.SelectString + ","
+ (selectPkName + " AS " + selectPkName);
}
}
....
}The PVS-Studio warning: V3122 Lowercase string is compared with a different mixed case string. NavigatManager.cs 1131
Let's take a closer look at what's going wrong here. First, the result.SelectString string is converted to lowercase using the ToLower method, and then the resulting string is checked whether it contains $" {selectPkName.ToLower()} AS {selectPkName.ToLower()}". The substring we're looking for contains uppercase letters (AS). As a result, the Contains method will always return false.
To fix the code, we can simply convert all the characters to lowercase:
!result.SelectString
.ToLower()
.Contains($" {selectPkName.ToLower()} as {selectPkName.ToLower()}"))public override string ToSqlString()
{
....
if (it.InsertServerTime || it.InsertSql.HasValue())
{
return GetDbColumn(it,null);
}
object value = null;
if (it.Value is DateTime)
{
....
}
else if ( it.Value is int
|| it.Value is long
|| it.Value is short // <=
|| it.Value is short // <=
|| it.Value is byte
|| it.Value is double)
{
return it.Value;
}
....
}The PVS-Studio warning: V3001 There are identical sub-expressions 'it.Value is short' to the left and to the right of the '||' operator. QuestDBInsertBuilder.cs 89
Using the it.Value is short sub-expression twice is pointless. Perhaps short should be replaced with another type, such as decimal or float.
public static Func<string, object> GetTypeConvert(object value)
{
if ( value is int
|| value is uint
|| value is int?
|| value is uint?)
{
return x => Convert.ToInt32(x);
}
else if ( value is short
|| value is ushort
|| value is short?
|| value is ushort?)
{
return x => Convert.ToInt16(x);
}
else if ( value is long
|| value is long? // <=
|| value is ulong?
|| value is long?) // <=
{
return x => Convert.ToInt64(x);
}
....
}The PVS-Studio warning: V3001 There are identical sub-expressions 'value is long?' to the left and to the right of the '||' operator. UtilMethods.cs 288
The developers used the value is long? condition twice here. Based on the code above, we can assume that the second identical condition should be replaced with value is ulong.
private string GetName(ExpressionParameter parameter,
MemberExpression expression,
bool? isLeft,
bool isSingle)
{
if (isSingle)
{
return GetSingleName(parameter, expression, IsLeft);
}
else
{
return GetMultipleName(parameter, expression, IsLeft);
}
}The PVS-Studio warning: V3196 The 'isLeft' parameter is not utilized inside the method body, but an identifier with a similar name is used inside the same method. MemberExpressionResolve.cs 770
The isLeft parameter isn't used, but the method accesses a property whose name differs from the parameter name only in the capitalization of the first letter.
The developers might have wanted to use isLeft parameter of the GetName method intended of the IsLeft property.
public string GetValue(Expression expression)
{
var numExp = (expression as MethodCallExpression).Arguments[0];
var num =1;
if (ExpressionTool.GetParameters(numExp).Any())
{
var copyContext = this.Context.GetCopyContextWithMapping();
copyContext.IsSingle = false;
copyContext.Resolve(numExp, ResolveExpressType.WhereMultiple);
copyContext.Result.GetString(); // <=
}
else
{
num = ExpressionTool.DynamicInvoke(numExp).ObjToInt();
}
var take = (expression as MethodCallExpression);
if (....)
{
return "TOP " + num;
}
else if (this.Context is OracleExpressionContext)
{
return (HasWhere ? "AND" : "WHERE") + " ROWNUM<=" + num;
}
else if (....)
{
return "limit " + num;
}
else if (this.Context.GetLimit() != null)
{
if (this?.Context?.Case != null)
{
this.Context.Case.HasWhere = this.HasWhere;
this.Context.Case.Num = num;
}
return this.Context.GetLimit();
}
else
{
return "limit " + num;
}
}The PVS-Studio warning: V3010 The return value of function 'GetString' is required to be utilized. SubTake.cs 64
The analyzer reports that the return value of the GetString method isn't used. At the same time, the GetString implementation reveals that the method only builds and returns a string representation of _Result. It doesn't modify the object's state or perform any actions whose results are used later in the code:
public string GetString()
{
if (_Result == null) return null;
if (IsUpper)
return
_Result.ToString()
.ToUpper()
.Replace(UtilConstants.ReplaceCommaKey,",")
.TrimEnd(',');
else
return _Result.ToString()
.Replace(UtilConstants.ReplaceCommaKey, ",")
.TrimEnd(',');
}So, the result of the GetString call is discarded in the current code. The developers probably forgot to use the return value when calculating num.
The surrounding code also supports this assumption: every return from the method uses the num variable. Following this logic, both branches of the first condition should calculate the same value and assign it to the num variable.
The correct code may look like this:
copyContext.Resolve(numExp, ResolveExpressType.WhereMultiple);
num = copyContext.Result.GetString();Now let's look at some errors and issues in the RepoDB project. The source code comes from this commit.
public static object GetValue(this ConditionalExpression expression)
{
var test = expression.Test.GetValue();
var trueValue = expression.IfTrue.GetValue();
if (expression.Test.NodeType == ExpressionType.Equal)
{
return test == trueValue ? trueValue : expression.IfFalse.GetValue();
}
else if (expression.Test.NodeType == ExpressionType.NotEqual)
{
return test != trueValue ? trueValue : expression.IfFalse.GetValue();
}
else if (expression.Test.NodeType > ExpressionType.GreaterThan)
{
....
}
else if (expression.Test.NodeType > ExpressionType.GreaterThanOrEqual)// <=
{
....
}
else if (expression.Test.NodeType > ExpressionType.LessThan) // <=
{
....
}
else if (expression.Test.NodeType > ExpressionType.LessThanOrEqual) // <=
{
....
}
throw new NotSupportedException(....);
}The PVS-Studio warnings:
In the GetValue method, expression.Test.NodeType is successively compared against different values from the ExpressionType enumeration. However, the last four checks use the > operator instead of the equality operator.
The else if chain executes sequentially, and the enumeration values in the lower branches are greater than those in the branches above them.
public enum ExpressionType
{
....
GreaterThan = 15,
GreaterThanOrEqual = 16,
....
LessThan = 20,
LessThanOrEqual = 21,
....
}The expression.Test.NodeType > ExpressionType.GreaterThan condition covers all the conditions that follow.
So, if expression.Test.NodeType > ExpressionType.GreaterThan evaluates to true, the subsequent conditions aren't checked. If it evaluates to false, the following conditions are false.
public override int GetHashCode()
{
// Make sure to return if it is already provided
if (this.hashCode != null)
{
return this.hashCode.Value;
}
// Get first the entity hash code
var hashCode = HashCode.Combine(base.GetHashCode(), Name, ".UpdateAll");
// Get the fields
if (Fields != null)
{
foreach (var field in Fields) // <=
{
hashCode = HashCode.Combine(hashCode, field);
}
}
// Get the qualifier <see cref="Field"/> objects
if (Fields != null) // <=
{
foreach (var field in Qualifiers)
{
hashCode = HashCode.Combine(hashCode, field);
}
}
....
}The PVS-Studio warning: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'Qualifiers' variable should be used instead of 'Fields'. UpdateAllRequest.cs 124
Fields is checked for null twice. Most likely, the second check should use Qualifiers instead of Fields, since this collection is used in foreach.
public static Task<int> UpdateAllAsync<TEntity>(....
Expression<Func<TEntity, object>> qualifiers,
....,
IEnumerable<Field> fields = null,
....)
where TEntity : class
{
return UpdateAllAsyncInternal<TEntity>(connection: connection,
tableName: tableName,
entities: entities,
qualifiers: fields, // <=
batchSize: batchSize,
fields: fields, // <=
hints: hints,
commandTimeout: commandTimeout,
traceKey: traceKey,
transaction: transaction,
trace: trace,
statementBuilder: statementBuilder,
cancellationToken: cancellationToken);
}The PVS-Studio warning: V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. UpdateAll.cs 619
Here, the fields parameter is passed to the UpdateAllAsyncInternal method twice, while the qualifiers parameter isn't used at all.
The developers might have wanted to pass qualifiers as follows:
qualifiers: Field.Parse<TEntity>(qualifiers)public override string CreateBatchQuery(string tableName,
IEnumerable<Field> fields,
int page,
int rowsPerBatch,
IEnumerable<OrderField> orderBy = null,
QueryGroup where = null,
string hints = null)
{
// Ensure with guards
GuardTableName(tableName);
// Validate the hints
GuardHints(hints);
// There should be fields
if (fields?.Any() != true)
{
throw new MissingFieldsException(fields?.Select(f => f.Name));
}
....
}The PVS-Studio warning: V3191 Iteration through the 'fields' collection makes no sense because it is always empty. SqlServerStatementBuilder.cs 70
The fields?.Any() != true condition is true when the fields collection is empty or null. In that case, fields?.Select(f => f.Name) serves no purpose, as it will always return an empty collection or null.
public override Guid GetGuid(int i)
{
ThrowExceptionIfNotAvailable();
return Guid.Parse(GetValue(i)?.ToString());
}The PVS-Studio warning: V3105 The result of null-conditional operator is passed as the first argument to the 'Parse' method and is not expected to be null. DataEntityDataReader.cs 405
Here, GetValue(i) may return null, which will be passed to the Guid.Parse method because of the ? operator. And Guid.Parse will throw an ArgumentNullException when passed the null argument.
public static void AssertMembersEquality(
object obj,
IDictionary<string, object> dictionary)
{
....
var value1 = property.GetValue(obj);
var value2 = dictionary[property.Name];
if (value1 is byte[] b1 && value2 is byte[] b2)
{
....
else
{
var propertyType = property.PropertyType.GetUnderlyingType();
if (propertyType == typeof(TimeSpan) && value2 is DateTime dateTime)
{
value2 = dateTime.TimeOfDay;
}
else if (propertyType == typeof(string) && value2 is DateTime)
{
value1 = DateTime.Parse(value1?.ToString()); // <=
}
....
}The PVS-Studio warning: V3105 The result of null-conditional operator is passed as the first argument to the 'Parse' method and is not expected to be null. Helper.cs 157
Again, if value1 is null, the ArgumentNullException will be thrown.
Despite the substantial amount of code and extensive functionality of the projects, the analysis revealed only a few suspicious code fragments. This suggests that both projects are well written and maintained at a high standard.
Still, perfect code doesn't exist. Even mature and popular libraries can contain copy-paste errors, forgotten parameters, incorrect conditions, and other minor issues that are easy to overlook during development and testing. This is where static analysis is particularly useful: it can highlight potential issues before they become real problems.
If you'd like to analyze your own project with PVS-Studio, you can try it via this link.
Take care of yourself and your code!
0
0
0
0