# 5 amusing oddities in the code of Entity Framework Core
Entity Framework Core is developed by professionals who avoid mistakes thanks to their experience, code reviews, and a powerful testing system\. However, even such a project is not without its gags\. You are about to read an article on the bizarre snippets of code that top developers have overlooked\.
## Brief introduction
Usually we write articles in which we analyze different bugs that we have found in open\-source projects with the help of PVS\-Studio\. However, this is not such a case\. The code in the project is really well written :\)\. So, when I said about developer's professionalism, it wasn't sarcasm\.
This is a case where I've actually checked the project, but I can't write a classic article about analyzing bugs\. There are some curious anomalies that I'll tell you about\. Overall, however, the code is of high quality and looks very clean\. The analyzer issues almost no warnings that indicate obvious issues\.
I used the PVS\-Studio 7\.26 analyzer to search for oddities and potential errors\. The version of Entity Framework Core is [7\.0\.10](https://github.com/dotnet/efcore/releases/tag/v7.0.10)\. The top is entirely subjective – if you disagree, feel free to comment, and we'll discuss\.
## Fifth place\. Inconsistency
```cpp
protected virtual ModelBuilder VisitDatabaseModel(....)
{
....
if (!string.IsNullOrEmpty(databaseModel.DatabaseName)) // <=
{
modelBuilder.Model.SetDatabaseName(
!_options.UseDatabaseNames
&& !string.IsNullOrEmpty(databaseModel.DatabaseName) // <=
? _candidateNamingService.GenerateCandidateIdentifier(....)
: databaseModel.DatabaseName
);
}
....
}
```
The first issue from the top immediately reflects the idea behind most of the weirdness I found in Entity Framework Core: there is a surprising amount of inconsistency in the code, especially between fragments that are literally right next to each other\.
First, the code is checked to see whether _databaseModel\.databaseName_ is an empty string or _null_\. If there is a perfectly normal string value, the code decides to check the property value again\. Why? What was the developer trying to do?
The strangest thing is that these checks are only 3 lines apart\! How's that?
For those who choose to count the lines
In the example I gave, the gap is 4 lines \(oh yes, that makes a big difference :D\)\. However, this is only because the code had to be slightly formatted for the article\. Originally, the _\!\_options\.UseDatabaseNames_ calls and the extra _DatabaseName_ check are written in the same line\.
I found this fragment with the help of the following analyzer warning:
[V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: \!string\.IsNullOrEmpty\(databaseModel\.DatabaseName\)\.
## Fourth place\. There is no model, take a schema from it
```cpp
protected virtual IEnumerable
GenerateModificationCommands(InsertDataOperation operation,
IModel? model)
{
....
if ( operation.ColumnTypes != null
&& operation.Columns.Length != operation.ColumnTypes.Length)
{
throw new InvalidOperationException(
RelationalStrings.InsertDataOperationTypesCountMismatch(
....,
FormatTable(operation.Table,
operation.Schema ?? model?.GetDefaultSchema())));
}
if (operation.ColumnTypes == null && model == null)
{
throw new InvalidOperationException(
RelationalStrings.InsertDataOperationNoModel(
FormatTable(operation.Table,
operation.Schema ?? model?.GetDefaultSchema())));
}
....
}
```
Another great example of inconsistency and a lack of thought when writing the code\. Indeed, who cares what the exception is even about when you can just copy and paste :\)\. The check makes sure that _model_ is _null_, then we throw an exception that there is **no model**\. Why not refer to the model when making the exception? :\)
Wait, it uses the '?\\\.' operator\\\. Then it's okay\\\!
It's not that referring to the model will cause an exception to be thrown here\. The point is that the programmer \(I guess so\) wrote this code without thinking about its meaning\. Considering that this is one of the most well\-known and widely\-used \.NET libraries, such an approach to development is troubling\.
Another theory is that there may be an error in the check and '\|\|' should have been used instead of '&&'\. Although, that's just speculation\.
I found the oddity described here thanks to the [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) diagnostic rule:
Expression 'model' is always null\. The operator '?\.' is meaningless\.
## Third place\. Double maximum
```cpp
public virtual SqlExpression? Translate(....)
{
if (SupportedMethods.TryGetValue(method, out var sqlFunctionName))
{
RelationalTypeMapping? typeMapping;
List? newArguments = null;
if (sqlFunctionName == "max" || sqlFunctionName == "max") // <=
{
typeMapping = ExpressionExtensions.InferTypeMapping(arguments[0],
arguments[1]);
newArguments = new List
{
....
};
}
else
{
typeMapping = arguments[0].TypeMapping;
}
var finalArguments = newArguments ?? arguments;
return _sqlExpressionFactory.Function(....);
}
return null;
}
```
This is where things can get dangerous\. It looks like some special handling is implemented for the _max_ function\. Well, the developers apparently wanted to do something similar for another function\. I think it might be for the _min_ function, but it's hard to say for sure\.
Unfortunately, the code looks like it's asking again: "Maybe the function name is max after all?" \(if the first comparison returns _false_\)\. Well, what if something has really changed? :\)
PVS\-Studio, which was used to search for this oddity, gives the following comment:
[V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'sqlFunctionName \=\= "max"' to the left and to the right of the '\|\|' operator\.
## Second place\. Suspicious success
```cpp
public override Expression Visit(Expression expression)
{
....
if ( TryGetPartitionKeyProperty(entityType,
out var partitionKeyProperty)
&& entityTypePrimaryKeyProperties.SequenceEqual(queryProperties)
&& (partitionKeyProperty == null || ....)
&& ....)
{
var propertyParameterList = queryProperties.Zip(....);
var readItemExpression = new ReadItemExpression(entityType,
propertyParameterList);
return ....;
}
....
}
```
Take a closer look at the code above\. Can you see anything weird there?
Well, I can't see it either :\)\. It may not really be an error at all, but it's a curious case nonetheless\.
The call to the _TryGetPartitionKeyProperty_ function is strange here\. Theoretically, it works in the usual Try\-methods pattern: the function returns _true_ if the operation was successful, otherwise it returns _false_\.
If you thought the same thing I did, congratulations — the code fooled you\. The _TryGetPartitionKeyProperty_ method looks as follows:
```cpp
static bool TryGetPartitionKeyProperty(IEntityType entityType,
out IProperty partitionKeyProperty)
{
var partitionKeyPropertyName = entityType.GetPartitionKeyPropertyName();
if (partitionKeyPropertyName is null)
{
partitionKeyProperty = null;
return true;
}
partitionKeyProperty = entityType.FindProperty(partitionKeyPropertyName);
return true;
}
```
Well yes, functions written by successful developers always return success :\)\.
The strange thing here is the following: if the value of _partitionKeyPropertyName_ is not received, _null _is explicitly written to the out\-parameter\. The _TryGetPartitionKeyProperty_ method is still considered successful \(which sounds strange when you read it\)\. There are two ways of looking at this point\.
On the one hand, there is nothing to worry about\. The code that calls the method is ready for _null_ to be written to the _out_ parameter:
```cpp
if ( TryGetPartitionKeyProperty(entityType,
out var partitionKeyProperty)
&& entityTypePrimaryKeyProperties.SequenceEqual(queryProperties)
&& (partitionKeyProperty == null || ....)
&& ....)
```
The developers could rewrite the method, so that it would only return _IProperty_ \(without any Try, out\-parameters, and also without returning a boolean value\)\. Although it doesn't seem like a big deal\.
On the other hand, it's not entirely clear what exactly this _null_ check is doing here\. For example, it could be added in case _null_ returns a call to _entityType\.FindProperty\(partitionKeyPropertyName\)_\.
I have a feeling that something is wrong here\. It's very strange that the _TryGetPartitionKeyProperty_ method returns _true_ \(as if saying: "Yes, we successfully got everything\!"\) but at the same time sets the corresponding out\-parameter to _null_\.
I found this strange code snippet thanks to the PVS\-Studio warning:
[V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: TryGetPartitionKeyProperty\(entityType, out var partitionKeyProperty\)\.
## First place\. Who needs this ef\.exe anyway?
```cpp
protected override int Execute(string[] _)
{
....
var targetPlatformIdentifier = startupProject.TargetPlatformIdentifier!;
if ( targetPlatformIdentifier.Length != 0
&& !string.Equals(targetPlatformIdentifier, "Windows", ....))
{
executable = Path.Combine(
toolsPath,
"net461",
startupProject.PlatformTarget switch
{
"x86" => "win-x86",
"ARM64" => "win-arm64",
_ => "any"
},
"ef.exe");
}
executable = "dotnet";
args.Add("exec");
args.Add("--depsfile");
args.Add(depsFile);
....
return Exe.Run(executable, args, startupProject.ProjectDir);
}
```
I'd say this code fragment amused me more than the others\. First, there is some serious value check, followed by the equally serious path building functionality that uses a complex _Path\.Combine_ call\. And then all the results are simply wiped away :\)\. All that's left is 'dotnet'\.
I don't mind, but it looks weird\. Even if there is no error here, we still have some big inconsistencies of logic\. Looks like the developers inserted random lines into the code without looking at what is happening nearby\. Perhaps this is the result of an unsuccessful branch merging\.
I found this snippet thanks to a message from the [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) diagnostic rule:
The 'executable' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 139, 127\.
## Mini conclusion
Despite all I've said, it's worth acknowledging that Entity Framework is indeed written by real professionals\. Almost none of the presented warnings look like real bugs\. And believe me, I tried very hard to find the best examples\.
Of course, you could say that the analyzer just didn't do a good job here\. However, it does find quite a few obvious issues in other projects\. By the way, if you're interested in giving this tool a try, click the [link](https://pvs-studio.com/en/pvs-studio/try-free/) to get it for free\.
Well, that's it for me\. I hope you've enjoyed reading this :\)\.