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

Enhance nullability check for "==" and "!=" operators for LINQ provider #1996

Merged
merged 11 commits into from
Feb 27, 2020

Conversation

maca88
Copy link
Contributor

@maca88 maca88 commented Jan 27, 2019

This PR adds a slightly complex logic that determines if it should add a OR <Property> IS NULL check for == and != operators, fixes #1860. The logic will not add the additional null check for the following scenarios:

  • The property is a value type.
    Example: s.Query<Order>().Where(o => o.Number != 5)
  • The property is mapped as not null.
    Example: s.Query<Order>().Where(o => o.Status != "NEW")
  • The property has a not null check before or after the != operator.
    Example: s.Query<Order>().Where(o => o.Status != null && o.Status != "NEW")
  • The nullable property has a HasValue check before or after the != operator.
    Example: s.Query<Order>().Where(o => o.Number.HasValue && o.Number != 3)
  • The expression for the != operator is not null.
    Example: s.Query<Order>().Where(o => (o.NotNullProperty + o.NotNullProperty2) != "TEST")
  • Two nullable properties are compared with == operator and one of them or both have a not null check.
    Example: s.Query<Order>().Where(o => o.NullProperty != null && o.NullProperty == o.NullProperty2)

There are some scenarios where the logic will add a null check where currently it is not added:

  • Using a subquery with an aggregate method that can return null when the subquery has no records.
    Example: s.Query<Order>().Where(o => o.Rows.Max(r => r.Number) != 0)
  • Using a property that is translated to a sql function on a property that can be null.
    Example: s.Query<Order>().Where(o => o.Status.Length != 3)

In addition, the Equals method was modified to use the same logic as the == operator.

@fredericDelaporte fredericDelaporte added this to the 5.3 milestone Jan 29, 2019
@@ -260,13 +265,7 @@ public async Task NullEqualityWithNotNullAsync()
q = session.Query<AnotherEntityRequired>().Where(o => (o.NullableOutput + o.Output) == o.Output);
await (ExpectAsync(q, Does.Not.Contain("is null").IgnoreCase));

q = session.Query<AnotherEntityRequired>().Where(o => (o.Input + o.Output) == o.Output);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to o.Output + o.Output as otherwise it fails in oracle as in oracle concatenating null with a non empty string will not result in a null value like in the other databases. I think that we should add an additional logic when concatenating strings in order to have a consistent behaviour across various databases and possible align with Linq-to-Object so that null + "test" will not result in a null value.

q = session.Query<AnotherEntityRequired>().Where(o => o.RelatedItems.Max(r => r.Id) != 0);
await (ExpectAllAsync(q, Does.Contain("is null").IgnoreCase));

q = session.Query<AnotherEntityRequired>().Where(o => o.RelatedItems.Take(10).All(r => r.Output != null) != o.NullableBool);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the Take call as it produces an invalid sql statement for Oracle.

</id>
<property name="Output" not-null="true" />
<property name="Input" not-null="true" />
<property name="NullableOutput" formula="Output" lazy="true" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've had to use lazy as otherwise an invalid query is generated due to the missing table alias for the column used in the formula.

@maca88 maca88 changed the title Enhance nullability check for "==" and "!=" operators for LINQ provider WIP - Enhance nullability check for "==" and "!=" operators for LINQ provider Apr 7, 2019
MapPropertyToIndex(null, prop, i);
}

private void MapPropertyToIndex(string path, Mapping.Property prop, int i)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from #2079. It is a fix for including nested components.

<property name="InvalidLoginAttempts" type="Int32" />
<property name="RegisteredAt" type="DateTime" />
<property name="LastLoginDate" type="DateTime" />

<many-to-one name="CreatedBy" class="User" not-null="true" lazy="false">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add lazy="false", otherwise some tests in QueryReuseTests (CanReuseAfterFirst, CanReuseAfterFirstOrDefault, CanReuseAfterSingle, CanReuseAfterSingleOfDefault) would fail.

@maca88 maca88 changed the title WIP - Enhance nullability check for "==" and "!=" operators for LINQ provider Enhance nullability check for "==" and "!=" operators for LINQ provider Apr 10, 2019
@maca88
Copy link
Contributor Author

maca88 commented Oct 5, 2019

Rebased and fixed TryGetEntityName method to work with custom entity names. I am not sure what to do with the current CodeFactor issues, I think that we should allow those issues for the test project and increase the allowed complexity a little bit :)

@hazzik
Copy link
Member

hazzik commented Oct 5, 2019

I am not sure what to do with the current CodeFactor issues, I think that we should allow those issues for the test project and increase the allowed complexity a little bit :)

I will agree with CodeFactor that this method is too complex (and too hard for me to understand).

@maca88
Copy link
Contributor Author

maca88 commented Oct 5, 2019

Ok, I will try to make the method more readable by adding documentation, split into multiple methods, try to simplify the logic and add isolated tests.

@maca88
Copy link
Contributor Author

maca88 commented Oct 8, 2019

I tried my best to make the method more readable and to reduce its complexity in #2036. I've added several isolated tests in order to cover all kind of mappings. I think that the method is now more readable but the complexity didn't dropped, it even increased so I am not sure what to do about it. Should I split it into more smaller methods or replace the method with a method object?

EDIT:
By splitting the method into two, solved the CodeFactor issue.

@maca88
Copy link
Contributor Author

maca88 commented Feb 21, 2020

Rebased and updated code to use ExpressionHelper.TryGetMappedNullability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LINQ "==" operator generates OR with IS NULL
3 participants