search

cajuncoding / GraphQL.RepoDB

A set of extensions for working with HotChocolate GraphQL and Database access with micro-orms such as RepoDb (or Dapper). This extension pack provides access to key elements such as Selections/Projections, Sort arguments, & Paging arguments in a significantly simplified facade so this logic can be leveraged in the Serivces/Repositories that encapsulate all data access (without dependency on IQueryable and execution outside of the devs control).
MIT License
40 stars 5 forks source link

Bug in IEnumerableOffsetPagingCustomExtensions.SliceAsOffsetPage #8

Closed davidjamesb closed 4 months ago

davidjamesb commented 2 years ago

Hi. There is a bug in this line where Take() is retrieving 1 item more than it should:

https://github.com/cajuncoding/GraphQL.RepoDB/blob/52dabefdeec5f6d29e4f97c3a927bc05426ac31c/GraphQL.PreProcessingExtensions/Paging/OffsetPaging/IEnumerableOffsetPagingCustomExtensions.cs#L54

It should be:

var pagedResults = items.Skip(skipPast).Take(takeSome).ToList();

This is particular nasty if no skip and take arguments are specified in the graphql query. In that case the value for take will be int.MaxValue = 2147483647. Adding 1 to that will cause it to wrap around to -int.MaxValue-1 = -2147483648. This causes it to always return an empty pagedResults.

Rather than relying on int.MaxValue I think it is better to instead use the default values set when calling IRequestExecutorBuilder.SetPagingOptions - however I'm not sure how to inject/access these values such that GetOffsetPagingArgsSafely can use them:

https://github.com/cajuncoding/GraphQL.RepoDB/blob/52dabefdeec5f6d29e4f97c3a927bc05426ac31c/GraphQL.PreProcessingExtensions/Paging/OffsetPaging/IResolverContextOffsetPagingExtensions.cs#L20

I didn't specific any pagination arguments on my query and I expected it to return a page size that I set as the default. I was puzzled why I kept getting back no results.

davidjamesb commented 2 years ago

Hey @cajuncoding. Did you close this prematurely? The PR I submitted is independent of this issue here.

cajuncoding commented 2 years ago

Well that’s what I get for multi-tasking…I totally confused/conflated the PR with the Issue…re-opening now and look more closely tomorrow.

cajuncoding commented 2 years ago

You’ve raised an excellent issue in the logic of incrementing take by one! Def. a bug waiting to happen…

The incrementing of take by one is how we efficiently determine if there are any following pages… for which this logic would always be false if the default int.MaxValue is used. We can certianly improve the logic to handle that case more explicitly.

This custom extension method is intended to be only a helper method with no coupling to any defaults configured for GraphQL… which would be made avaialable and/or controlled by the consuming code (whatever layer that may be)… So it should rely on defaults and or throw exceptions if missing arguments aren’t provided.

However, as you it would be intuitive that this would/could be handled by GetOffsetPagingArgsSafely().... however, it's important to note though that this is handled in a non-intuitive way by HotChocolate and there are some issues accessing these default values, once set by HC in startup, making this not as easy as I wished.... and I noted it as a pending item not done in the Readme for the PreProcessing extensions project here:

image

However, in your code you can store your defaults elsewhere (e.g. your own Constants) and then implement them.... And this would be easier to do if the null value wasn't already resolved... I think an overload that doesn't resolve the nulls would enhance this so that it's easier to work with :-)...

cajuncoding commented 2 years ago

To clarify, I didn't put alot of time in testing these custom extensions because these are in-memory processing only... which means that in many/most situations they won't be suitable/scalable. Ideally paging should be pushed down into the Database layer.

These were primarily intended to be used to helping test/validate the RepoDB libraries that push the Pagination down into the database layer (Sql Server only supported/tested at this time) here where paging is done efficiently by the DB..

In addition, for some users simply using HotChocolate's OOTB paging to handle IEnumerable may be a good way to go.

However, we should def. fix the bug you described here because of course there may be other use cases for these extensions-- as long as you keep an eye on the performance implications of doing all in-memory processing.

Just wanted to provide a little more context here to the purpose of these extensions 😏

cajuncoding commented 4 months ago

The core logic noted by this issue has been enhanced to be more reliable and intuitive for it's expected use case as a low level extension method. It is still assumed that if you call this manually then you must also manage your own default values manually because there should not be any coupling between the extension method to the default values managed by Hot Chocolate (in addition to not being accessible).

However, the method should not result in overflow issues or fail, and should reliably process results and skip/take params... If null is passed for take then it is assumed that take=0 and therefore no results will be returned, however the hasNextPage logic will still work as expected and return true by looking for any item past the take boundary and if anything exists then there is by definition more results to retrieve after take=0.

This is validated for expected behavior in the Unit Test: https://github.com/cajuncoding/GraphQL.RepoDB/blob/505ce562788e52ed0c22fc7a4055e723c2f79603/GraphQL.ResolverProcessingExtensions.Tests/UnitTests/ParamsContextPagingTests.cs#L214