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.

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

✨ order series #14

Merged
merged 6 commits into from
Jan 14, 2022
Merged

✨ order series #14

merged 6 commits into from
Jan 14, 2022

Conversation

roiLeo
Copy link
Contributor

@roiLeo roiLeo commented Jan 6, 2022

Will it blend?

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Can we blend?

ORDER BY total DESC
LIMIT $1 OFFSET $2;
`, [limit, offset])
ORDER BY $1 $2
Copy link
Member

Choose a reason for hiding this comment

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

Can we maybe leverage interfaces for this ?

enum OrderBy {
  DESC = 'DESC',
  ASC = 'ASC'
}

with string we are bit prone to some sql hack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🏴‍☠️ Never trust user input

@vikiival
Copy link
Member

vikiival commented Jan 6, 2022

Screenshot 2022-01-06 at 12 22 57

@roiLeo
Copy link
Contributor Author

roiLeo commented Jan 6, 2022

@vikiival that wasn't working because the query was transform like:

ORDER BY 'total' 'ASC'

instead of:

ORDER BY total ASC

we have to build query dinamicly beforehand.

I've managed to setup the project, will do same change on spotlight next.
Screenshot 2022-01-06 at 17-08-01 Query node playground

@yangwao
Copy link
Member

yangwao commented Jan 11, 2022

viki has said that maybe we need to turn of server sorting something he is not opening mouth really loud so translating literally through my brain to keyboard at evening and it should work for series insights 👯‍♂️

@vikiival
Copy link
Member

@roiLeo I'm still getting weird behaviour
Screenshot 2022-01-14 at 16 06 44

@roiLeo
Copy link
Contributor Author

roiLeo commented Jan 14, 2022

@roiLeo I'm still getting weird behaviour

because

LIMIT undefined OFFSET undefined

we could try to setup default value or update nullable to false

@vikiival
Copy link
Member

vikiival commented Jan 14, 2022

@roiLeo fixed 👁️

check: 3b56c2e

@vikiival vikiival merged commit 2c00943 into kodadot:main Jan 14, 2022
@roiLeo
Copy link
Contributor Author

roiLeo commented Jan 17, 2022

Something will not work here, unique is reserved sql term, it won't be able to order this column

@roiLeo
Copy link
Contributor Author

roiLeo commented Jan 17, 2022

Capture d’écran 2022-01-17 à 3 35 42 PM

Screenshot 2022-01-17 at 17-08-49 GraphQL Playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants