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

Switch to using SQL DB for the on-server feedback, not Redis #14

Closed
BryceStevenWilley opened this issue Jul 19, 2022 · 0 comments · Fixed by #18
Closed

Switch to using SQL DB for the on-server feedback, not Redis #14

BryceStevenWilley opened this issue Jul 19, 2022 · 0 comments · Fixed by #18
Assignees

Comments

@BryceStevenWilley
Copy link
Contributor

Most of the conversation already happened in the original PR.

Hm. SQLObject tbh seems a lot more complicated, and even though I don't expect us to go above 1000 feedback entries a year, it might be a pain to port those to be SQLObjects eventually, and I don't know how well Redis will scale / when it will need to be ported (I expect Redis will scale pretty well actually, but the pickling might slow things down). SQLObjects also seem to be more in line with other types of ORMs, but we don't need an ORM: we need a write once, read a few times sort of thing. Wrapping that in 2-3 functions that just execute SQL (create the table, add an entry, and get a list or subset of entries) matches how this interview will access the DB info a lot closer, and I've done a ton of similar things in the EFSP server, so I might try for that implementation.

We'll need something similar for the thumbs-up/down feedback thing in the toolkit too probably. I might put this on hold until I get something ready for that feature, and then change this to use that feature as well.

I think you're right, just directly using the SQL queries might be better. Just use SQLAlchemy or a similar SQL frontend tool for the features that help prevent SQL injection, but we don't need to map it to an object.

@BryceStevenWilley BryceStevenWilley self-assigned this Jul 19, 2022
BryceStevenWilley added a commit that referenced this issue Jul 21, 2022
In anticipation of scaling better with thousands of feedback items.
The only significant change is to change the GUID to a normal, incrementing SQL int id. GUID isn't a SQL standard thing (only a postgres thing), and tbh we don't get too much from it, so the flexibility of working with other dbs is probably better than keeping it.

Fixes #14.
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 a pull request may close this issue.

1 participant