[DT-4061] Promote study registration asset lists to first-class fields - #3055
otchet-broad merged 7 commits into
Conversation
37de009 to
f7668a1
Compare
d169eed to
cd0fa78
Compare
kevinmarete
left a comment
There was a problem hiding this comment.
The promotion skips a key when its value is not an array, but the strip changeset removes that key regardless. For example, an existing unvalidated assets value containing "models": "custom-value" will lose that data. Please retain wrong-shaped values unless a value was successfully promoted or an authoritative first-class property already exists, and add a migration test for a JSON object containing a promoted-name key with a non-array value.
f7668a1 to
a7f8085
Compare
cd0fa78 to
6a3d672
Compare
a7f8085 to
d41a878
Compare
6a3d672 to
223f8ef
Compare
d41a878 to
a1c1f9e
Compare
223f8ef to
415c6ee
Compare
kevinmarete
left a comment
There was a problem hiding this comment.
Here is a finding:
The runtime compatibility handling can discard legacy values that the migration deliberately preserves.
For example, the migration retains an unvalidated value such as "models": "custom-value" because it cannot promote it to an array. However, StudyAssets.assemble() removes the legacy models key and restores it only when it resolves to a nonempty list, so the value disappears from registration and search responses. On the next registration write, promotedValue() ignores the non-collection value while stripPromoted() removes it, deleting the only stored copy.
Please preserve wrong-shaped promoted-name values through both reads and writes, or explicitly migrate/reject them without silently losing data. Add a round-trip test covering a legacy assets object with a non-array value under a promoted key.
a1c1f9e to
4d19126
Compare
415c6ee to
4c1dcff
Compare
4d19126 to
7f7d63e
Compare
4c1dcff to
945bae9
Compare
kevinmarete
left a comment
There was a problem hiding this comment.
One finding:
promotedValue() performs a case-sensitive lookup with assets.get(key), while stripPromoted() removes list-valued keys using equalsIgnoreCase. Consequently, a legacy value such as "Models": [...] is not promoted but is removed from assets, losing the data during a registration update. Please use the same case-insensitive lookup in both paths and add a round-trip test for a differently cased promoted key.
7f7d63e to
086ced7
Compare
945bae9 to
0f27f8c
Compare
086ced7 to
af4c136
Compare
0f27f8c to
2f961bc
Compare
af4c136 to
6e8b06f
Compare
2f961bc to
d38d813
Compare
|
Some minor suggestions identified with Claude:
|
6e8b06f to
76d0c15
Compare
d38d813 to
f30a5ae
Compare
fboulnois
left a comment
There was a problem hiding this comment.
Two security suggestions, identified with Claude and Codex:
- Data exposure: The branch adds
COALESCE(latest_dar.data ->> 'piName', u.display_name) AS pi_name
andi.institution_name(via newLEFT JOIN users/LEFT JOIN institution) to
findSummaryMetricApprovedDARsByDatasetIdIncludesExpired, surfaced through new
DarMetricsSummaryfields. That query backsGET /api/metrics/dar-summaries/{datasetId}, which is
@PermitAll, andMetricsService.generateDarSummariesdoes an existence check only with no user
parameter at all. Confirmed: the resource's@Auth DuosUser useris unused
(@SuppressWarnings("unused")), and every new study-scoped sibling in the same file
(generateStudyDarSummaries,generateStudyResearchOutputs,getSimilarStudies,
getFrequentlyRequestedWith) routes throughrequireStudy→DatasetService.requireReadableStudy.
The legacy dataset-scoped route was given the new PII columns without the gate.
Recommendation: Pass the already-availableUserintogenerateDarSummariesand gate it via
requireReadableStudy(dataset.getStudyId(), user). If the route must stay ungated for
compatibility, droppi_name/institution_namefrom it and expose them only on the gated
study-scoped query. - Broken access control:
patchStudyByIdauthorizes a write with a read predicate.
checkPublicVisibilityForUsernow delegates todatasetService.verifyStudyVisibilityAccess→
canReadStudy, which is
Boolean.TRUE.equals(study.getPublicVisibility()) || isCreatorCustodianOrAdmin(...). On any public
study the first disjunct short-circuits, leaving only the class-level
@RolesAllowed({ADMIN, CHAIRPERSON, DATASUBMITTER}). ContrastdeleteStudyById, which additionally
requires creator-or-admin. The weak gate is pre-existing: diffed against90901c6a, where the
inline version was semantically identical fortrue. What this branch changes is the surface behind
it:StudyPatchgainspiInstitutionId,piOrcid,piLinkedinUrl,piWebsiteUrl.
Recommendation: Use the write predicate the sibling endpoints already use
(isCreatorCustodianOrAdmin), and rename the helper so a read check can't be mistaken for a write
check again.
76d0c15 to
5faafa5
Compare
f30a5ae to
63a41a3
Compare
Adds a study_comment table and the endpoints behind it: GET/POST
/api/dataset/study/{studyId}/comments and DELETE
/api/dataset/study/{studyId}/comments/{commentId}. The list response
carries the comments plus their average rating.
One comment per user per study, enforced by a unique constraint on
(study_id, user_id), so POST upserts rather than accumulating rows. The
rating is constrained to 1-5 in the database as well as in the service, and
posting requires an active researcher — the Researcher role plus a library
card. Deletes only ever remove the caller's own comment; the user id is
part of the DELETE predicate rather than a check before it.
Reads go through DatasetService#verifyStudyVisibilityAccess, so comments
on a study that is not publicly visible are only readable by its creator,
its custodians, and admins — the same rule the study itself uses.
Entirely additive: a new table, new endpoints, and no change to any
existing response. Nothing in the running UI calls these yet.
Comment text has to be sent as a JSON string. isJsonPrimitive is also
true of numbers and booleans, so getAsString stored 123 as "123" and
false as "false"; the type is now asserted the way the rating's already
was, and the parameterized test covers a number, a boolean and an array.
Admins can remove any user's comment. Comments on a visible study are
readable by every authenticated user, and until now the only delete was
the author's own, so there was no moderation path at all: nobody could
remove another user's comment, and an author who lost the Researcher
role could not remove their own. DELETE now allows the Admin role
alongside Researcher and the service picks the scope - deleteAny for an
admin, deleteOwn otherwise - which leaves the role requirement intact
for authors: losing the Researcher role still ends the ability to
delete. A comment the caller may not touch is reported absent rather
than forbidden, so the response does not confirm it exists.
A study's creator and its custodians may rate it, on the same terms as
anyone else - the Researcher role and an active library card - and their
ratings count toward the average. That was already the behavior; it is
now the documented and tested decision rather than a side effect of the
gate.
GET is paged. A study's comment count grows with its readers, so the
unpaged list grew without limit; limit (1-100, default 25) and offset now
bound it, and the response carries a total. averageRating and total are
computed in SQL across every comment rather than over the page, so
neither moves as a caller pages through, and the average no longer
depends on having loaded the whole list into memory.
Comments are capped at 2000 characters, enforced in the service and
declared in the spec. The column stays TEXT deliberately: the limit is a
product decision that should not need a migration to change.
The page carries the caller's own comment alongside it. Paging puts that
comment on an unpredictable page, so a client scanning only the returned
page would offer to add a comment to someone who already has one and then
silently revise it on save. study_comment is unique on (study_id,
user_id), so there is at most one to carry.
post() fetches the row it just wrote by id instead of reloading every
comment on the study and filtering. A comment deleted between the write
and the read now reports as absent rather than reaching orElseThrow and
becoming a 500.
The visibility check no longer loads more of the study than the rule
reads. DatasetService.requireReadableStudy reads the study's own details
and applies the gate; findStudyById additionally opened a REPEATABLE_READ
transaction and fetched dataset ids and the alternative data sharing plan
file on every request. The comment, metrics, and asset services had three
copies of that same method; they now share this one.
Adds the DAO coverage the endpoints were missing: the 1-5 range enforced
by ck_study_comment_rating rather than only by the service, an upsert
preserving create_date while advancing update_date, ordering by creation
so an edit does not move a comment, paging, the study-wide average, and
findById scoped to its study.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds GET /api/metrics/dar-summaries/study/{studyId} and GET
/api/metrics/research-outputs/study/{studyId}. The study-scoped summary
query does the whole study in one round trip rather than re-running the
per-dataset query once per dataset.
A DAR qualifies for a study summary when it was approved on one of the
study's datasets or is a closeout against one of them, and each row is
sourced from the most recently submitted qualifying DAR in its collection.
Sourcing from the collection's newest submission instead would let a
pending progress report — a submitted child DAR with no election of its
own — overwrite the grant's title, RUS, and date and reset its
current/expired chip.
Research outputs aggregate the presentations, publications, and
intellectual properties reported across a study's progress reports.
DarMetricsSummary gains submissionDate, piName, and institutionName.
piName prefers the PI recorded on the DAR itself so the metric stays a
stable record of what was granted, falling back to the submitter for DARs
predating that field; the DAR's own institution field is deprecated and no
longer written, so the submitter's current institution is the only source.
These three fields are also now returned by the existing per-dataset
endpoint — additive only, so existing consumers are unaffected.
Reads go through DatasetService#verifyStudyVisibilityAccess, matching the
study endpoint. No migration.
Note for review: MetricsServiceTest carries an unused
generateDarMetricsSummary(String, Timestamp) overload that can be dropped.
The visibility check uses the shared gate rather than its own copy.
MetricsService had the same requireStudy as the comment and asset
services, each loading more of the study than the rule reads:
findStudyById opens a REPEATABLE_READ transaction and fetches dataset ids
and the alternative data sharing plan file, none of which the check looks
at. DatasetService.requireReadableStudy reads the study's own details and
applies the gate. A study that does not exist and one the caller may not
read now answer identically here, which they already did through the
resource.
The per-dataset route is gated on being able to read the dataset.
generateDarSummaries checked only that the dataset existed and took no
user at all - the resource's @Auth argument was annotated unused - so any
authenticated caller could walk dataset ids and read the project titles
and research use statements of approved DARs. It now takes the user the
resource already had and runs findDatasetByIdForRead, the same
existence-then-visibility rule the other dataset routes apply, which also
still allows a dataset belonging to no study. That costs assembling the
dataset where an id lookup used to do.
The per-dataset query carries no requester identity. Its only caller,
GET /api/metrics/dar-summaries/{datasetId}, is @permitAll and verifies
nothing beyond the dataset existing, so selecting the granted PI's name
and affiliation there would let any authenticated user harvest them by
walking dataset ids. Nothing read those fields from that route - the
study page reads the study-scoped sibling, which is gated on the study's
visibility - so they are selected as null and the two joins that fed them
are gone. The tests that asserted the fallback and the DAR-recorded PI
now assert them through the study route, and a new one pins the
difference between the two.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ions
Adds GET /api/metrics/study-recommendations/{studyId}/similar and GET
/api/metrics/study-recommendations/{studyId}/frequently-requested-with.
Both return at most 12 publicly visible studies with their dataset counts.
"Similar" matches on a shared PI or at least one overlapping data type,
ranked by the size of the data type overlap plus a point for the PI match.
A blank pi_name is not an identity, so it never matches another blank one.
"Frequently requested with" ranks candidate studies by how often they
appeared in the same data access request as the source study. Only
submitted, non-archived, non-progress-report DARs score: a draft cart is
not a request, an archived DAR should stop counting, and a progress report
carries its own reference_id, so counting one would score its parent DAR
more than once.
Both queries restrict candidates to public_visibility = TRUE, so a
recommendation never leaks the existence of a private study, and the source
study itself goes through DatasetService#verifyStudyVisibilityAccess.
No migration and no change to any existing response.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DatasetRegistrationSchemaV1.yaml declared `assets` as client-managed metadata "preserved and returned as-is by the backend; not backend-validated". Backend endpoints and dashboard counts came to depend on individual keys inside it, so that contract no longer held. This promotes the eight lists the backend reads - models, workspaces, presentations, publications, clinicalTrials, intellectualProperties, biospecimens, funding - to top-level registration fields, each stored in its own study_property row, with schemas for the item types. Compatibility is the point of the StudyAssets helper. Reads fall back to the legacy object for a study not yet rewritten, and writes are still accepted through it, so a client that has not moved to the top-level fields keeps working. Registration reads and the search index both still return the full assets object - every promoted list plus whatever unpromoted keys remain - so no consumer sees a shape change. Two details worth review attention. A promoted property that parses is authoritative even when empty, so a submitter who removed the last asset of a type does not get it restored from a stale legacy copy. And "provided" means present, not non-empty: registration reads return each list both top-level and inside `assets`, so an edit clearing the top-level list arrives beside the pre-edit legacy copy, and treating [] as "not provided" would resurrect what was just removed. Migration promotes each array-valued key out of `assets` into its own row, then strips a key only once its value is safely stored elsewhere - either this migration promoted it, or a first-class property already existed. A key whose value is the wrong shape was never promoted, so it stays put: `assets` was unvalidated and free to hold anything, and dropping such a value would lose the only copy of it. An empty list is likewise not promoted and so is retained, which nothing reads either way. Any `assets` row left empty is deleted. The promotion compares against the empty array rather than measuring with jsonb_array_length. The planner may evaluate that before the typeof guard, and the length function raises "cannot get array length of a scalar" on a string or number value - which the migration test now covers with a real one. Note for reviewers who ran the earlier version of this changeset against a local database: the checksum changed, so Liquibase will need a clear or a fresh database. The changeset ids are unchanged. Also corrects IntellectualProperty.filingDate from boolean to string. The runtime path now preserves what the migration preserves. The migration keeps a legacy value under a promoted name that is not a list, because the promotion cannot take it - but assemble() removed the legacy key and restored it only when it resolved to a nonempty list, so the value disappeared from registration and search reads, and stripPromoted() then removed it on the next write while promotedValue() ignored it, deleting the only stored copy. assemble() now leaves such a value exactly as stored, and stripPromoted() removes a promoted key only when its value is a list, which is the same shape test promotedValue() applies. A round-trip test covers a legacy assets object with a non-array value under a promoted key, and fails against the previous behavior. The one combination still not representable is a study holding both a promoted property and a non-list legacy value under the same name: one key cannot carry both. The promoted list wins, being the shape clients expect and the newer intent, and the legacy value is left on disk rather than stripped. The migration does not produce that combination. Every lookup and removal of a promoted name in the legacy assets object ignores case, through one helper. The object is client-supplied, so its casing is not guaranteed, and matching case-insensitively in one place and exactly in another loses data: a list under "Models" was not promoted, because promotedValue looked it up exactly, but was still stripped from the assets object, because the removal matched it case-insensitively. findAssetList's legacy fallback had the same mismatch on the read path. Round-trip tests cover a differently cased promoted key, both list-valued and wrong-shaped. Each migration statement now draws its rows from a MATERIALIZED CTE that filters on the key, so the jsonb casts can only ever see an `assets` row. Left in one WHERE beside `key = 'assets'`, a cast is free to be evaluated first against every study_property row, and the String-typed ones - phenotypeIndication, species, dbGaPPhsID - hold bare text that raises "invalid input syntax for type json" and fails the whole migration. MATERIALIZED rather than a plain CTE because Postgres inlines a CTE by default, which would put the filter back in the same qual list as the cast. A test covers a study whose neighbours hold non-JSON properties. That test does not reproduce the bad plan: idx_study_prop_key, and the planner preferring a cheap equality to an expensive cast, mean the filter runs first in practice, which is why the original was fragility rather than a standing break. The CTEs remove the dependence on that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5faafa5 to
261637c
Compare
63a41a3 to
8c173e5
Compare
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-fields Catches this branch up to its base after #3052, #3053 and #3054 were squash-merged into it. The squashes carry the same content as the original commits still in this branch's history, so every registration list - the Jersey resources, the Guice module, the OpenAPI paths and the changelog includes - conflicted with itself. Each is resolved as the union of both sides, deduplicated. Verified by taking the branch the other way as well: rebasing onto the new base and dropping the three superseded commits produces a byte-identical tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c5bfabf
into
otchet-dt-4061-study-pi-details
Base: branch 5 · 20 files, +1227/-16 · Migration: yes (2 changesets, data rewrite) · UI impact: designed to be none — verify
The riskiest PR in the stack, and the only one that rewrites existing data.
DatasetRegistrationSchemaV1.yamldeclaredassetsas client-managedmetadata "preserved and returned as-is by the backend; not
backend-validated". Backend endpoints and dashboard counts came to depend on
individual keys inside it, so that contract no longer held. This promotes the
eight lists the backend reads —
models,workspaces,presentations,publications,clinicalTrials,intellectualProperties,biospecimens,funding— to top-level registration fields, each stored in its ownstudy_propertyrow, with schemas for the item types.Compatibility is the point of the
StudyAssetshelper. Reads fall back tothe legacy object for a study not yet rewritten, and writes are still
accepted through it, so a client that has not moved to the top-level fields
keeps working. Registration reads and the search index both still return the
full
assetsobject — every promoted list plus whatever unpromoted keysremain — so no consumer sees a shape change.
Two details deserve review attention. A promoted property that parses is
authoritative even when empty, so a submitter who removed the last asset of a
type does not get it restored from a stale legacy copy. And "provided" means
present, not non-empty: registration reads return each list both top-level
and inside
assets, so an edit clearing the top-level list arrives besidethe pre-edit legacy copy, and treating
[]as "not provided" would resurrectwhat was just removed.
Migration promotes each array-valued key out of
assetsinto its own row,then strips the promoted keys and drops any
assetsrow left empty. Guardedto skip non-object and non-array values and to skip a study that already has
the target row.
Rollback hazard: once the strip changeset runs, code from before this
branch can no longer see these lists — they no longer live in
assets.Rolling back the deploy requires restoring the data.
Also corrects
IntellectualProperty.filingDatefrom boolean to string.Depends on: nothing in branches 1-5 — no code dependency on any of them.
Placed last deliberately, because it is the only data rewrite in the stack and
the hardest to roll back. It can be reordered earlier if that suits review.
Where this sits in the DT-4061 stack
otchet-dt-3990-study-ratings-pi-detailswas too large to review meaningfully(95 files, +8168), so it was split into eight PRs. This PR targets
otchet-dt-4061-study-recommendations,not
develop, so its diff shows only its own work.otchet-dt-4061-study-visibility-authzdevelopotchet-dt-4061-study-patch-ownershipotchet-dt-4061-study-visibility-authzotchet-dt-4061-study-pi-detailsotchet-dt-4061-study-visibility-authzotchet-dt-4061-study-commentsotchet-dt-4061-study-pi-detailsotchet-dt-4061-study-dar-metricsotchet-dt-4061-study-commentsotchet-dt-4061-study-recommendationsotchet-dt-4061-study-dar-metricsotchet-dt-4061-study-asset-fields← this PRotchet-dt-4061-study-recommendationsotchet-dt-4061-study-asset-endpointsotchet-dt-4061-study-asset-fieldsBranch 1b is a sibling rather than a link in the chain: nothing depends on it,
so it can be held or dropped without blocking the others. Land the numbered
chain in order — merging out of order, or squash-merging, will require rebasing
the descendants.
The split is verified lossless: branch 7 plus 1b reproduces the original branch
exactly, apart from five
scripts/verify-study-*.shhelper scripts that weredropped at the author's request.
./mvnw test-compilepasses on each branchindependently.
🤖 Generated with Claude Code