Allow replacing NULLs with arbitrary values

[nikicc]

This PR is proposal to alleviate the issues with the nullable fields. IIUC the current suggestion, as mentioned in the know limitations, is to avoid all nullable columns. With the changes in this PR, nullable columns could be allowed as long as they are accompanied by some other columns that make the combination of all columns unique (which I think is currently not possible).

Main idea If NULLs are replaced with a user provided value, nullable columns could be included in the cursors. This PR extends the Rule definition to allow users to provide what the NULL values should be replaced with for the purposes of pagination. Then NULLs are replaced with the provided values when making SQL queries and also when encoding the cursors.

With this, also the NULLS { FIRST | LAST } problems are somehow alleviated as the user can decide where to put NULLs (with the value it provides for replacement).

@pilagod let me know what you think about this proposal. I'd be happy to fix the tests & polish the code more if you think this is a good feature to add.

[coveralls]

Pull Request Test Coverage Report for Build 170

• 19 of 21 (90.48%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.5%) to 97.093%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
paginator/paginator.go	19	21	90.48%
<!--	Total:	19	21	90.48%	-->

Totals
Change from base Build 164:	-0.5%
Covered Lines:	334
Relevant Lines:	344

---

💛 - Coveralls

[pilagod]

Hello @nikicc,

Really appreciate for your contribution! It is so nice to have you to make this project better together 😃

I am currently on vacation, I will look into the details and get back to you a few days later.

[pilagod]

Hello @nikicc. Sorry for the late reply.

I have gone through the details, this idea is really brilliant, you let me see the first light in the darkness! 🤩 Agree to what you said, this solution is possible to solve the NULL problem this package facing currently.

I share some thoughts that we can discuss further:

1. Although users can select their replacements for NULL value, it is still hard to guarantee the order of result sorting. For instance, given a SQL column with text type, if I want to put NULL last, it seems hard to define a fixed value to guarantee NULL value will be place at last. (a string bigger than any other values in the same column)

2. Rule.SQLRepr is for the column name used in where condition, but COALESCE can only apply to the value, right? Should we move COALESCE into buildCursorSQLQuery ?

3. I think encoder might not need to take care of replacement value. When NULL value found in the cursor, we can just replace it under the hood.

[nikicc]

@pilagod thanks for getting back to me!

Although users can select their replacements for NULL value, it is still hard to guarantee the order of result sorting. For instance, given a SQL column with text type, if I want to put NULL last, it seems hard to define a fixed value to guarantee NULL value will be place at last. (a string bigger than any other values in the same column)

Totally, agree. I was thinking mostly about date fields and numbers (assuming you know their min and max values) where that is somehow less of an issue. Although, the issue also present for numbers and dates if they could contain arbitrary values.

Rule.SQLRepr is for the column name used in where condition, but COALESCE can only apply to the value, right?

Yes, I think so. And you can use both if needed, as COALESCE will read the column name from p.rules[i].SQLRepr.

Should we move COALESCE into buildCursorSQLQuery ?

Yes, let me move it.

I think encoder might not need to take care of replacement value. When NULL value found in the cursor, we can just replace it under the hood.

I think you're right. Let me try that.

[nikicc]

Should we move COALESCE into buildCursorSQLQuery ?

Yes, let me move it.

Actually, @pilagod, I don't think we should move it. If we move it out of the setup function we would need to move it into both appendPagingQuery function and as well into buildOrderSQL function. As that would duplicate the logic I think it might be better if we keep it in the setup function. WDYT?

[nikicc]

I think encoder might not need to take care of replacement value. When NULL value found in the cursor, we can just replace it under the hood.

This is now done, let me know what you think.

[pilagod]

@nikicc Nice, I think you are right 💪💪💪

I just help you to add a test case (also some minor refactoring) to address this feature, it works like a charm 🚀

The only obstacle I found is that using json.Marshal to parse replace value would cause unexpected problem. For example, an empty string will be parsed into "", and will present '""' in SQL instead of ''. I think maybe we can throw the value directly to %v, how do you think?

Another question, I found out that current fields on Rule are all noun, do you think that it is better to rename ReplaceNULLWith to noun something like NULLReplacement?

[nikicc]

@pilagod sorry for a late reply. This time I was on vacation 🏝️

I just help you to add a test case (also some minor refactoring) to address this feature, it works like a charm 🚀

Thanks for adding the tests and cleaning up the logic!

The only obstacle I found is that using json.Marshal to parse replace value would cause unexpected problem. For example, an empty string will be parsed into "", and will present '""' in SQL instead of ''. I think maybe we can throw the value directly to %v, how do you think?

Yes, you are right. Not sure why I initially thought we need an extra set of quotes. I tested the current implementation and it seems to be working with both strings and numbers, so let's use '%v' as you suggested. This also makes to logic nicer 💪

Another question, I found out that current fields on Rule are all noun, do you think that it is better to rename ReplaceNULLWith to noun something like NULLReplacement?

Yes, sounds nicer. I've renamed to NULLReplacement now.

From my side, this is ready to go. I see that the coverage decreased, but I'm not sure how to cover this part in the tests; is it perhaps redundant and could go out? Please check again and let me know what do you think.

[pilagod]

@nikicc Welcome back 😎

I think the coverage missing part is due to the cursor decoder impl here: https://github.com/pilagod/gorm-cursor-paginator/blob/4f4950ced47f1b851784733442b471cb62d3b205/cursor/decoder.go#L40

Decoder parses values in cursor according to their corresponding model field type, which makes values out become typed nil. I think one way to test that code branch is to add a interface{} type field to GORM model in test, but I'm not sure how would GORM map value from DB to interface{} field in model 😂

I also can't figure out any use case that needs to define interface{} field on GORM model, and I wonder there is one that I just don't know. 😓

So for safety, maybe we can leave it there to avoid accidents, I think coverage >= 95% is well enough. It's also OK If you want to add more tests to cover it 💪

Same as you, I think this feature is almost done and can be merged at any time. Let me know when you're ready, then we go go go 🚀

[nikicc]

So for safety, maybe we can leave it there to avoid accidents, I think coverage >= 95% is well enough.

I agree. Let's leave it in.

I think this feature is almost done and can be merged at any time. Let me know when you're ready, then we go go go 🚀

I'm ready. Feel free to merge. Excited to test this once it is released 😎

[pilagod]

OK, let's go! It's almost 1:00 AM in my timezone, I will merge it and update document tomorrow. I will inform you when new version is published 😎

[nikicc]

Great, thanks! Sure it can wait till tomorrow if it's so late 💤

[pilagod]

Hello @nikicc,

I just release v2.2.0 to include this feature, and also update the document 🎉

Big thanks to your brilliant idea and hard work, it is really nice and happy to work with you 😃

[nikicc]

@pilagod thanks for the update. Will check it shortly 😄

Thanks you for reviewing and pushing the fixes in 💪 🚀 Enjoyed working with you as well 😎