mrahhal / MR.EntityFrameworkCore.KeysetPagination

Keyset/Seek/Cursor pagination for Entity Framework Core.
MIT License
236 stars 12 forks source link

Is library thread safe? Particularly the HasNextAsync/HasPreviousAsync methods? #47

Closed mr-davidc closed 1 year ago

mr-davidc commented 1 year ago

I have been writing some unit tests for a project of mine which call an API endpoint which uses keyset paging and I have noticed that when I run the tests until failure, they will intermittently fail. They always seem to fail on checking whether HasMore is = true/false.

This is the line of my test that always fails:

paging!.HasMore.Should().BeFalse(); OR paging!.HasMore.Should().BeTrue();

This is my keyset pagination query:

var items = await dataContext.Items
    .KeysetPaginate(b => b.Ascending(m => m.Id), KeysetPaginationDirection.Forward, reference)
    .Query
    .Select(u => new
    {
        Id = u.Id
    })
    .Take(take)
    .ToListAsync();

This is how I determine whether hasMore (cut down for brevity):

if (keysetPaginationContext.Direction == KeysetPaginationDirection.Forward)
{
    hasMore = await keysetPaginationContext.HasNextAsync(items);
}
else
{
    hasMore = await keysetPaginationContext.HasPreviousAsync(items);
}

This is my Paging<T> model which contains hasMore:

public class Paging<T>
{
    public bool HasMore { get; init; }
    public List<T> Items { get; init; }

    public Paging(List<T> items, bool hasMore)
    {
        Items = items;
        HasMore = hasMore;
    }
}

I am wondering if because the 8 tests run in parallel, perhaps something is getting wires crossed between tests? Some state is being used somewhere in the HasNextAsync(items) or HasPreviousAsync(items) ?

I am also creating the EF Core 7 data context fresh for every test so there should be no issues regarding that.

Any advice would be much appreciated.

mrahhal commented 1 year ago

No internal state is kept for any of the Has calls. Regarding thread safety, EFCore's DbContext itself isn't thread safe, and so obviously this library in extension isn't. The returned context that you chain Has calls on isn't as well. But similarly to thread safety guarantees in .NET, any static state is guaranteed to be thread safe, you shouldn't have to worry about that.

In the Has* calls, this library only calls into EFCore's AnyAsync (you can check the code here). But you mentioned that you recreate the DbContext in each test, so that removes one possibility.

Where are you getting the keysetPaginationContext from in your second snippet? It doesn't look like you're storing it anywhere in the first snippet. I can't tell what the problem could be from just the snippets you have above, especially that they don't seem to be connected. If you can have a full small repro repo of this on GitHub I might be able to have a closer look.

mrahhal commented 1 year ago

Also, your second snippet looks weird to me. Why are you testing on the direction and then calling either HasNextAsync or HasPreviousAsync depending on that? You might be misunderstanding HasNextAsync/HasPreviousAsync.

mr-davidc commented 1 year ago

Hi @mrahhal , thanks for the response.

You might be misunderstanding HasNextAsync/HasPreviousAsync.

Hmm, I had a look through the source code again and I think you might be right.

I was under the impression that if the pagination context had been configured for a Forward direction, then I needed to call HasNextAsync to determine if there is "more items to get" and then vice versa for a Backward direction? But I think what you're saying is, for my use case, I should just always call HasNextAsync (regardless of what direction the pagination context is set as) ?

If so, then I will make that change in my code but I'm not sure it would resolve the intermittent test failures I am getting. And if it still isnt working, then yes I will get a GitHub repo together for you to have a look at.

Thanks

mrahhal commented 1 year ago

Yes, HasNextAsync will always tell you if there's next relevant to your defined keyset order, the particular direction of your query doesn't matter. But a caveat is that you need to make sure EnsureCorrectOrder is called on your data list before calling these methods (this is explained in the README).

mr-davidc commented 1 year ago

Hmm, so I went and made the change to only call HasNextAsync regardless of direction but then my code didnt work anymore.

For more context, I have two parameters, one called starting_after and another called ending_before. starting_after is for navigating forwards (i.e. getting the next page) and ending_before is for navigating backwards (i.e. getting the previous page).

Given a simplified paging example, with set of Ids like so:

1 2 3 4 <---- ending_before 5 6

If ending_before is set to 4 and page size of 10, my expected returned items are 1,2 and 3 and HasMore = false. If I use HasNextAsync, it actually returns HasMore = true because the DB query checks for Id values > 4 and not for Id values < 1 (as HasPreviousAsync does).

Using HasNextAsync works when used in conjunction with starting_after because I am navigating forwards through the result set.

mrahhal commented 1 year ago

It's very likely you're doing something incorrectly at some point (I feel in particular related to how and with what you're calling the Has* methods). The use case you explained above is extremely basic. You can check MR.AspNetCore.Pagination to compare and contrast with what you're doing, since I do something very similar here in a generalized way. Otherwise, I might need a small repro.

In addition to MR.AspNetCore.Pagination, also make sure to check the sample in this repo for examples on how to use these properly.

mrahhal commented 1 year ago

I'm closing this for lack of additional info. Please reopen if you still have issues.