hedyorg / hedy

Hedy is a gradual programming language to teach children programming. Gradual languages use different language levels, where each level adds new concepts and syntactic complexity. At the end of the Hedy level sequence, kids master a subset of syntactically valid Python.
https://www.hedy.org
European Union Public License 1.2
1.27k stars 282 forks source link

🪲 Running programs throws an error if logged in #5513

Closed rix0rrr closed 1 month ago

rix0rrr commented 1 month ago

When logged in, we automatically save a program as the user runs it.

In a recent change, we update the program counter only when a program is modified from the example code. But we wrote the entire 'program' record back to the database in an update() call, which includes the id field; Dynamo does not allow including the id field in an update() call.

This is also not necessary: a DynamoDB update() call only needs to include the fields we actually want to change, and not any of the fields that are unchanged (this may even lead to a race condition if any of those fields were changed in between the read and update calls, we would undo the intermediate change again).

Instead, only update the is_modified field, and update the Dynamo abstraction layer to check for this error condition.

How to test

Log in, and run a program.

rix0rrr commented 1 month ago

@Annelein this should have thrown an error locally before deploying and running against an actual DynamoDB, so my apologies that it didn't. But also, update() is supposed to only get the fields you want to change, while put() should be used if you really want to overwrite all fields of an entire record (and delete all non-included fields).

# Insert or replace an entire record (all fields not part of this write are gone)
table.put(entire_record)

# Update a single field (all other fields remain unchanged)
table.update(record['id'], { 'field': 'new_value', 'another_field': 'another_value' })

update() is safe against concurrent updates(), while put() will just overwrite all fields (even if another request changed any of the fields in the mean time).

mergify[bot] commented 1 month ago

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

mergify[bot] commented 1 month ago

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

Annelein commented 1 month ago

@rix0rrr thank you for the feedback, fix and explanation! Really helpful.