feat: add dataset collections - #177
Merged
Merged
Conversation
- Add tests for the authenticated half of derived visibility: a caller with the matching role sees a previously-gated collection and the larger dataset_count/dataset list on a mixed collection. - Scope _accessible_by_collection's dataset query to the resolved collection id on the detail path, so an existing-but-inaccessible collection no longer runs a full dataset-table scan that an unknown slug skips -- closing a timing side channel that could otherwise reveal collection existence.
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
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.
Groups datasets under the study they came from. Motivated by the 71 HTAN datasets recently added to the catalog, which made a flat dataset list unusable — 32 of them are samples from a single breast-cancer study, and there was nowhere to record what a study is or link to its paper.
Backend only. The frontend work is a separate change.
Design
datasets.collection_idis nullable, so datasets predating collections stay valid with no data migration.dataset_countcounts only accessible datasets.user_can_accessstays the single access rule, so the two can never disagree.collection_idso the two paths don't differ measurably in time either.ondelete="SET NULL"ondatasets.collection_id: deleting a collection orphans its datasets, never deletes them.What's here
7a49b55Collectionmodel,datasets.collection_id, Alembic revision68e27d5da1f4d6collectionon dataset responses, eagerly loaded viaouterjoin8d917f6e7de9d4GET /api/collectionsand/api/collections/{slug}5e50e80collection_idon dataset create/updateGET /api/datasetsis unchanged apart from the additivecollectionfield, andcollection_idis null everywhere until a follow-up backfill assigns the HTAN datasets.Testing
make test→ 342 passed (321 before this branch; 21 new). Covers derived visibility for anonymous and role-holding callers, partial access, the null-collection path, migration up and down, and that datasets survive collection deletion.Note: three admin-key tests fail if
ADMIN_API_KEYis exported in your shell — it leaks intoSettings. Runenv -u ADMIN_API_KEY make test. That's pre-existing and unrelated to this branch.Follow-ups, not in this PR
/collections/:slugpage.chat_threads/chat_messagesindexes andDataset.prompt_addendum's type. Deliberately excluded here, but it's real and worth a separate look.