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

feat: warn when SQL parameters are unused #9841

Merged
merged 23 commits into from May 17, 2024
Merged

feat: warn when SQL parameters are unused #9841

merged 23 commits into from May 17, 2024

Conversation

trombonekenny
Copy link
Contributor

Checks in realtime to see if a SQL query was passed more parameters than were used in the query and logs with a warning.

Copy link
Contributor

github-actions bot commented May 10, 2024

All images

Image Platform Old Size New Size Change
prairielearn/executor:e61d92f105f15c0dd44ecb0380ee11f60724871a null 1649.20 MB 1649.01 MB -0.01%
prairielearn/prairielearn:e61d92f105f15c0dd44ecb0380ee11f60724871a linux/amd64 1645.00 MB 1649.01 MB 0.24%

@trombonekenny trombonekenny marked this pull request as draft May 10, 2024 15:52
@trombonekenny trombonekenny marked this pull request as ready for review May 14, 2024 14:31
apps/prairielearn/src/lib/manualGrading.ts Outdated Show resolved Hide resolved
apps/prairielearn/src/lib/score-upload.ts Show resolved Hide resolved
apps/prairielearn/src/server.js Outdated Show resolved Hide resolved
apps/prairielearn/src/tests/groupScoreAndSync.test.ts Outdated Show resolved Hide resolved
IdSchema,
);
if (updated == null) {
if (item.id) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel a bit hesitant to check for truthy instead of for explicit null/undefined.

Suggested change
if (item.id) {
if (item.id != null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll fix that.

sql.select_assessment_instance_group,
{
assessment_id,
uid: json.uid,
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is not used, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll fix that too. I guess I was expecting a test to fail but maybe we never run something on that branch.

apps/prairielearn/src/lib/score-upload.ts Outdated Show resolved Hide resolved
@trombonekenny trombonekenny added this pull request to the merge queue May 17, 2024
Merged via the queue into master with commit ebd65b2 May 17, 2024
7 checks passed
@trombonekenny trombonekenny deleted the sqlParamCheck branch May 17, 2024 17:13
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.

None yet

3 participants