feat: add support for custom encoder/decoder implementations

sashahilton00 commented 4 months ago

This pull request adds the ability to provider custom cursor encoder/decoder implementations.

It is a non-breaking change, adding two new config options CursorEncoderFactory and CursorDecoderFactory that if defined will be used for encoding/decoding the cursor.

Internally the library has been slightly modified to use interfaces for encoding/decoding as opposed to using the hardcoded cursor package. If no implementation is provided the existing cursor implementation is used.

pilagod commented 4 months ago

I didn't think of it before, this sounds a good idea to me 👍

Since encoding and decoding must be paired in order to function well, I would suggest to design a single interface, for example, CursorCodec containing both Encode and Decode function.

Besides that, another small question is that I don't find which line uses the DecodeStruct in the new born interface, not sure if we really need it?

sashahilton00 commented 4 months ago

I did initially use a single strict but then spun it into two for flexibility - but in reality it was probably overkill, I can merge them into one easily enough.

I need to check where DecodeStruct is used - my implementation that uses base62 was a lazy copy of the original cursor implementation with a few changes, so it may be that I didn't check it closely enough to see if I could cut it down to an interface with just Encode/Decode.

sashahilton00 commented 3 months ago

Hi,

Sorry completely forgot about this until I needed it earlier today. I've refactored and squashed the previous commits. The new interface for supplying a customer encoder/decoder is CursorCodec, and bundles the Encode/Decode methods.

Internally we still use the cursorEncoder/cursorDecoder from the original commits, as if I harmonised them into one cursorCodec I'd need to refactor the cursor package with a breaking change, which seems unnecessary given it's almost no extra overhead to add the encoder/decoder in a backwards compatible fashion.

Also DecodeStruct is gone from the interface.

pilagod commented 3 months ago

@sashahilton00 Thanks for your follow-up 🙌

Based on your work, in my opinion it could be better to further decouple the creation of codec from this library, to make codec a pure interface. I wrote an example to illustrate the design:

https://github.com/sashahilton00/gorm-cursor-paginator/compare/master...pilagod:gorm-cursor-paginator:cursor-codec

If you do agree with the suggestion, please feel free to pick up any commits from that branch, and also help us to update the document accordingly. Thank you so much 🙂

sashahilton00 commented 2 months ago

@pilagod decoupling the creation of the codec is cleaner, though it is a breaking change. If you don't mind that I'll pull the changes you made in, I might also add a base62 codec to be available out of the box, as base64 is not url safe, which was the original reason for this codec change.

There is another change I was thinking about but didn't implement because it was breaking - incorporating the paging direction into the cursor itself - so instead of passing a query param such as previous or next with the corresponding BeforeCursor or AfterCursor, instead one would pass a query param, eg. cursor, which would have the direction of forward or backward encoded in. It looks as though only one cursor is used internally anyway, so building the direction into the cursor might be a nice simplification.

As an example of what I mean, https://github.com/djrobstep/sqlakeyset does this.

pilagod commented 2 months ago

To my understanding, both versions (codec factory / codec interface) would not be a breaking change, since the new option is with proper default value which is backward-compatible, it should not affect current users. One evidence is that all tests written before this patch would be still passed.

Not sure if I missed something. Would be glad to know more details about your concern of the breaking changes.

And the new cursor idea sounds worth further studying to me. I would need some time to get a thorough understanding of it, and we can consider it in the future PR 💪

Really appreciate your opinions, they are quite inspiring. 🙌

sashahilton00 commented 2 months ago

Good point, I didn't fully review the commit you provided, on closer inspection it shouldn't be breaking. I'll pull the changes in when I get a moment and update the docs. I may add the direction change to the cursor in a separate PR if I get a moment, I'll keep this one just focused on the codec.

pilagod commented 2 months ago

Thank you so much 💪

Since changing the cursor direction usage would cause a breaking change, I would suggest that we could discuss further in issue before putting it into implementation.

sashahilton00 commented 1 month ago

Sorry about the delay, I completely forgot about this, will try and implement tomorrow.

pilagod commented 1 month ago

No worry. This is not in a hurry, just taking your time.

sashahilton00 commented 1 month ago

Have pulled in the changes and updated the docs 👍

sashahilton00 commented 3 weeks ago

@pilagod are you happy to merge this and take a look at the new implementation in #69?

coveralls commented 3 weeks ago

Pull Request Test Coverage Report for Build 11074526361

Details

21 of 21 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.06%) to 96.956%

Totals
Change from base Build 9772600585:	0.06%
Covered Lines:	414
Relevant Lines:	427

💛 - Coveralls

pilagod commented 3 weeks ago

Sorry for the late reply, I am swamped with work recently.

This PR looks good to me, though I come up with a few ideas to refine the whole structure of the document, I will merge this PR first and release a new version after the document refinement.

Really appreciating your new feature exploration in the #69, I may need some more time to check it closely in order to give out helpful feedback.

pilagod / gorm-cursor-paginator