Skip to content

There is no good way to filter by the parent with isnull or isnotnull operators #548

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

Closed
sk-pub opened this issue Aug 26, 2019 · 7 comments · Fixed by #549
Closed

There is no good way to filter by the parent with isnull or isnotnull operators #548

sk-pub opened this issue Aug 26, 2019 · 7 comments · Fixed by #549

Comments

@sk-pub
Copy link
Contributor

sk-pub commented Aug 26, 2019

Description

There is no good way to filter by the parent with isnull or isnotnull operators.

If I have a class Child referencing the Parent, both use int as a Key, then I cannot use a filter query like this
api/children?filter[parent.id]=isnull:
— it fails to create the equality expression (because you can't compare int and null).

There are 2 bad workarounds for this right now:

  1. Add the [Attr] attribute to the Parent property of the Child and use the following filter instead
    api/children?filter[parent]=isnull:
    It causes the loading of the whole Parent hierarchy into attributes.
  2. Change the Key type from int to Nullable. It's nonsense to have a nullable primary key.

Environment

  • JsonApiDotNetCore Version: the current master branch
@sk-pub
Copy link
Contributor Author

sk-pub commented Aug 26, 2019

Created a pull request to fix the issue: #549.

@wisepotato
Copy link
Contributor

why do you want to check for isnull on the id? Why not check it on the referencing foreign key?

@sk-pub
Copy link
Contributor Author

sk-pub commented Aug 27, 2019

@wisepotato, you mean the 1st option from the description?
To use a filter as ...?filter[parent] I have to add the [Attr] attribute to the Parent property and it's not what I want — "parent" appears in attributes in the output JSON.

@sk-pub
Copy link
Contributor Author

sk-pub commented Aug 27, 2019

To be completely clear, here is the class I have

    public abstract class View : Identifiable<int>
    {
        [Required]
        [Attr]
        public string Name { get; set; }

        [HasMany]
        public IList<View> Children { get; set; }

        [HasOne]
        public View Parent { get; set; }
    }

I need to load only top-level items:
...api/views?include=parent,children&filter[parent.id]=isnull:

@maurei
Copy link
Member

maurei commented Aug 29, 2019

Hi @unicrus, thanks for opening up this issue. Filtering by the existence of relationships seems like a very useful instrument and we should support this. I fully agree the workarounds you mentioned are not desired.

I'm not sure however about the semantics you're proposing to implement filtering on (non-)existing relationships
api/children?filter[parent.id]=isnull
To me intuitively, the following would make more sense:
api/children?filter[parent]=isnull
Of course, we should support this without having to add [Attr].

Some motivations for this choice:

  • The latter could be extended more intuitively to work for HasMany and HasManyThrough relationships, by introducing something like isempty and isnotempty:
    api/parent?filter[children]=isempty
    whereas I'm not sure if in this case checking for null primary keys is possible.
  • I think that checking for parent.id=isnull implicitly exposes some internals of JsonApiDotNetCore that the developer should not have to worry about. I believe when the parent relationship is not set, internally an "empty dummy" instance is assigned to the parent property, (which is not shown in the API response), and this I think is the reason why calling parent.id internally when checking for null does not throw a null reference exception. This would break if we would no longer use an empty instance but set the parent property to null instead, (which I feel would be the cleanest approach and could follow from refactoring in the future)

What are your thoughts on this @unicrus?

@sk-pub
Copy link
Contributor Author

sk-pub commented Aug 29, 2019

I think is the reason why calling parent.id internally when checking for null does not throw a null reference exception.

The reason is that Entity Framework generates SQL using expression trees. That doesn't throw a NullReference exception even if Parent is null. It's the same even in NHibernate — you can write items.Where(item => item.Parent.Id == null) and it will be executed as SQL

select *
from items item
join items parent on item.Parent = parent.Id
where parent.Id is null

with no issues.

So the query api/children?filter[parent.id]=isnull: may only expose some internal stuff related to how ORM and SQL work, but not something specific to JsonApiDotNetCore.

I think that the query api/children?filter[parent.id]=isnull: is OK since it transforms into a valid and clear LINQ expression and SQL query.

And I agree that it may not be useful to people who do not work with an SQL database. The expression items.Where(item => item.Parent.Id == null) will fail with NullReferenceException if you try to execute it with LINQ-to-objects (on a dictionary, for example).

To summarize, I think, the best solution would be to add support of the api/children?filter[parent]=isnull: query without the need to add the [Attr] attribute to the Parent property.
The request api/children?filter[parent.id]=isnull: is also valid if you're using an SQL database. And this solution is significantly easier to implement (from my perspective).

@maurei
Copy link
Member

maurei commented Aug 29, 2019

Thanks for elaborating on this. What you're saying makes sense to me: [parent.id]=isnull is not really exposing anything about JADNC. Rather than that it is just reflecting querying semantics used in the realm of SQL. As such it makes sense to support [parent.id]=isnull for now as [parent]=isnull indeed is a lot more work. I'll accept your PR and make new issues for [parent]=isnull

Thanks :)

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

Successfully merging a pull request may close this issue.

3 participants