-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CZID-8655] Small fix for markUploadComplete mutation #74
[CZID-8655] Small fix for markUploadComplete mutation #74
Conversation
@@ -68,14 +68,14 @@ async def mark_upload_complete( | |||
else: | |||
file.status = db.FileStatus.SUCCESS | |||
file.size = file_size | |||
await session.commit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be handled by our session handler wrappers -- is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why, but when I call that mutation, it doesn't update the DB without that line. Could it that we need session.commit here or is that unrelated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh yeah I think that's the better place for the commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I tried moving it there, but ran into 2 issues:
- It breaks this part of the code, where we need to commit to the DB to generate the UUID before returning it to the user, otherwise GQL complains with errors like
Cannot return null for non-nullable field Sample.id
(since the ID was created after the return) - We could return a successful API response, but it's possible that saving to the database fails, in which case what we're returning is not accurate 🙁
What do you think about keeping the session.commit() after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, sounds like it needs more thought, we can come back to it later 👍
Make sure we commit changes to the DB