Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor pagination SQL queries to use generic paramaters #2021

Merged
merged 2 commits into from
Dec 30, 2024
Merged

Refactor pagination SQL queries to use generic paramaters #2021

merged 2 commits into from
Dec 30, 2024

Conversation

rvarun11
Copy link
Contributor

Changes examples to use [] to match the expected [Action] parameter type.

@rvarun11 rvarun11 requested a review from mpscholten December 14, 2024 15:57
@mpscholten
Copy link
Member

Will the empty list not generate a type error?

@rvarun11
Copy link
Contributor Author

No, using the () you get

 Couldn't match expected type ‘[Database.PostgreSQL.Simple.ToField.Action]’

It will get rid of that error but you may need to annotate it with the type of model if you get the following:

 The type variable ‘model0’ is ambiguous

to fix that, you can:

(users, pagination) <- paginatedSqlQuery @User "SELECT * FROM users" []

Should I add this as well?

@amitaibu
Copy link
Collaborator

Yes, I think it's best to have a fully working example.

@rvarun11
docs: Fix pagination examples to use empty list instead of unit
dbd7bb5 
Changes examples to use [] to match the expected [Action] parameter type.
@rvarun11 rvarun11 force-pushed the doc-pagination-example branch from 282a457 to dbd7bb5 Compare December 15, 2024 11:51
@rvarun11 rvarun11 requested a review from amitaibu December 15, 2024 11:52
@mpscholten
Copy link
Member

The type of paginatedSqlQuery is wrong. It needs to be a type parameter. E.g. sqlExec or sqlQuery in IHP use a generic query type argument with a ToRow q constraint:

ihp/IHP/ModelSupport.hs

Line 416 in 17f17b5

sqlExec :: (?modelContext :: ModelContext, PG.ToRow q) => Query -> q -> IO Int64

@rvarun11 rvarun11 changed the title docs: fix pagination examples to use [] instead of () Refactor pagination SQL queries to use generic paramaters Dec 21, 2024
@rvarun11 rvarun11 self-assigned this Dec 21, 2024
@rvarun11 rvarun11 force-pushed the doc-pagination-example branch 2 times, most recently from 41fad11 to 08fc538 Compare December 21, 2024 21:14
@rvarun11 rvarun11 marked this pull request as draft December 21, 2024 21:41
@CSchank
Copy link
Contributor

CSchank commented Dec 21, 2024 •
edited
Loading

@rvarun11 We played with this exact problem recently and ran into lots of weird stuff with the constraints. I believe the FromRow constraint is unnecessary and will constrain things more than needed. For instance, sqlQuery doesn't require it: https://ihp.digitallyinduced.com/api-docs/IHP-ModelSupport.html#v:sqlQuery. It would mean you can't use certain types of values as inputs, if they don't have FromRow instances, at least that's what we ran into.

I think it seems to be needed here because of the subquery to do the count query. We actually implemented this exact function in our project a few months ago and were able to avoid the need for FromRow. Our code isn't open source, but I'll provide you a link to our version in case it's useful.

@CSchank
Copy link
Contributor

CSchank commented Dec 23, 2024 •
edited
Loading

Update: sorry, I got confused. FromRow is on the output result, not the input. So I think this matches what we came up with. I think for a while our type signature was more complex and I remember us running into some other weird things but we both arrived at the same type signature in the end. 😊

And sqlQuery does have the FromRow constraint on the output too, I must have misread it yesterday!

@rvarun11 rvarun11 force-pushed the doc-pagination-example branch 5 times, most recently from 5733289 to 4359aac Compare December 27, 2024 17:44
@rvarun11 rvarun11 marked this pull request as ready for review December 27, 2024 17:45
@rvarun11
Copy link
Contributor Author

@mpscholten It is ready for review

mpscholten
mpscholten reviewed Dec 27, 2024
View reviewed changes
IHP/Pagination/ControllerFunctions.hs Outdated
@@ -179,12 +173,12 @@ defaultPaginationOptions =
--
-- *AutoRefresh:* When using 'paginatedSqlQuery' with AutoRefresh, you need to use 'trackTableRead' to let AutoRefresh know that you have accessed a certain table. Otherwise AutoRefresh will not watch table of your custom sql query.
paginatedSqlQuery
:: forall model
. ( FromRow model
:: ( FromRow r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the type variable name changed from model to r?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To maintain consistency with similar functions:

ihp/IHP/ModelSupport.hs

Line 379 in 17f17b5

sqlQuery :: (?modelContext :: ModelContext, PG.ToRow q, PG.FromRow r) => Query -> q -> IO [r]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but let's keep the old name. Single variable names are not good. (We should eventually patch sqlQuery to fix this there as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. That makes sense. I've updated the PR accordingly.

IHP/Pagination/ControllerFunctions.hs Outdated
@@ -202,14 +196,14 @@ paginatedSqlQuery = paginatedSqlQueryWithOptions defaultPaginationOptions
--
-- *AutoRefresh:* When using 'paginatedSqlQuery' with AutoRefresh, you need to use 'trackTableRead' to let AutoRefresh know that you have accessed a certain table. Otherwise AutoRefresh will not watch table of your custom sql query.
paginatedSqlQueryWithOptions
:: forall model
. ( FromRow model
:: ( FromRow r
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@rvarun11 rvarun11 force-pushed the doc-pagination-example branch from 4359aac to efeca2b Compare December 29, 2024 12:51
@rvarun11
Refactor pagination SQL queries to use generic parameters
2cd71cf 
Change type signatures to use generic `parameters` type with `ToRow` constraint instead of `[Action]`
@rvarun11 rvarun11 force-pushed the doc-pagination-example branch from efeca2b to 2cd71cf Compare December 29, 2024 13:00
@mpscholten mpscholten merged commit cb275d4 into master Dec 30, 2024
2 checks passed
@mpscholten mpscholten deleted the doc-pagination-example branch December 30, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpscholten mpscholten mpscholten approved these changes

@amitaibu amitaibu Awaiting requested review from amitaibu

Assignees

@rvarun11 rvarun11

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rvarun11 @mpscholten @amitaibu @CSchank