Add superseded_calibration in Calibration. Modified related functions and tests. - #769
Add superseded_calibration in Calibration. Modified related functions and tests.#769EstelleDa wants to merge 9 commits into
Conversation
Coverage Report for CI Build 31660920553Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 88.699%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
…alembic down_revision identifier.
| if action is not None and not has_permission(user_data, next_score_calibration_in_chain, action).permitted: | ||
| return score_calibration |
There was a problem hiding this comment.
Here, I think we should break rather than returning the score calibration directly. Tracing the flow:
- A caller passes calibration A (private, superseded by B)
- We read A.superseding_calibration → B, and check permissions on B. The user has none
- We return A directly, with no permission check on A itself
This is not reachable today on production data because it's required a calibration or score set is published and public before it is superseded. However, if this ever changed this would silently widen permissions for superseded score calibrations without us realizing. Adding a break here would just route us through the bottom loop and check permissions as we expect.
There was a problem hiding this comment.
Basically what we talked about on Slack, where we add a new action for this which belongs in lib/permissions/score_calibration.py. Filed as #854 so the score set half can follow separately.
We should make supersession a first-class action here rather than checking READ at the call site. Something like
Action.SUPERSEDE: _handle_supersede_action,with the predicate being the UPDATE population minus the published gate: admin, or owner, or investigator_provided and a contributor to the score set.
The reason I would rather it live here than inside validate_superseded_score_calibration is that permissions/user-is-permitted/{model_name}/{urn}/{action} already serves score_calibration and takes the Action enum directly, and the front end already wraps it in useDatasetPermissions. So adding the member is the whole change as far as the UI is concerned. The calibrations page adds one string to its actions array and can render a supersede affordance. We wouldn't need a new endpoint or a new field on the model.
The creation flow is the other case: when you are creating a calibration and want to offer the option to supersede an existing one, you need a list rather than one lookup, and useDatasetPermissions resolves a single URN per call. I think that wants the existing per-score-set list endpoint rather than a search, see the comment on the search filter.
We can leave the #549 published-immutability rule for now. Superseding becomes the supported way to correct a published calibration, so there is no pressure to open up updates on published rows to make this feature work.
|
|
||
| if owner_or_contributor is not None: | ||
| query = query.filter( | ||
| or_( |
There was a problem hiding this comment.
The or_ here admits calibrations the requesting user has no read permission on. Tracing it:
- Score set
Shas userAincontributors - User
B, who has no role onS, submits a calibration toS.create_score_calibration_in_score_setsetsinvestigator_provided = False, sinceBis not a contributor, creator, or modifier Bleaves it privateAcalls/score-calibrations/me/search. The second disjunct matches on contributorship,search.privateis unset so nothing filters private rows, andB's private calibration comes back
_handle_read_action would deny that row on GET /score-calibrations/{urn}: it is private, A is not the owner, investigator_provided is false so the contributor branch does not apply, and A is not an admin. search_score_calibrations never calls has_permission, so nothing catches it.
I think this is inherited from how the score-set search is put together rather than a mistake made here. search_score_sets has the same single owner_or_contributor parameter, but it has two callers and each one supplies the precondition that makes its own use safe: /score-sets/search forces search.published = True and rejects published is False outright, and /me/score-sets/search is safe because for score sets contributor implies read unconditionally.
Neither condition applies to the calibration path:
- no
privateforcing on the calibration route, so private rows are returned by default - no
investigator_providedterm. Score sets have no equivalent concept, so there was nothing in the template to copy
Rather than add the missing precondition at this call site, I would put the check inside the helper. fetch_score_set_search_filter_options already takes requester alongside owner_or_contributor and post-filters with has_permission(requester, ...). search_score_calibrations collapsed those two into one parameter, so it cannot check permissions at all since it does not know who is asking. Restoring requester and filtering there makes it safe for the second caller too.
Either way, if we go the Action route, it is probably the case we don't actually need the endpoint this helper drives.
|
|
||
| db.add(calibration) | ||
|
|
||
| if calibration.superseded_calibration and calibration.superseded_calibration.primary: |
There was a problem hiding this comment.
This will raise when the superseding calibration is research-use-only. promote_score_calibration_to_primary rejects RUO calibrations, so publishing an RUO calibration that supersedes the current primary fails here. After line 685 has already set private = False and line 687 has added it to the session, the request aborts partway through the change. The error the caller gets is about promotion rather than publication as well.
Publishing an RUO calibration that supersedes a primary one seems like something we should allow, so I think maybe we want to skip the auto-promote for RUO calibrations.
| if superseded_calibration is None: | ||
| raise ValueError("Superseded calibration does not exist.") | ||
|
|
||
| if not has_permission(user_data, superseded_calibration, Action.READ).permitted: |
There was a problem hiding this comment.
This will gate on Action.SUPERSEDE rather than Action.READ.
As written, READ passes for any published calibration, so any authenticated user can supersede any published calibration in MaveDB.
So a stranger can take a calibration off a score set and strip its primary status in one request.
We already state the opposite policy one function over. _handle_change_rank_action carries "Community calibration owners may not ... the score set team controls ranking of community contributions." Reordering someone else's calibration is gated to the score set team, but as written deprecating it outright is gated only on being able to see it.
Action.UPDATE is not the substitute, which I think is why READ ended up here. _handle_update_action denies updates on published calibrations to everyone except admins, and only published calibrations can be superseded, so requiring UPDATE would make supersession admin-only. That is the argument for a separate action rather than reusing an existing one.
For comparison, the score set path gates supersession on owner-or-contributor rather than READ — fetch_score_set_by_urn(db, urn, user_data, user_data, True), where the fourth argument requires the caller to own or contribute to the result. Worth routing score sets through the new action too so there is one spelling of this, but that can be a follow-up issue rather than part of this PR, see #854.
| if superseded_calibration.superseding_calibration: | ||
| raise ValueError("Cannot supersede a superseded calibration. Please edit it instead.") | ||
|
|
||
| return superseded_calibration |
There was a problem hiding this comment.
There is no constraint that the superseded calibration belongs to the same score set as the one being created. The lookup above is by URN across the whole table, and nothing compares superseded_calibration.score_set_id against containing_score_set.id.
#721 describes supersession as happening within a score set, and the rest of the code assumes that. Two places would go wrong if a calibration on score set X supersedes one on score set Y:
- the visibility filter in
get_score_calibrations_for_score_setis scoped byscore_set_id, so the superseded calibration disappears from Y's list because of a row that lives on X. Nothing visible from Y explains where it went. promote_score_calibration_to_primarydemotes withinscore_set_id == calibration.score_set_id, so publishing the superseding calibration on X demotes X's primary. The superseded one staysprimary = Trueon Y while being filtered out of Y's list, which leaves Y with a hidden primary and no visible one.
I think this wants an explicit check, rejecting when superseded_calibration.score_set_id != containing_score_set.id. That does mean the function needs the containing score set passed in: right now it only sees calibration_create and the score set is resolved by the caller.
There is a genuine case for crossing score sets: porting calibrations onto a superseding score set, where when a score set supersedes another score set and each new calibration would then supersedes its original. That seems complicate though, so it's a separate issue: #855.
| foreign_keys="ScoreCalibration.superseded_calibration_id", | ||
| remote_side=[id], | ||
| ) | ||
| superseding_calibration: Mapped[Optional["ScoreCalibration"]] = relationship( |
There was a problem hiding this comment.
superseding_calibration is declared uselist=False, but replaces_id only gets a non-unique index in the migration, so nothing stops two calibrations pointing at the same original. When that happens SQLAlchemy emits Multiple rows returned with uselist=False and picks one of them arbitrarily, which makes the visibility filter in get_score_calibrations_for_score_set nondeterministic: the superseded calibration appears or disappears between requests depending on which superseding row got loaded.
We should use the behavior you implemented for score sets in #706.
| return None | ||
|
|
||
| if urn_re.MAVEDB_CALIBRATION_URN_RE.fullmatch(v) is None: | ||
| if urn_re.MAVEDB_TMP_CALIBRATION_URN_RE.fullmatch(v) is None: |
There was a problem hiding this comment.
I don't think this branch is reachable. If it's written because of MyPy complaints, then we can just type ignore the None check and add a comment mentioning #372.
|
|
||
| # Solve Pydantic model validation error | ||
| for sc in available_calibrations: | ||
| sc.superseded_calibration = None |
There was a problem hiding this comment.
These are always null in the response now, for every caller including admins. Same loop in get_score_calibrations_for_score_set, around line 151. They're the two fields #721 adds so consumers can follow the deprecation chain, so that part of the API isn't observable. That is probably why the front end half is editor-only, since there's nothing arriving for a deprecation notice to render from.
Assigning to superseding_calibration also isn't a serialization no-op, and this is an idiom that exists in the code base but I'd like to start avoiding going forward. It's the back_populates reverse side of the many-to-one, so None detaches the other calibration and marks that row's replaces_id to be nulled. Essentially if we called .commit() here, we'd erase the calibrations from the database on accident even though our intention is to just hide them from the API response.
What's the Pydantic error the comment refers to? If it's ShorterScoreCalibration failing to validate against the ORM object I'd rather fix it there than drop the data.
3 participants
No description provided.