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

Temporary (read: BAD) fix to casing issue in Reactions template vars #359

Closed
wants to merge 2 commits into from

Conversation

cliffcaseyyet
Copy link
Member

@cliffcaseyyet cliffcaseyyet commented Jan 15, 2024

@milt @invaliduser I have uncovered an issue in reactions that is breaking UI testing, and I'm not sure how it even went unnoticed by both of us. For whatever reason SQLite does not respect alias case at all, so when you do something like:

SELECT condition_XAZ.payload as condition_XAZ
FROM xapi_statement as condition_XAZ
WHERE ((json_extract(condition_XAZ.payload, ?) = ?) AND (condition_XAZ.stored <= ? AND (condition_XAZ.statement_id = ?)) AND json_extract(condition_XAZ.payload, ?) = ?);

(and BTW the default generated titles of conditions have caps) - The actual edn row data returned from SQL ends up throwing out the case:

{:condition_xaz ...}

Which is fine except the canonical name/key for the condition is condition_XAZ, and that is what is used to perform variable injection. It concerns me that SQL is just doing whatever weird column conversion stuff it does, like what about whitespace or special characters etc. It makes me nervous that users are directly inputting something that is the query pivot of all the variable injection. I tried using double quotes on the alias because most SQL engines respect that to mean "absolutely do not mess with this alias" but alas SQLite explicitly does not support that trick.

This fix sorta kinda works for me to continue my work today, but it's extremely naive. We may need to think a bit on this one.

Copy link
Member

@milt milt left a comment

Choose a reason for hiding this comment

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

Yep, I get the approach here, but as I mentioned in standup would prefer to limit what can be used for condition names to keep things transparent and not worry about uniqueness.

@cliffcaseyyet
Copy link
Member Author

Yep, I get the approach here, but as I mentioned in standup would prefer to limit what can be used for condition names to keep things transparent and not worry about uniqueness.

that's fine. I literally called this a bad solution, i knew we had choices to make.

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.

2 participants