mrahhal / MR.EntityFrameworkCore.KeysetPagination

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

Consider support for source generators #53

Open mpetito opened 5 months ago

mpetito commented 5 months ago

There is a fair amount of runtime expression manipulation handled by this library that might benefit from using a source generator at compile time. As I'm integrating this into a keyset paginated API, some benefits I could see from a source generated version:

It may also be reasonable to generate both IQueryable and IEnumerable versions using source generation to resolve #12, or determine if EFCore Async methods are available for IQueryable to resolve #32.

mrahhal commented 5 months ago

At the time, my opinion was that since this is built on top of an ORM, any performance gains from using source generation will be negligible compared to the database query that will take place as part of calling this. But you make a good point about supporting AOT. And this library is about performance too after all.

Also, generating bespoke methods to build the keyset might greatly decrease allocations as right now we box to object every time we obtain a column's value if it's a value type (and most columns in keysets are usually value types).

One problem would be generating the code to obtain the column values of a reference, due to loose typing this can be any object. Would need to find a good way or an alternative to remove reflection here.

This will likely require a new major version as I suspect some breaking changes, but that's fine.

As for pagination tokens. The way this library works now, it's up to the consumer how a reference is obtained. Usually, an id is sent between the front and the back. But I could see some code generation giving more flexibility to what you're allowed to provide instead of this reference. i.e. Instead of needing an object reference, it could be a record of the keyset as an alternative, which would allow a frontend to send the keyset values instead of an id, which would remove the need for a db query to load the reference. Not sure if this is similar to what you meant by serializable key.

In any case, this will be a major rewrite but would love to do this eventually.

mpetito commented 5 months ago

it could be a record of the keyset as an alternative, which would allow a frontend to send the keyset values instead of an id, which would remove the need for a db query to load the reference. Not sure if this is similar to what you meant by serializable key.

Yes, this is exactly what I'd love to see, as the lookup introduces an extra db call and is the source of a potential bug when that record has been removed in between page navigations.

One problem would be generating the code to obtain the column values of a reference, due to loose typing

Presumably the reference could be of the generic type corresponding to the query or the record of the keyset type. For other arbitrary types, you could require the caller maps their reference object to the keyset record type. Or, since the DTO pattern is common here, you could potentially name additional types during specification of the sort which would generate typed overloads that perform that mapping for you.

mrahhal commented 5 months ago

Agreed. Mapping is less ergonomic (keysetContext.HasNextAsync(data.Select(x => ...))), but could be a good base option. An alternative of providing the additional types to be generated might be a good second option as these should be limited.