Robot table #19
Robot table #19
Conversation
|
pretend this pr doesnt exist for now i will make a mess of things |
|
This PR is "done" now, but I still wouldn't call the userauth side of db done (not even just what we currently have implemented). The logic with the auth tokens needs to actually ping the db |
|
Im assuming add to waitlit creates a new banned user |
More specifically each "id" key is a String and a primary key. No secondary keys are admitted. Note for the future: "columns" is a really bad name for create_dynamodb_table, maybe keys would be better.
"Columns" is misleading because it implies that every field of the table ought to be specified, which is contrary to how NoSQL databases work.
but the types are fine because the command runs
this is REALLY IMPORTANT!!!
This will reduce the mental overload when we inevitably need to deserialzie these tokens!
we want unique ids on everything
|
rebased on master and force pushed |
| keys=[ | ||
| ("id", "S", "HASH"), | ||
| ], | ||
| gsis=[("emailIndex", "email", "S", "HASH")], |
There was a problem hiding this comment.
nit
| gsis=[("emailIndex", "email", "S", "HASH")], | |
| gsis=[ | |
| ("emailIndex", "email", "S", "HASH"), | |
| ], |
reason is that if we add more indexes it keeps the file blame easier to parse
There was a problem hiding this comment.
I initially did that, make format is what made it all in one line. I concur, I would have written it in multiple lines, but probably best to obey the linter.
There was a problem hiding this comment.
if you put a comma inside the list, black will format it this way
| ], | ||
| gsis=[("ownerIndex", "owner", "S", "HASH")], |
| @@ -40,16 +43,16 @@ async def add_token(self, token: Token) -> None: | |||
|
|
|||
| async def get_token(self, email: str) -> Token | None: | |||
| table = await self.db.Table("Tokens") | |||
| token_dict = await table.get_item(Key={"email": email}) | |||
| if "Item" not in token_dict: | |||
| token_dict = await table.query(IndexName="emailIndex", KeyConditionExpression=Key("email").eq(email)) | |||
There was a problem hiding this comment.
why not get_item here? this should be a unique element right, not a list?
There was a problem hiding this comment.
Unfortunately DynamoDB does not allow you to get_item on anything besides a primary key. It has no mechanism of enforcing uniqueness on secondary indices, which is why we have to manually check that emails are unique whenever we attempt to perform a user insertion.
Furthermore, even after we manually enforce uniqueness, DynamoDB still has no interest in just returning the first entry with the correct email it finds. There is no method to perform a singular query on a secondary index.
There was a problem hiding this comment.
never mind we will only be using email for tokens
There was a problem hiding this comment.
actually tokens and the entire auth structure require significant rearch. think it is worth considering using traditional cookie based auth since it is much simpler and easier to implement. (notably i believe this implementation is wrong, though how easy it is to fix is not known to me)
| AttributeDefinitions=[{"AttributeName": n, "AttributeType": t} for n, t, _ in columns], | ||
| AttributeDefinitions=[ | ||
| {"AttributeName": n, "AttributeType": t} | ||
| for n, t in itertools.chain(((n, t) for (n, t, _) in keys), ((n, t) for _, n, t, _ in gsis)) |
|
closing bc changes got moved onto later branch |
Also set primary key as "id" for all tables and rename "columns" to "keys" ("columns" was quite misleading)