Additional filter operations: isnull and isnotnull

[NullVoxPopuli]

This superscedes: https://github.com/json-api-dotnet/JsonApiDotNetCore/pull/386/files
Closes #{ISSUE_NUMBER}

FEATURE

• write tests that address the requirements outlined in the issue
• fulfill the feature requirements
• bump package version

/cc @milosloub

[jaredcnance]

The dotnet CLI comes with a command for this: dotnet clean

[jaredcnance]

👏 👏 👏

[jaredcnance]

Generally, it is better to check the status code immediately after getting the response. De-serialization will fail if the response type is an ErrorCollection which makes it difficult to understand the actual error. Something like this would be fine:

// act
var response = await _fixture.Client.SendAsync(request);

// assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);

// or even something like:
Assert.True(response.StatusCode == HttpStatusCode.OK, $"Received response with status code: '{response.StatusCode}' and body: {response.Content.ReadAsString()}");

Otherwise, I just see this in the build log which isn't particularly useful:

[NullVoxPopuli]

I getchya. I was just copying from other tests ;)

[NullVoxPopuli]

@jaredcnance how much refactoring would you want on acceptance tests? I typically extract all the repetitive stuff like request building and such into helper methods (I could do this in a subsequent PR)

[jaredcnance]

The problem you're running into is that we're trying to compare DateTime? (left) and string (right). I think you meant to include the logic in the else block (ln143-153) here (the conditional on ln 147 is always false).
In this block, right should be null.

Now, backing up a bit to your comment:

what's to stop the usage of is:somethingelse? :-\ would we hard-code checks on null value usage?

I'm not sure it makes sense to just have a "special equals" that handles null/not null cases and everything else is the same as eq: and ne:....:thinking:

If we stick with a new operator, I think it should be a single value and not an operation/value pair.

Maybe something like this. And any value after the operator is ignored.

• ?filter[attr]=isnull:
• ?filter[attr]=isnotnull:

Based on this it seems like others have chosen to just use null as a special filter value. Maybe we could hide it behind a global feature flag? And maybe provide fine-grained control at the Attr level:

options.AllowNullFilterValues = true;

[Attr(allowNullFilters: false)]
public string NeedToFilterOnEmptyStrings { get; set; }

or maybe define an enum:

[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.AsNull)]
[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.AsEmptyString)]
[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.Ignore)]

@rtablada do you have any insight into how other frameworks handle this problem?

[NullVoxPopuli]

Maybe something like this. And any value after the operator is ignored.

This is actually what I started with, should be easy to go back. :)

[Attr(unsetFilterValueBehavior: UnsetFilterValueBehavior.AsNull)]

I guess the case I keep thinking of is what if someone has a nullable string field?

In previously projects (rails), I used https://github.com/activerecord-hackery/ransack which has a bunch of matchers: https://github.com/activerecord-hackery/ransack#search-matchers including null and not_null

[jaredcnance]

LGTM! A few minor comments on the tests, once that's in, I think we'll be good to go ahead and release this. Thanks for the awesome work!

[jaredcnance]

Let's verify the returned TodoItems have actually been filtered (e.g. all results have UpdatedDate = null.
Maybe even consider explicitly adding a TodoItem to the db with UpdatedDate = notNullDateValue;
It shouldn't be an issue after #294, but for now I think we should assume we're not working with an empty db.

[NullVoxPopuli]

added assertion

[jaredcnance]

See comment below.

[NullVoxPopuli]

added assertion