[FEAT] Check field types in database emulation #4862

rix0rrr · 2023-12-06T19:32:27Z

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

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 doesn't allow that. 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).

for more information, see https://pre-commit.ci

ghost · 2023-12-06T19:34:29Z

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

Legend

rix0rrr · 2023-12-06T19:34:56Z

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

…db-types

for more information, see https://pre-commit.ci

rix0rrr · 2024-01-11T10:06:48Z

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

for more information, see https://pre-commit.ci

…db-types

jpelay · 2024-04-25T18:00:57Z

website/database.py

+                         'created': int,
+                         'classes': Optional(SetOf(str)),
+                         'keyword_language': Optional(str),
+                         'password': str,


I like this!!! I think this makes a lot of sense. More typing annotations on our DB would be a huge win. I think the work now would be to adapt the definitions of all the tables. I can handle that!

🙏 that would be great, thanks! You probably also have a better view into what the currently expected data contract is

for more information, see https://pre-commit.ci

Felienne · 2024-05-30T14:32:24Z

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

jpelay · 2024-06-06T16:54:04Z

utils.py

@@ -88,7 +88,7 @@ def times():
    return int(round(time.time()))


-DEBUG_MODE = False
+DEBUG_MODE = not os.getenv('NO_DEBUG_MODE')


Fixed it for now, but maybe this is not the cleanest way?

Doesn't this always set it to True ?

I don't think so, it's what we are already doing:

hedy/app.py

Lines 2908 to 2912 in 6e7573d

# Start the server on a developer machine. Flask is initialized in DEBUG mode, so it

# hot-reloads files. We also flip our own internal "debug mode" flag to True, so our

# own file loading routines also hot-reload.

no_debug_mode_requested = os.getenv('NO_DEBUG_MODE')

utils.set_debug_mode(not no_debug_mode_requested)

. Just that this get executed before

I think the better solution is probably to control the initialization order of all these things better, by making the tables class members of Database and initializing it when the app is created

Yeah, that makes sense too.

rix0rrr · 2024-06-06T18:56:52Z

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 · 2024-06-06T21:40:22Z

website/dynamo.py

+    def is_valid(self, value):
+        if not isinstance(value, dict):
+            return False
+        first_type = list(self.inner.keys())[0]
+        if isinstance(first_type, InstanceOf):
+            return all(first_type.is_valid(k) and self.inner.get(first_type).is_valid(v) for k, v in value.items())
+        else:
+            return all(validator.is_valid(value.get(key)) for key, validator in self.inner.items())


Rico, what do you think about this? I changed that you now can specify str in the left-hand side of the validator dictionary. This way we can be a little more generic in our type checks, like this one:

'sorted_adventures': RecordOf({ str: ListOf(RecordOf({ 'name': str, 'from_teacher': bool }))

It's important to note that you can only use str and no other type, since that would go against Dynamo naming rules either way. This is just a way to relax the type checking a little bit without needing to use dict.

A "record" (a set of known fields, with known and potentially different types for every value) is a different kind of type than a "map" (can be any field, but the types are the same).

Examples:

person = { "name": "Bertus", "age": "67" } favorite_restaurant_in_city = { "Oegstgeest": "Suzie's & Van Aken", "Amsterdam": "Ron's Gastropub", }

In Python, you might use a dict to represent both of these, but the type-of-type is very different!

They are:

person :: { "name" -> str, "age" -> int } # but favorite_restaurant_in_city :: { str - > str }

So I wouldn't overload RecordOf in this way, because it looks like it will just get confusing. Also, it would allow you to construct this... now what does that mean?

RecordOf({ str: str, "age": int })

I would make a MapOf(str, str) or DictOf(str, str) or something.

But you have to explain the differences very clearly, because I can see if even you are confused about it, there is a good chance other people will also not realize the difference.

I would make a MapOf(str, str) or DictOf(str, str) or something.

Hmm I think this would make sense! In essence what I'm trying to do is relax the checking a bit, so another name for that type of validation makes a ton of sense.

So I wouldn't overload RecordOf in this way, because it looks like it will just get confusing. Also, it would allow you to construct this... now what does that mean?

RecordOf({ str: str, "age": int })

I thought of that! If you use str in the left hand side, then you can't use a named field (and all other classes aren't allowed)

hedy/website/dynamo.py

Lines 1677 to 1680 in dd20d55

if str in validator_dict.keys() and len(validator_dict.keys()) > 1:

raise ValueError(f'If you specify str as part of the validation, you cant define more type checks.\

You dindt do that for {validator_dict}')

type_dict = {}

But you have to explain the differences very clearly, because I can see if even you are confused about it, there is a good chance other people will also not realize the difference

Yes, I need to add something to the wiki and to the comments.

jpelay · 2024-06-06T22:29:26Z

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 · 2024-06-07T08:23:27Z

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 · 2024-06-11T17:55:02Z

@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 · 2024-06-11T19:11:35Z

❤️ 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 · 2024-06-11T19:37:52Z

❤️ 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 · 2024-06-11T20:15:16Z

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

jpelay · 2024-06-12T14:49:25Z

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!!

jpelay

Let's go! And hope nothing breaks !

mergify · 2024-06-12T14:50:26Z

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

rix0rrr marked this pull request as draft December 6, 2023 19:32

[pre-commit.ci] auto fixes from pre-commit.com hooks

de29d52

for more information, see https://pre-commit.ci

rix0rrr changed the title ~~[FEAT] Check field types in emulation layer~~ [FEAT] Check field types in database emulation Dec 6, 2023

rix0rrr and others added 5 commits December 6, 2023 20:39

Fix some TODOs

73576a6

Merge branch 'dynamodb-types' of github.com:Felienne/hedy into dynamo…

a305114

…db-types

[pre-commit.ci] auto fixes from pre-commit.com hooks

74c1bf6

for more information, see https://pre-commit.ci

Remove this file

a9706ee

Precommit

6c14204

rix0rrr closed this Jan 11, 2024

rix0rrr mentioned this pull request Feb 8, 2024

🧹 Document database schema in database.py #4678

Open

rix0rrr reopened this Apr 24, 2024

rix0rrr and others added 2 commits April 24, 2024 21:05

Merge remote-tracking branch 'origin/main' into dynamodb-types

e6f1de1

[pre-commit.ci] auto fixes from pre-commit.com hooks

0e6a3f4

for more information, see https://pre-commit.ci

rix0rrr marked this pull request as ready for review April 25, 2024 05:32

rix0rrr added 2 commits April 25, 2024 08:01

Fix tests

4e7fc67

Merge branch 'dynamodb-types' of github.com:Felienne/hedy into dynamo…

a47872a

…db-types

jpelay reviewed Apr 25, 2024

View reviewed changes

rix0rrr and others added 2 commits April 27, 2024 10:08

Fix linter problems

27d0a01

[pre-commit.ci] auto fixes from pre-commit.com hooks

23c3d04

for more information, see https://pre-commit.ci

jpelay self-assigned this Apr 29, 2024

jpelay added 5 commits June 5, 2024 19:08

adding types to users and programs

44a2ee7

adding types for adventures table

29bc7aa

update invitations and tags types

de01cd0

Merge branch 'main' into dynamodb-types

1c82014

some more types

a7bfa0d

some more types and set debug mode directly from the env variable

e0278f8

jpelay reviewed Jun 6, 2024

View reviewed changes

adding a more generic type checking for records

3f217a3

jpelay reviewed Jun 6, 2024

View reviewed changes

jpelay added 4 commits June 6, 2024 17:43

flake8 made this look weird

7330e76

more types

93043ca

Merge branch 'main' into dynamodb-types

d840587

fix is_teacher field

c182318

Fix set validation

dd20d55

jpelay and others added 7 commits June 10, 2024 11:30

change recordof to dictof

e28982b

Merge branch 'main' into dynamodb-types

76a872c

fix e2e tests

5b77fdf

some fields are actually optional

6e93937

add or validator

8771d66

fix tags definition

8e86827

Merge branch 'main' into dynamodb-types

a28e451

change name from of to eitherOf

5e90276

jpelay approved these changes Jun 12, 2024

View reviewed changes

mergify bot merged commit 16f1246 into main Jun 12, 2024
12 checks passed

mergify bot deleted the dynamodb-types branch June 12, 2024 14:50

jpelay mentioned this pull request Jun 12, 2024

🪲 Initialize DB tables on app inizialitation #5609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Check field types in database emulation #4862

[FEAT] Check field types in database emulation #4862

rix0rrr commented Dec 6, 2023 •

edited

Loading

ghost commented Dec 6, 2023 •

edited by ghost

Loading

Legend

rix0rrr commented Dec 6, 2023

rix0rrr commented Jan 11, 2024

jpelay Apr 25, 2024

rix0rrr Apr 27, 2024

Felienne commented May 30, 2024

jpelay Jun 6, 2024

rix0rrr Jun 6, 2024

jpelay Jun 6, 2024

rix0rrr Jun 6, 2024

jpelay Jun 6, 2024

rix0rrr commented Jun 6, 2024

jpelay Jun 6, 2024

rix0rrr Jun 7, 2024 •

edited

Loading

jpelay Jun 7, 2024

jpelay commented Jun 6, 2024

rix0rrr commented Jun 7, 2024

jpelay commented Jun 11, 2024

rix0rrr commented Jun 11, 2024

jpelay commented Jun 11, 2024

rix0rrr commented Jun 11, 2024

jpelay commented Jun 12, 2024

jpelay left a comment

mergify bot commented Jun 12, 2024

	# Start the server on a developer machine. Flask is initialized in DEBUG mode, so it
	# hot-reloads files. We also flip our own internal "debug mode" flag to True, so our
	# own file loading routines also hot-reload.
	no_debug_mode_requested = os.getenv('NO_DEBUG_MODE')
	utils.set_debug_mode(not no_debug_mode_requested)

	if str in validator_dict.keys() and len(validator_dict.keys()) > 1:
	raise ValueError(f'If you specify str as part of the validation, you cant define more type checks.\
	You dindt do that for {validator_dict}')
	type_dict = {}

[FEAT] Check field types in database emulation #4862

[FEAT] Check field types in database emulation #4862

Conversation

rix0rrr commented Dec 6, 2023 • edited Loading

ghost commented Dec 6, 2023 • edited by ghost Loading

Legend

rix0rrr commented Dec 6, 2023

rix0rrr commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Felienne commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jun 6, 2024

Choose a reason for hiding this comment

rix0rrr Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpelay commented Jun 6, 2024

rix0rrr commented Jun 7, 2024

jpelay commented Jun 11, 2024

rix0rrr commented Jun 11, 2024

jpelay commented Jun 11, 2024

rix0rrr commented Jun 11, 2024

jpelay commented Jun 12, 2024

jpelay left a comment

Choose a reason for hiding this comment

mergify bot commented Jun 12, 2024

rix0rrr commented Dec 6, 2023 •

edited

Loading

ghost commented Dec 6, 2023 •

edited by ghost

Loading

rix0rrr Jun 7, 2024 •

edited

Loading