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

Guid fields do not send back an empty list when no matching guid exists #150

Open
pdevito3 opened this issue Jul 4, 2021 · 10 comments
Open
Assignees
Labels
bug Something isn't working

Comments

@pdevito3
Copy link

pdevito3 commented Jul 4, 2021

Describe the bug
When doing a filter like GuidField == 08009a84-f343-b4c5-5577-a56a23588f9d when the value exists works as expected and filters the list down to just that value, but when the value doesn't exist, it returns a full unfiltered list instead of an empty list like it usually would for a non-guid field.

To Reproduce
Steps to reproduce the behavior:

  1. Create an entity with a guid property and add the sieve filter attribute to it (e.g. id)
  2. pass a filter to your endpoint of id == 08009a84-f343-b4c5-5577-a56a23588f9d where the guid value does not exist in the dataset

Expected behavior
I would expect an empty list [] to show up like we usually see with other data types, but we get a full unfiltered list of records

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS] Windows 10
  • Browser [e.g. chrome, safari] any most recent browser
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@pdevito3
Copy link
Author

hey, @ITDancer13 any idea on when this might get resolved?

@pdevito3
Copy link
Author

just went to submit a PR for this and when I made tests for it I realized it was working! gave it a whirl in an API I have on v2.5.4 and it seems to be working now. not sure what version it got updated in, but I'll go ahead and close this out

@fority
Copy link

fority commented Jan 20, 2022

i tried filter Id==018BB311-7E80-8711-9D62-DCD5BD0B04B2
still get full unfiltered list of records

i'm using Version 2.5.4

@ITDancer13
Copy link
Collaborator

@fority can you please provide a minimal sample which shows your issue.

@fority
Copy link

fority commented Jan 20, 2022

@fority can you please provide a minimal sample which shows your issue.

sorry is my bad. I forget to add mapper to property.

it is working now

@pdevito3
Copy link
Author

pdevito3 commented Jan 25, 2022

reopening this. I AM still seeing this when i pass a non-guid to a filter, for example: /api/items?filters=itemid==string

@pdevito3 pdevito3 reopened this Jan 25, 2022
@pdevito3
Copy link
Author

FYI @ITDancer13 I made a repo for you to see it in action here.

To repro:

  1. Run the RecipeManagement api and it should open swagger automatically. If not for some reason, go to https://localhost:5375/swagger/
  2. Go to the Recipes GET endpoint
  3. Select Try it out
  4. Hit Execute and it will give you a random list of 3 items. Copy one of the guids
  5. In the Filters, enter id==YOURCOPIEDGUID and it should work as expected. It will also return an empty list if you enter a random guid, (as expected)
  6. Now, update the Filters to id==s where s is any string, and you will get a full list

I started working on a PR for this, but something like the below throws a compile error:

        [Fact] 
        public void Filtering_Guid_Field_By_String_Returns_Count_Zero()
        {
            var stringToUserForFilter = "something";
            var comments = new List<Post>
            {
                new Post
                {
                    ExternalId = Guid.NewGuid(),
                    Title = "title 1"
                },
                new Post
                {
                    ExternalId = Guid.NewGuid(),
                    Title = "title 2"
                },
            }.AsQueryable();

            var model = new SieveModel
            {
                Filters = $"ExternalId=={stringToUserForFilter}"
            };

            var result = _processor.Apply(model, comments);
            Assert.Equal(0, result.Count());
        }

So I'm not sure what the difference is offhand. It did seem like the implementation that throws a bug uses an InternalDbSet<OBJECT> which was painful to implement in the unit tests so i didn't get to fully explore that.

@pdevito3
Copy link
Author

@ITDancer13 following up on your thoughts here. This is a pretty severe bug.

@ITDancer13
Copy link
Collaborator

@pdevito3 I'll take a look at it within the next days.

@pdevito3
Copy link
Author

Happy to help or provide more info, but not familiar enough with the workings of the codebase to properly dig in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants