feat(urns): forward URNs retired by publication to the published record - #863
Merged
davereinhart merged 5 commits intoSep 16, 2026
Merged
Conversation
publish_score_set overwrites the tmp:<uuid> URN of an experiment set, an experiment and a score set in place, and refresh_variant_urns rebuilds every variant URN from the score set's. Nothing recorded the old value, so a link already shared to the unpublished record began returning 404 under a name the caller had no way to guess. Reloading a score set page after publishing it was enough to hit this. Record what each URN became in a new urn_redirects table, and resolve it in forward_retired_urns, an application-wide dependency: a read naming a retired URN is answered 308 to the same path under the record's current URN. One implementation covers every route that takes a URN, sub-resources included, and since substitution operates on the URN substring, a variant follows its score set without a row of its own. A dependency rather than ASGI middleware, because it needs the request's session; middleware runs outside dependency resolution, so it would open a session of its own that no dependency_overrides could redirect. Reads only: an owner is permitted to publish a published score set, so forwarding a stale POST .../publish would rename a live public record. And only onto a target confirmed public, since a Location header names its target to an anonymous caller before any route checks a permission. That check also keeps a deleted record's surviving row from answering a permanent redirect with a 404. The dependency reads the path from the ASGI scope. request.url.path truncates at the '#' in a variant URN, because Starlette rebuilds that URL by re-parsing it, which turns everything after the '#' into a fragment and drops the variant number, the sub-resource and the query string. lib/logging/context.py has the same pattern and is left for a separate change. Forwarding is one hop, which is all that can arise while nothing renames a published record. URNs retired before this are unrecoverable, since publication overwrote them and kept no history, so the table is not backfilled and links to records published earlier stay broken.
davereinhart
marked this pull request as draft
September 2, 2026 21:48
Coverage Report for CI Build 35155101147Warning No base build found for commit Coverage: 88.969%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
The two "Pytest on Core Dependencies" jobs run poetry install --with dev without --extras server, so fastapi, starlette, arq, cdot and psycopg2 are all absent. tests/lib/test_urn_redirects.py imported mavedb.lib.urn_redirects at module scope, which reaches fastapi and starlette directly and arq, biocommons and cdot through mavedb.deps, so the module failed to import at collection rather than skipping, and one unimportable module ends the whole run: "Interrupted: 1 error during collection". Guard the import the way every sibling module already does. tests/routers/test_urn_redirects.py carried its guards from the start; this one was written without them. Verified under a meta path finder that makes the server extra unimportable: tests/ collects clean and runs 980 passed, 116 skipped, where before it stopped during collection.
davereinhart
marked this pull request as ready for review
September 8, 2026 16:18
bencap
reviewed
Sep 15, 2026
bencap
left a comment
bencap
left a comment
Collaborator
There was a problem hiding this comment.
Looks great! The one blocking item isn't really introduced by your changes, but is something we should fix while we're working on this file.
Comment on lines
+79
to
+102
| def _target_is_public(db: Session, urn: str) -> bool: | ||
| """ | ||
| Report whether the record a redirect points to is one that may be named to any caller. | ||
|
|
||
| A ``Location`` header discloses the URN it carries, to whoever asked -- including an anonymous | ||
| caller, since forwarding happens before a route checks anything. Publication only ever records a | ||
| redirect onto a record it is making public, and nothing in the application returns a published | ||
| record to private, so a private target should not arise; a row written out of band, or by some | ||
| later feature, would be enough for one to. Withholding on anything but a confirmed public record | ||
| keeps that from becoming a disclosure. | ||
|
|
||
| A target that no longer exists is likewise not public: a deleted record leaves its redirect row | ||
| behind, and forwarding to it would answer a permanent redirect with a 404. | ||
|
|
||
| :param db: An active database session. | ||
| :param urn: The URN a redirect points to. | ||
| :return: True only if a record under this URN exists and is public. | ||
| """ | ||
| for urn_re, model in FORWARDING_TARGET_MODELS: | ||
| if urn_re.fullmatch(urn): | ||
| private = db.execute(select(model.private).where(model.urn == urn)).scalar_one_or_none() | ||
| return private is False | ||
|
|
||
| return False |
Collaborator
There was a problem hiding this comment.
This function is re-deriving 'is this record visible to an anonymous caller', which I think has_permission(None, entity, Action.READ) already does for all three possible models here. Using that function in place of this helper would guarantee any future visibility changes would also be reflected here.
Collaborator
Author
There was a problem hiding this comment.
Added this and replaced the loop using FORWARDING_TARGET_MODELS with an if statement to keep it more readable and ensure the target entity is properly typed.
Comment on lines
+139
to
+160
| """ | ||
| Forward a request that names a retired URN to the same resource under its current URN. | ||
|
|
||
| Installed as an application-wide dependency in :mod:`mavedb.server_main`, which is what makes one | ||
| implementation cover every route that takes a URN, sub-resources included: a stale link to a score | ||
| set's scores CSV or mapped variants is forwarded on the same terms as a link to the score set. | ||
|
|
||
| A dependency rather than ASGI middleware, though it sits at the same single point in the request | ||
| path, because it needs the request's database session. Middleware runs outside dependency | ||
| resolution, so it would have to open a session of its own, which no ``dependency_overrides`` could | ||
| redirect and which would therefore reach past the test database. | ||
|
|
||
| ``308`` rather than ``301``: the redirect is permanent, and 308 forbids a client from rewriting the | ||
| request to a GET on the way, which is what makes the header safe to emit for any method. | ||
|
|
||
| Only reads are forwarded. What the issue asks for is that shared *links* keep working, and a write | ||
| is a different proposition: the caller addressed a private draft, and the record now under that URN | ||
| is published, with different rules and a wider audience. ``POST .../publish`` is the sharp case -- | ||
| an owner is permitted to publish a published score set, so forwarding a stale one would rename a | ||
| live public record. A write to a retired URN keeps getting the 404 it gets today, which tells the | ||
| client to look the record up again. | ||
| """ |
Collaborator
There was a problem hiding this comment.
Not blocking, but I might try to place design rationale in the commit message or PR description rather than the docstring. Kind of related to our discussion at standup, I've found the AI agents rather like doing this historical narration in their comments, and I've been trying to cut it more aggressively recently so that docstrings are more precise to what the caller would actually need to know about the function. Here, it's well scoped and well named so a caller wouldn't need to know about much besides the first two and maybe part of the last paragraph.
Collaborator
Author
There was a problem hiding this comment.
Good point. I've noticed this too and the generated comments always seem relevant enough to keep, but agree some of it can move to the commit messages to avoid this type of bloat. I adjusted this one and will probably try to add something to CLAUDE.md to prevent it going forward.
publish_score_set assigned a fresh URN unconditionally: generate_score_set_urn ran on every call, and nothing checked whether the score set was already public. _handle_publish_action permits an owner to publish their own score set without regard to its state, so an owner could POST .../publish a second time and give a live public record a new URN, retiring the one already cited, indexed and shared. refresh_variant_urns then rewrote every variant URN to match. Reject the second call with a 409 before any URN is generated. This gap predates the URN forwarding in cc762ce and is reachable without it, but forwarding is what makes it worth closing now: the redirect table records what each retired URN became, and a repeat publish would fill it with rows retiring permanent URNs, which forwarded_path never matches because it only resolves tmp: shapes. With publication confined to private score sets, every recorded old URN is a temporary one.
_target_is_public decided whether a redirect's target could be named to an anonymous caller by reading the record's private column. That restated a rule the permission layer already owns, and would drift from it silently: a later change to what makes a record readable would have to be made in both places, or the Location header would disclose a URN a route would have withheld. Fetch the record and ask has_permission(None, target, Action.READ), which covers all three kinds a redirect can point at. Behaviour is unchanged for every case under test: a public target forwards, a private one is withheld, and a target that no longer exists is withheld. Dispatching on the URN pattern with the model named in each branch replaces the table of models the loop read from, which also keeps the fetched record concretely typed rather than as Base. Also cuts the design rationale from forward_retired_urns, leaving what a caller acts on. The removed paragraph on why writes are not forwarded argued from an owner being permitted to republish a published score set, which 69e035e no longer allows.
forwarded_path was only ever exercised against a score set target, so the experiment and experiment set branches df88be3 introduced in _target_is_public had no coverage at this level. The router tests reached them end to end, which is a slower signal and does not isolate the lookup. Drive the fixture and the parametrized tests from one ENTITY_TYPES table that names each type once, carrying the URN the fixture gives it, the route its URN sits under, the attribute path reaching it from the score set the fixtures build, and the sub-resource the API serves after its URN. Adding a type is one entry: the fixture iterates the table and every parametrize takes it as its argument source. Retired URNs come from generate_temp_urn rather than a hand-kept list, since a redirect row is unique on the URN it retires. That holds for as long as new types keep arriving through score set publication, which is the only thing that renames a URN today. An entity published by its own workflow would have no attribute path from a score set, so the fixture would need another way to build it. The table comment says so where someone adding an entry will read it. Every route-bearing test now runs for all three types. The sub-resource test covers the two types that have one, since the API serves nothing after an experiment set's URN, and the variant test stays on score sets, since a variant URN is built from a score set's.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a robust mechanism for forwarding requests that use "retired" URNs (Uniform Resource Names) to the current, published URNs for datasets, experiments, and score sets. This ensures that links shared before publication continue to work after a record is published and its URN changes. The implementation covers database schema changes, backend logic, API integration, and tests.
URN Redirects: Database and Model Support
urn_redirectstable (with migration) and corresponding SQLAlchemy modelUrnRedirectto track mappings from old (retired) URNs to new (published) URNs. This enables the system to look up and forward requests for outdated URNs. [1] [2]urn_redirectmodel in the models package for application-wide access.URN Redirect Logic and Integration
record_urn_redirectand related logic inurn_redirects.pyto record URN changes and determine when a request should be forwarded. Only public, published records are eligible as redirect targets, and only read requests (GET/HEAD) are forwarded.API and Application Integration
forward_retired_urnsdependency globally to all FastAPI routes, ensuring that any request using a retired URN is automatically redirected to the current URN if appropriate. [1] [2] [3]Testing
Refs: VariantEffect/mavedb-ui#617