[FEAT] Check field types in database emulation

[rix0rrr]

There is currently a problem with a table that has an index on a certain field, but is trying to insert values of a type not matching that index. It fails in Dynamo, which requires that all values used as index keys are of a single declared type (either string, number or binary).

However, the local emulation layer doesn't catch that, because it doesn't know anything about types that need to be checked.

Add a required types argument to Table, which must declare at least the key fields (of the table itself and of the indexes), and can optionally declare the types of other fields. It will be an error to insert values of the wrong type into the database.

This may look like an easy feature, but it has some edge cases you need to be aware of:

• First of all, it relies on users declaring the indexes in code; they can choose NOT to declare the indexes in code, and there may still be an error after deploying.
• Second of all: just because you declare a type on a fields saying what may be inserted into a table going forward, this can not make any guarantees on data already in the table. Don't use this feature to make any assumptions on data you read out of the table, only use it to sanity-check new data going into it!

We may decide this change is not worth it... (for example, it would not have caught the current problem, as the index was there in Dynamo but not declared in code, so this feature wouldn't have caught it).

[ghost]

👇 Click on the image for a new way to code review

Legend

[rix0rrr]

Let's see how badly this breaks on the Cypress tests... :)

[rix0rrr]

Not sure anybody really wants or needs this, so I'll be closing it.

[Felienne]

I am picking up #4678 so it would be nice if we can merge this 🙏

[jpelay]

@rix0rrr Do you know why when I run the server it always run on production mode? I realized that the type checking always passed, and it was because DEBUG_MODE is set to False does it also happen to you?

[jpelay]

Ohh I think I get what's happening: only_in_dev is being called before DEBUG_MODE is set to True

[rix0rrr]

Ohh I think I get what's happening: only_in_dev is being called before DEBUG_MODE is set to True

Oh shit, yes that makes sense.

[jpelay]

Seems like adding an element to a set is giving errors, for example adding a student to a class gives this:

    CLASSES.update({"id": class_id}, {"students": dynamo.DynamoAddToStringSet(student_id)})
  File "/home/capybara/hedy/website/querylog.py", line 211, in wrapped
    return fn(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^
  File "/home/capybara/hedy/website/dynamo.py", line 547, in update
    self._validate_types(updates, full=False)
  File "/home/capybara/hedy/website/dynamo.py", line 695, in _validate_types
    raise ValueError(f'In {data}, value of {field} should be {validator} (got {value})')
ValueError: In {'students': Add(('user1',))}, value of students should be optional set of instance of <class 'str'> (got Add(('user1',)))

[rix0rrr]

Seems like adding an element to a set is giving errors, for example adding a student to a class gives this:

You're right, I made a mistake. Fixed it and added a test.

[jpelay]

@rix0rrr I think I'm finished tweaking the PR! Added types for all of the tables and also added two new type validators: Or and DictOf as you suggested. What do you think?

[rix0rrr]

❤️ Love it! Instead of Or(), consider perhaps naming it Either() or OneOf(), but this is great stuff!

Still not sure about the initialization of only_in_dev()... but if you feel comfortable with it, I feel good about it too!

Feel free to ship this at any point (since I'm the author, you need to be the shipper 😉 )

[jpelay]

❤️ Love it! Instead of Or(), consider perhaps naming it Either() or OneOf(), but this is great stuff!

Damn, you're good at coming out with names!

Still not sure about the initialization of only_in_dev()... but if you feel comfortable with it, I feel good about it too!

I think it's best to tackle that in a new PR, so ship this one and then take your suggestion of explicitly initializing the tables in a new PR. @Felienne wants this PR ready because it'll help her with the live documentation of the tables.

Feel free to ship this at any point (since I'm the author, you need to be the shipper 😉 )

Understood 🫡

[rix0rrr]

Thanks for taking this across the finish line, and fixing my gigantic fuckup of the code not actually doing anything 🫣🫠😆

[jpelay]

Thanks for taking this across the finish line, and fixing my gigantic fuckup of the code not actually doing anything 🫣🫠😆

Hahaha the core idea was so cool already! Thanks a lot for that!!

[mergify[bot]]

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).