Repository navigation
[DT-4274][risk=low] Add the showcase catalog projection query and eligibility filter (3/3) - #3184
Merged
Merged
Conversation
…ope vocabulary Add ShowcasePublicField, ShowcaseScopeKey and the flat ShowcaseDatasetSummary record, a key-set test that ties the record to the allowlist, and TranslationUtil.summaryCodes for consent codes without free text. Record the field, eligibility and scope decisions in the S1.1 contracts doc. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…irements Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…gibility tests Add DatasetDAO.findShowcaseSummaries: one fixed query that reads named columns and allowlisted dataset/study properties through bound lists, and applies public visibility and the open/external-or-DAC-approved rule in its WHERE clause. Map rows with ShowcaseDatasetSummaryMapper. DAO tests cover access classification with legacy keys, private and missing studies, hard deletes, unapproved properties, type conversion and unsafe URLs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ring.trim Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the JVM time zone Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rializing Gson #3174 removed GsonUtil.buildGsonNullSerializer(). Add getNullSerializingInstance(), which keeps null fields without the registration enum adapter, and point the summary key-set test and the S2.2 requirements at it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pe, use assertNull Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…species and date caveats Older registrations stored the provider link as consentGroup.url, as they did for access management. Read it after url. Drop the unsupported UTC claim for availableDate, and record species as a free-text field that needs a steward decision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
otchet-broad
requested review from
fboulnois and
rushtong
and
a balanced review from Copilot
and removed request for
a team
October 26, 2026 18:50
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The public-field contract still requires data-steward and AppSec sign-off, particularly on exposing unrestricted species text.
No open issues 🎉
What changed in this PR
Implements the S2.1 Data Showcase catalog projection for a future public endpoint. This PR adds the data-access foundation and contract; it introduces no endpoint or caller.
Changes:
- Defines the public-field allowlist, summary record and DAC scope vocabulary.
- Adds an eligibility-filtered query and mapper, codes-only consent summaries and null-preserving serialization.
- Adds regression tests and documents the contract, approval gates and S2.2 integration requirements.
| File | Description |
|---|---|
src/test/java/org/broadinstitute/consent/http/util/gson/GsonUtilTest.java |
Tests preservation of null fields. |
src/test/java/org/broadinstitute/consent/http/models/ShowcaseDatasetSummaryTest.java |
Checks summary fields and serialization against the allowlist. |
src/test/java/org/broadinstitute/consent/http/matching/TranslationUtilTest.java |
Tests consent-code ordering and null input. |
src/test/java/org/broadinstitute/consent/http/db/DatasetShowcaseSummaryDAOTest.java |
Covers eligibility, projection, conversions and ordering. |
src/main/java/org/broadinstitute/consent/http/util/gson/GsonUtil.java |
Adds a null-serializing Gson instance. |
src/main/java/org/broadinstitute/consent/http/models/ShowcaseDatasetSummary.java |
Defines the flat public summary record. |
src/main/java/org/broadinstitute/consent/http/matching/TranslationUtil.java |
Extracts consent codes without free-text values. |
src/main/java/org/broadinstitute/consent/http/enumeration/ShowcaseScopeKey.java |
Defines exact DAC-name scope matching. |
src/main/java/org/broadinstitute/consent/http/enumeration/ShowcasePublicField.java |
Defines field metadata and property allowlists. |
src/main/java/org/broadinstitute/consent/http/db/mapper/ShowcaseDatasetSummaryMapper.java |
Maps summaries and validates counts and provider URLs. |
src/main/java/org/broadinstitute/consent/http/db/DatasetDAO.java |
Adds the public-study and access-eligibility projection query. |
docs/plans/data-showcase-delivery-plan.md |
Updates contract references and the summary example. |
docs/plans/data-showcase-contracts.md |
Documents fields, eligibility, scope and remaining approval gates. |
Review effort: Balanced
rushtong
reviewed
Oct 26, 2026
rushtong
left a comment
rushtong
left a comment
Contributor
There was a problem hiding this comment.
Review notes from a Claude Code pass (high effort). The first two comments are the most important: they affect which data becomes public. The last two are minor cleanup.
…sitive and non-blank properties - Approval counts only with a DAC that exists and is not soft-deleted; dacName ignores soft-deleted DACs. - Allowlisted property names match ignoring case, and blank values are skipped, so an empty older row no longer hides a newer value or the legacy url. - Provider links are checked by authority, so hosts with an underscore or a non-ASCII character are kept; an "@" in the authority is still rejected. - Compute the access value once; state how summaryCodes differs from translateSummary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sh non-ASCII hosts as punycode The authority check accepted any non-blank authority, so links with no host, a bad port, or bidi and homograph characters were published. Keep URI's checks when it parses a host; otherwise parse the host and port, convert the host with IDN.toASCII and require valid labels and a port from 1 to 65535. Lower-case the allowlists with Locale.ROOT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…IDNA deviation characters A whole-host regex overflowed the stack on a very long host. Check each label instead, with the 63 and 253 character limits, on both URI branches. Reject the characters that java.net.IDN (IDNA2003) and browsers (UTS 46) map to different hosts, such as "ß", and accept leading zeros in a port. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rushtong
approved these changes
Oct 27, 2026
rushtong
left a comment
rushtong
left a comment
Contributor
There was a problem hiding this comment.
Thank you for the updates and responses, they all look good. Pending the semgrep fix, 👍🏽
fboulnois
reviewed
Oct 27, 2026
fboulnois
left a comment
fboulnois
left a comment
Contributor
There was a problem hiding this comment.
Can we split this into smaller PRs?
…the provider-link host validHostName checks the ASCII result of IDN.toASCII, and that result is what the URL publishes, so the check runs after the transformation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and scope vocabulary ShowcasePublicField lists each public field with its source, type and nullability; ShowcaseDatasetSummary is the flat record whose components must equal that list; ShowcaseScopeKey fixes the scope vocabulary. The contracts doc records the fields, eligibility and approval gates. Split from #3184 at reviewer request; the query that fills the record stays in #3184. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 27, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
otchet-broad
changed the title
Contributor
Author
|
@fboulnois Split into three stacked PRs:
The merge order is #3189, #3190, then #3184. The code is unchanged. I merged the new branches into this branch instead of rewriting it, so the earlier review threads are kept. |
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eld types against the record Address review on #3190: a PropertyTable per field drives the allowlists, so relabeling a Source cannot change which rows the projection reads. IDENTIFIER and ACCESS_URL are DERIVED. Tests check each field's type against its record component, compare serialized keys as a set and check that property sources read their table. Drop the unused units metadata, and mark the contracts doc's query rules as pending #3184. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… required summary fields Sonar S3776: safeUrl's handling of hosts that java.net.URI does not parse moves to asciiHostUrl, with no change in behavior. Every DAO test result is now checked against ShowcasePublicField.isNullable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Address review on #3190: nothing calls the enum yet, and its SQL column text belongs in the DAO query. The contracts doc keeps the agreed scope vocabulary; S2.2 adds the code with the query that uses it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Base automatically changed from
otchet-dt-4274-contract
to
otchet-dt-4274-utils
October 27, 2026 22:03
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.
Addresses
https://broadworkbench.atlassian.net/browse/DT-4274
Security risk: low — adds a read-only query for a future public endpoint (S2.2). No endpoint or caller is added here.
Summary
Part 3 of 3 of the DT-4274 showcase catalog projection. At reviewer request, the original PR was split: the helpers are in #3189 and the field contract is in #3190. This PR now holds only the query and its mapper. It is stacked on #3190; merge order: #3189, #3190, then this PR. The review threads below are kept. The
TranslationUtilandShowcaseScopeKeythreads now apply to #3189 and #3190.DatasetDAO.findShowcaseSummariesis one fixed SQL block. It reads named columns, plusdataset_propertyandstudy_propertyrows filtered by separate bound allowlists fromShowcasePublicField.ShowcaseDatasetSummaryMapperbuilds theShowcaseDatasetSummaryrecord. Property names match ignoring case and blank values are skipped. Provider links must be absolute http(s) with a valid host, a valid port and no user info; non-ASCII hosts are published in punycode. Participant counts that are not non-negative integers become null. Consent codes come fromTranslationUtil.summaryCodes([DT-4274][risk=low] Add consent-code summaries and a null-serializing Gson (1/3) #3189), which leaves out free text.WHERE: the study must be public, and the dataset must be open/external or approved by a DAC that is not soft-deleted. Access classification followsDataset.getAccessManagement()(current key before legacyconsentGroup.accessManagement, case-insensitive, trimmed). The admin, creator, custodian and no-study exceptions ofDatasetServicedo not apply.IDN.toASCIIin the mapper carries anosemgrepforgitlab.find_sec_bugs.IMPROPER_UNICODE-1.validHostNamechecks the ASCII result, and that result is what the URL publishes, so the check runs after the transformation.The field table that data stewards and AppSec need to approve is now in #3190.
Testing
DatasetShowcaseSummaryDAOTest(64 tests, Testcontainers Postgres, synthetic records) covers open/controlled/external, controlled without DAC approval, private and missing studies, null visibility (rejected by the schema), a hard-deleted dataset, legacy access and URL keys, case-insensitive property names, blank rows, deleted or missing DACs, unknown values, unapproved properties, type conversion, unsafe URLs and ordering. A Claude + Codex cross-review found no high or medium issues; its three low findings are fixed or recorded in the contracts doc. The fixes for the review threads below were cross-reviewed again before pushing.Have you read CONTRIBUTING.md lately? If not, do that first.
🤖 Generated with Claude Code