[SE-3220] Add support for multiple statements in the XBlock #12

mavidser · 2020-10-22T19:36:04Z

This PR adds support for multiple statements in the XBlock, and improves error handling for failed queries. It also updates the XBlock version to 0.2

Adding that functionality in answer_query was a rather simple change - executing the query with executescript() instead of execute() (we actually try execute first, due to a caveat mentioned below).

executescript() in Python's sqlite library doesn't return any result back, hence any SELECT statements which are a part of executescript() are in-effect ignored. This does makes sense as multiple queries are often meant to perform updates only (no result returned), whereas multiple SELECT statements rarely makes sense.

So, if verify_query is being used and it's expecting multiple statements (eg - for testing triggers), then such queries need to be broken up into two parts (as done in this PR):

modification_query is a newly added parameter - and it will contain all modification statements to be performed after executing answer_query or the user's queries
verify_query will just contain a single SQL query, and returns a result

JIRA tickets: OSPR-5080, Opencraft/SE-3218

Sandbox URL:

Executing SQL queries might seem slow in the resource-limited sandbox, which is a pre-existing condition, due to the use of codejail. There has been no significantly increase in execution time as a result of this PR.

Studio
LMS

Testing instructions:

Install this XBlock in the LMS container by cloning the repo in /edx/src, and running pip install -e . in the repo
Run make test inside the repo
Repeat the above two steps in studio to install the XBlock there
To enable the block in a course, add "sql_grader" in "Course > Settings > Advanced Settings > Advanced Module List" inside the unit editor in studio
Add an exercise by clicking Advanced > SQL Problem at the bottom of the unit in studio
Play with different configurations and answers with the block in a course

Reviewers

@clemente
edX reviewer[s] TBD

openedx-webhooks · 2020-10-22T19:36:13Z

Thanks for the pull request, @mavidser! I've created OSPR-5080 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene · 2020-10-23T15:29:15Z

@mavidser Thank you for your contribution. Please let me know once it is ready for our review.

clemente · 2020-10-27T23:10:57Z

Allowing multiple statements in verify_query requires new changes (still under discussion), it will likely require a new optional field which stores the modification statements that will be run before running verify_query. The important point is tha verify_query must be a single statement and a SELECT, in order to return data. Unfortunately executescript doesn't return a pointer, so it's not enough to do executescript(verify_query) to have multiple statements + pointer to results.

@mavidser Please add to the description the summary of the solution when it's final.

clemente

@mavidser Great work!

I requested a few changes. Some of them are about error handling which you can do next sprint.

Other comments (positive 👍), and what I tested:

Code

the descriptions (e.g. modification_query) are good and concise and include everything (e.g. the fact that modification_query is optional)
the code isn't too complex (it is because there are many paths, but it didn't grow too much in complexity)
the code updates the requirements
I could install it in my devstack (in LMS. My Studio is broken but I tested Studio in the sandbox)

Tests

the code adds unit tests (there weren't any previously)
the old tests don't need to be modified
the new tests are very good and they're complete, including modification_query (with multiple statements) and multiple statements in answer_query
they use an in-memory database
they test clever things, like triggers, or combinations of insert+update+select (test_multiple_statements)
they also test is_ordered (to enforce ordering)
and wrong queries
they pass in my devstack, including quality tests, unit tests, coverage tests
I read through them (though I didn't have time to check every little detail) and tried modifying them

Usage

I could submit answers at the sandbox
It took some time to get feedback about my submission! like 25 seconds… It may be just in our sandboxes. It's not very intuitive to the student
I saw in the sandbox logs how it runs: 2020-11-01 23:52:52,967 INFO 27057 [codejail] [user 20] subproc.py:47 - Executed jailed code sql_grader in /tmp/codejail-gy8khxdc, with PID 2690
and I saw feedback about the expected results and my results
the To check your data modification statement, we ran the following query after your modification: text now includes the modification_query too, if present
I didn't test many cases since I'd have to design the problems, which takes time; and I already reviewed the scenarios in the unit tests
I „solved“ a complex problem that Sid prepared, that has multiple statements in student and modification queries. When I wrote the right trigger (trigger1 from answer_query), it passed
I found that error handling isn't good yet: if the student writes CRIATE, the query still runs and says „Incorrect“. But we have another task to improve it

Next steps: do the minor fixes. And I think it makes sense to leave the PR open until we add the error handling. Instead of having 2 open PRs that depend on each other. So please keep the WIP tag until all code is ready.

sql_grader/problem.py

clemente · 2020-11-12T05:11:35Z

@mavidser Thanks!

This is in addition to my previous review. Here I tested the end-user behaviour for student and staff.
Tests I did (all of them worked well):

When the student answer with errors:

When the staff creates a query with errors

verify_query with errors (e.g. INSERT+SELECT though it should be just SELECT): too many statements: Error: Problem setup incorrectly: verify_query: You can only execute one statement at a time., good
empty verify_query: I get Error: Problem setup incorrectly: You can only execute one statement at a time.. Ah, this is because I still had multiple statements in answer_query, my mistake; good. If I use a single-statement answery_query and empty verify_query, it runs
answer_query with a syntax error in one of the statements: Error: Problem setup incorrectly: near "endx": syntax error, good. The problem is incorrect
modification_query with errors: then it fails, Error: Problem setup incorrectly: near "create": syntax error, and the exercise doesn't run.
multiple statements in modification_query, one of which has an error: same, the whole exercise doesn't run

So for this new part and for the whole PR: 👍, the code is ready.

I tested this: see above (this comment, and my previous one, where I did more basic testing)
I read through the code: makes sense (see previous review) and it works, and has new tests, including test_error to check that the error message is appropriate. I didn't check all details used in test cases but I have tested most cases in practice
I checked for accessibility issues: no new issues
Includes documentation: yes, the new field is explained and the others are better explained. Also some comments
in addition: it doesn't introduce security issues; this is still run in a code jail

@mavidser Some requests:

could you update the link to the sandbox, and remove the warning about the old one
then create sandbox users and send them by mail, like last time
please update the PR description to mention that it adds some error handling and it updates versions
please squash the commits to 1 commit per major change. I'm reading in style guide that was discussed recently in our forum that you'll also need to mention the JIRA task in the title
then remove WIP, and ping edX for review, and then it can be merged

mavidser · 2020-11-13T09:22:02Z

@natabene This is ready for edx's review now 👍

kdmccormick · 2021-03-18T19:49:37Z

Hey @mavidser ; thanks for the PR! I'll take a look soon.

Do you mind rebasing when you get a chance? (Looks like it's just a re-run of make upgrade)

mavidser · 2021-03-21T22:27:41Z

@kdmccormick I've rebased the PR on master 👍

mavidser · 2021-03-29T16:48:30Z

Hey @stvstnfrd! Thanks for the detailed review! Just updating that I'll get to resuming work on this sometime next week :)

stvstnfrd · 2021-04-07T17:35:48Z

Sounds good @mavidser . Let me know when to take another look!

clemente · 2021-04-19T22:56:17Z

@stvstnfrd Hi, sorry that we couldn't review earlier. We've had reduced capacity issues and nobody available to continue last week. Next 2 sprints it seems tight too. I'll continue looking for who can review and apply the changes you proposed.
Please let us know about any deadlines.

CC @mavidser

clemente · 2021-04-30T18:12:15Z

I continued this review from @mavidser

@stvstnfrd Thanks, I applied your changes by doing more commits (didn't squash). But there's one missing, about doing error detection earlier (in Studio). I'd need more time to check. Since we're close to the estimated budget we'll also need to coordinate whether to do this fix or any other, otherwise we'll go over the estimate.
I'll take a look at this when I come back from holiday (after 2 weeks), but you can post comments earlier.

The rest of the comments are ready for a review. This can wait for Mike's comments if you prefer. I'll also write an e-mail.

I also updated the sandbox.

sql_grader/problem.py

stvstnfrd · 2021-05-06T18:53:50Z

sql_grader/problem.py

@@ -132,9 +132,11 @@ def run(database, query, is_single_query):

        database = cls.clone_database(source)

-        # `query` should be a single select statement if no verify_query is
-        # present in the problem
-        result, error = run(database, query, is_single_query=not verify_query)


stvstnfrd · 2021-05-06T18:54:34Z

sql_grader/tests/test_problem.py

        """
-        # Verify that errors are displayed when problem itself contains errors


stvstnfrd · 2021-05-06T19:08:21Z

sql_grader/tests/unit/fixtures/rating.sql

+insert into Movie values(105, 'Titanic', 1997, 'James Cameron');
+insert into Movie values(106, 'Snow White', 1937, null);
+insert into Movie values(107, 'Avatar', 2009, 'James Cameron');
+insert into Movie values(108, 'Raiders of the Lost Ark', 1981, 'Steven Spielberg');


I see. If the "live" dataset doesn't break the tests or substantially slow them down, I think we might still want to just load the live version.

As is, I don't think anything actually ensures that real dataset is valid, so we might not catch an error until it gets to production and breaks every problem in every course (the failure mode here).

Making the tests use the same data would catch invalid syntax before hitting production, by failing the test suite.
This is a significant benefit; the .sql files are code; we should at least test the syntax.

The more I think about this, the more I think we should do it; is there reason we can't/shouldn't?

It should be as easy as loading the fixture from a different path, right? 🤞

stvstnfrd

Whoops, I hit "publish" too soon...

Anyway, this looks great, thanks @clemente

I'll spin this up in my devstack today to smoke-check, but the code looks good.

The only suggested change is the mention of using the "live" dataset (.sql file) instead of a fixture one; I made a "PR Suggestion" with the change; I'll see if I can run this locally too.

stvstnfrd · 2021-05-06T19:24:56Z

sql_grader/tests/test_problem.py

+    """
+    def setUp(self):
+        current_folder = os.path.dirname(__file__)
+        sql_file = "{0}/fixtures/rating.sql".format(current_folder)


Suggested change

sql_file = "{0}/fixtures/rating.sql".format(current_folder)

sql_file = "{0}/../datasets/rating.sql".format(current_folder)

clemente · 2021-05-26T19:00:03Z

@stvstnfrd Thanks, I applied your comment.

I see. If the "live" dataset doesn't break the tests or substantially slow them down, I think we might still want to just load the live version.

I did that. I used a new commit instead of the GitHub suggestion, since I also had to delete the fixtures file.

I answered here about the proposal to do earlier validation (in Studio instead of when running the problem). But for now we'd have to wait for Mike's answer about budget (I just e-mailed him), or just consider the project done.

Let me know if you want me to squash commits (1 commit? OEP-51 conventional commits?).

stvstnfrd · 2021-06-08T07:29:43Z

@clemente Looks good! Go ahead and squash and use a conventional commit message to describe the changes.

Let me know when that's pushed and I'll merge this then cut a release!

- Add modification_query field, which allows running multiple statements before the submited answer is checked with verify_query - Improve error handling to allow multiple staments in answer_query - Add tests for the above two changes - Bump up version number

clemente · 2021-06-10T23:31:37Z

@stvstnfrd Done, I squashed everything into 1 commit.
Note that we already increased the package version from 0.1 to 0.2 since there are new features.

openedx-webhooks · 2021-06-11T18:15:42Z

@mavidser 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

clemente · 2021-06-14T18:13:51Z

@stvstnfrd Thanks! It was great working on this project, and the review process was very good.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 22, 2020

mavidser force-pushed the mavidser/se-3218-multiple-statements branch from a151caf to 149f9c4 Compare October 22, 2020 20:15

mavidser force-pushed the mavidser/se-3218-multiple-statements branch 3 times, most recently from 625c0cd to 63774d0 Compare October 24, 2020 19:48

mavidser force-pushed the mavidser/se-3218-multiple-statements branch from 1cbd3bb to 3915f01 Compare October 30, 2020 03:27

clemente suggested changes Nov 2, 2020

View reviewed changes

sql_grader/problem.py Outdated Show resolved Hide resolved

sql_grader/problem.py Show resolved Hide resolved

sql_grader/problem.py Outdated Show resolved Hide resolved

sql_grader/problem.py Outdated Show resolved Hide resolved

sql_grader/problem.py Outdated Show resolved Hide resolved

mavidser force-pushed the mavidser/se-3218-multiple-statements branch from 97e54df to 91f2c1b Compare November 10, 2020 10:33

mavidser changed the title ~~[WIP] Add support for multiple statements in the XBlock~~ Add support for multiple statements in the XBlock Nov 13, 2020

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 13, 2020

mavidser changed the title ~~Add support for multiple statements in the XBlock~~ [SE-3220] Add support for multiple statements in the XBlock Nov 13, 2020

mavidser force-pushed the mavidser/se-3218-multiple-statements branch 2 times, most recently from 9927fb7 to a7fa166 Compare November 13, 2020 09:20

openedx-webhooks added awaiting prioritization and removed needs triage labels Nov 17, 2020

openedx-webhooks added changes requested awaiting prioritization and removed awaiting prioritization changes requested labels Mar 19, 2021

mavidser force-pushed the mavidser/se-3218-multiple-statements branch from c832566 to f3196df Compare March 21, 2021 22:19

openedx-webhooks removed the awaiting prioritization label Mar 22, 2021

openedx-webhooks added the changes requested label Mar 23, 2021

openedx-webhooks added engineering review and removed changes requested labels May 5, 2021

stvstnfrd reviewed May 6, 2021

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. changes requested and removed engineering review waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 14, 2021

mavidser requested a review from a team May 26, 2021 18:48

openedx-webhooks added engineering review and removed changes requested labels May 27, 2021

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed engineering review labels Jun 8, 2021

clemente force-pushed the mavidser/se-3218-multiple-statements branch from ccd1583 to 8e6843b Compare June 10, 2021 23:28

stvstnfrd approved these changes Jun 11, 2021

View reviewed changes

stvstnfrd merged commit 858e047 into openedx:master Jun 11, 2021

openedx-webhooks added merged and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jun 11, 2021

clemente deleted the mavidser/se-3218-multiple-statements branch June 14, 2021 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SE-3220] Add support for multiple statements in the XBlock #12

[SE-3220] Add support for multiple statements in the XBlock #12

mavidser commented Oct 22, 2020 •

edited

openedx-webhooks commented Oct 22, 2020 •

edited

natabene commented Oct 23, 2020

clemente commented Oct 27, 2020

clemente left a comment

clemente commented Nov 12, 2020

mavidser commented Nov 13, 2020

kdmccormick commented Mar 18, 2021

mavidser commented Mar 21, 2021

mavidser commented Mar 29, 2021

stvstnfrd commented Apr 7, 2021

clemente commented Apr 19, 2021

clemente commented Apr 30, 2021

stvstnfrd May 6, 2021

stvstnfrd May 6, 2021

stvstnfrd May 6, 2021

stvstnfrd left a comment

stvstnfrd May 6, 2021

clemente commented May 26, 2021

stvstnfrd commented Jun 8, 2021

clemente commented Jun 10, 2021

openedx-webhooks commented Jun 11, 2021

clemente commented Jun 14, 2021

		"""
		# Verify that errors are displayed when problem itself contains errors

	sql_file = "{0}/fixtures/rating.sql".format(current_folder)
	sql_file = "{0}/../datasets/rating.sql".format(current_folder)

[SE-3220] Add support for multiple statements in the XBlock #12

[SE-3220] Add support for multiple statements in the XBlock #12

Conversation

mavidser commented Oct 22, 2020 • edited

openedx-webhooks commented Oct 22, 2020 • edited

natabene commented Oct 23, 2020

clemente commented Oct 27, 2020

clemente left a comment

Choose a reason for hiding this comment

clemente commented Nov 12, 2020

mavidser commented Nov 13, 2020

kdmccormick commented Mar 18, 2021

mavidser commented Mar 21, 2021

mavidser commented Mar 29, 2021

stvstnfrd commented Apr 7, 2021

clemente commented Apr 19, 2021

clemente commented Apr 30, 2021

stvstnfrd May 6, 2021

Choose a reason for hiding this comment

stvstnfrd May 6, 2021

Choose a reason for hiding this comment

stvstnfrd May 6, 2021

Choose a reason for hiding this comment

stvstnfrd left a comment

Choose a reason for hiding this comment

stvstnfrd May 6, 2021

Choose a reason for hiding this comment

clemente commented May 26, 2021

stvstnfrd commented Jun 8, 2021

clemente commented Jun 10, 2021

openedx-webhooks commented Jun 11, 2021

clemente commented Jun 14, 2021

mavidser commented Oct 22, 2020 •

edited

openedx-webhooks commented Oct 22, 2020 •

edited