feat: add support for custom encoder/decoder implementations

[sashahilton00]

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]

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]

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]

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]

@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:

sashahilton00/gorm-cursor-paginator@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]

@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]

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]

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]

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]

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

[pilagod]

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

[sashahilton00]

Have pulled in the changes and updated the docs 👍

[sashahilton00]

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

[coveralls]

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]

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.