pilagod / gorm-cursor-paginator

A paginator doing cursor-based pagination based on GORM
https://github.com/pilagod/gorm-cursor-paginator
MIT License
188 stars 44 forks source link

Support encoding/decoding structs #15

Closed dadgar closed 4 years ago

dadgar commented 4 years ago

This PR reworks how the cursor decoder works to support decoding structs. I believe it also simplifies the logic by removing custom type tracking. I added tests to support both an embedded struct and and a pointer to a struct.

The summary of the logic is to decode each element of the JSON array directly to the reflected zero value of the referenced struct by key name.

Primary motivation is we have a type that is roughly as follows:

type Model struct {
  ID UUID
  CreatedAt time.Time
}

type UUID struct {
  UUID []byte
}

The first commit adds a test showing that this behavior did not work.

dadgar commented 4 years ago

A suggestion for the v2 would be to allow Encode/Decode to return errors. As it is now, things get swallowed and would be hard to debug issues. Also there is not are way to handle encoding errors.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 73


Changes Missing Coverage Covered Lines Changed/Added Lines %
cursor_decoder.go 46 55 83.64%
<!-- Total: 46 55 83.64% -->
Files with Coverage Reduction New Missed Lines %
cursor_decoder.go 1 84.88%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 70: -2.6%
Covered Lines: 263
Relevant Lines: 276

💛 - Coveralls
pilagod commented 4 years ago

@dadgar This is awesome!!!!!

I have been bothered by type encode & decode problem and don't have a good way to handle it. You do it in a clean way and also make it support custom type, which is not possible to fulfill in my original implementation.

I totally agree to your suggestion for error handling. In v2 I will make both en/decoder and paginator Paginate to return error. Currently paginator Paginate only has ability to return DB error through gorm.DB. I didn't realize at beginning that cursor en/decode process could reach so much unexpected outcome.

This PR is really fantastic, many many thanks to you 😃

dadgar commented 4 years ago

@pilagod thanks for the great feedback! Do you think this could be merged and a new tag cut?

pilagod commented 4 years ago

Yes, I think this can be merged and release under v1.3.0, because all regression tests are passed and no breaking interface changes. For error enhancement, I have created another issue #16 to address it.

I would appreciate it if you could take some time to commit those refinements mentioned above. After that we can finish this merge 😃

Thanks again for your contribution, this does mean a lot to this package!

dadgar commented 4 years ago

@pilagod Sorry could you clarify what the refinements are you are looking for before this merges?

pilagod commented 4 years ago

@dadgar Oh sorry, I added these comments but forgot to submit, my fault 😭

dadgar commented 4 years ago

@pilagod Addressed the comments! PTAL

pilagod commented 4 years ago

@dadgar It looks good to me, let me merge it 👍

pilagod commented 4 years ago

I have released v1.3.0 to include this change 🎉🎉🎉