Bugfix/estelle/706/unique constraint to replaces#738
Conversation
Add and modified related tests.
Add an alembic file.
bencap
left a comment
bencap
left a comment
There was a problem hiding this comment.
Thanks for the work on this Estelle. A couple comments and a broad point about deployment. We'll need a deployment rule here where we clean production of these duplicate entries. I think we have one case right now where the duplicates are clearly junk score sets, and we can just delete those and retain the published successor that is meant to be the correct superseder. No action on this PR, but I want to make sure we persist that rule here so that we know to do it on deployment.
The other two comments are about clarity in our responses and error messages. I think we don't want to assume the user wants to supersede the head-- we should do what they say and raise a clear error if we cannot supersede the score set because of the constraint. Ultimately the root of this is that score set creation isn't necessarily atomic, but I think that is something we should handle separately in #759. Here, I think as long as we return a clear error to the user we can eliminate the confusing and unintentional behavior of having multiple superseders for the same score set.
| if item_create.superseded_score_set_urn is not None: | ||
| superseded_score_set = await fetch_score_set_by_urn( | ||
| current_superseded = await fetch_score_set_by_urn( | ||
| db, item_create.superseded_score_set_urn, user_data, user_data, True | ||
| ) | ||
|
|
||
| if superseded_score_set is None: | ||
| if current_superseded is None: | ||
| logger.info( | ||
| msg="Failed to create score set; The requested superseded score set does not exist.", | ||
| extra=logging_context(), | ||
| ) | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail="The requested superseded score set does not exist", | ||
| ) | ||
| superseded_score_set: Optional[ScoreSet] = find_superseded_score_set_tail(current_superseded, Action.READ, user_data) | ||
| if superseded_score_set is None or superseded_score_set.private: | ||
| logger.info( | ||
| msg="Failed to create score set; The newest version of the requested superseded score set is not accessible.", | ||
| extra=logging_context(), | ||
| ) | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail="The newest version of the requested superseded score set is not accessible. " | ||
| "It may be private and multiple score sets supersede the same score set.", | ||
| ) |
There was a problem hiding this comment.
I'm somewhat resistant here to do tail resolution on the score set that a user specifically requested as being the one they want to supersede. I think that instead, if a user requests to supersede a score set that already has been superseded, we should return a 409 Conflict with a clear message they cannot do that. Our logic simplifies to:
If the user requests to supersede a score set:
- Check if the superseded score set exists and is accessible. If not, return a 404.
- Check if the superseded score set already has a superseding score set. If yes, return a 409.
- If we pass the above checks, we can supersede the score set and do so.
There was a problem hiding this comment.
I think your suggestion is better. I only considered that the users supersede a wrong one, but didn't consider some potential problems.
There was a problem hiding this comment.
Here, I also think we should add a try/catch for a potential race condition with the unique constraint. If a user sent off two almost concurrent requests for superseding the same score set, they might race past our checks above and fail here. That would look something like:
try:
db.add(item)
db.commit()
except IntegrityError as e:
db.rollback()
if is_replaces_id_unique_violation(e):
raise HTTPException(
status_code=409,
detail="The requested score set has already been superseded.",
)
raise
We'd need a new helper function to check if the integrity error is the one we expect.
|
I found that only one score set has multiple private superseding score sets. I think we can delete the old superseding score set manually. |
2 participants
No description provided.