Move typo checks to after_insert
#17764
Conversation
| @@ -496,6 +502,92 @@ def yanked_releases(self): | |||
| ) | |||
|
|
|||
|
|
|||
| @event.listens_for(Project, "after_insert") | |||
There was a problem hiding this comment.
In other places we use our own @db.listens_for(db.Session, ... - any reason to not use that?
There was a problem hiding this comment.
My understanding is because that's a session event and not a instance event, it would require that we add a hook for every commit, and then iterate over every object created in the session to determine if any of them are a Project. I think that would work but it feels like overkill, although it does seem like we would be more likely to avoid unnecessary notifications as a result of flush().
There was a problem hiding this comment.
The other problem with after_commit is that the typo check emits SQL, which can't happen in the after_commit hook. See the test failures here: #17772
There was a problem hiding this comment.
I spoke too soon, the failures in #17772 were unrelated.
| @@ -496,6 +502,92 @@ def yanked_releases(self): | |||
| ) | |||
|
|
|||
|
|
|||
| @event.listens_for(Project, "after_insert") | |||
There was a problem hiding this comment.
This is the first time after_insert is used in warehouse - looking at the docs, it's clarifying that this happens after session flush, not as part of the ORM Insert approach.
How did you validate the notification is still sent?
(Marking as draft since this still needs tests).This is a proof-of-concept to show that we can move the non-blocking project name typo checks to an
after_insertmapper event hook, in order to avoid duplicate notifications if project creation is attempted but ends up failing due to various checks.