[DT-4274][risk=low] Define the showcase public fields and summary record (2/3) by otchet-broad · Pull Request #3190 · DataBiosphere/consent

otchet-broad · 2026-10-27T17:40:57Z

Addresses

https://broadworkbench.atlassian.net/browse/DT-4274

Security risk: low — defines the public field list and its record for a future public endpoint (S2.2). Nothing reads them yet.

Summary

Part 2 of 3 of the DT-4274 showcase catalog projection, split from #3184 at reviewer request. Stacked on #3189; merge order: #3189, this PR, then #3184.

ShowcasePublicField names each public field with its source, type, units and nullability.
ShowcaseDatasetSummary is the flat record whose components must equal that list. ShowcaseDatasetSummaryTest fails if a field is added to one and not the other, and checks that the serialized keys (with GsonUtil.getNullSerializingInstance() from [DT-4274][risk=low] Add consent-code summaries and a null-serializing Gson (1/3) #3189) equal ShowcasePublicField.jsonNames().
scope vocabulary: the contracts doc records it (dac, exact DAC names), as DT-4274 asks. The enum moved to S2.2 at review (discussion); S2.2 adds it with the query that uses it and keeps the key-to-column mapping in the DAO query.
Docs: docs/plans/data-showcase-contracts.md records the field table, eligibility, scope vocabulary and the S2.2 requirements. The delivery plan points to it. The eligibility section describes the query in [DT-4274][risk=low] Add the showcase catalog projection query and eligibility filter (3/3) #3184.

Reviewers: data stewards and AppSec

The field table in the contracts doc (section "S2.1 catalog projection") is an open gate. Please approve or change:

consentCodes returns codes only (GRU, HMB, ...), not the translated text.
availableDate is dataset.create_date for open/external and dataset.dac_approval_date otherwise. The columns have no time zone, so the date is UTC only if the writer ran in UTC.
species is free text with no length limit (it could hold an email or a note): approve as is, truncate, or omit?
Omitted: study description, phenotype, PI name, data types, file formats, size.

Testing

ShowcaseDatasetSummaryTest passes locally (5 tests). The code and docs are unchanged from #3184.

Have you read CONTRIBUTING.md lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

🤖 Generated with Claude Code

… 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>

rushtong

Review comments below. The most important one is the allowlist that comes from the Source labels (on ACCESS_URL).

rushtong · 2026-10-27T17:57:45Z

+      true),
+  ACCESS_URL(
+      "accessUrl",
+      Source.DATASET_PROPERTY,


datasetPropertyAllowlist() selects properties by their Source label. But this field's value is computed: the code returns it only for open/external datasets, validates it, and converts the host to punycode. By the javadoc on Source, it is DERIVED.

If someone corrects this label to DERIVED, url and consentGroup.url leave the allowlist. The #3184 query then stops reading them, and accessUrl is always null. Only the hard-coded list in testPropertyAllowlists catches this.

Suggestion: keep the allowlist separate from Source, for example with an explicit readsProperties flag or a separate list. The same label problem applies to IDENTIFIER (line 14): its source is COLUMN, but the code builds the value as DUOS- plus the padded alias.

Fixed in 1ae0900. Each field now names the property table the projection reads (PropertyTable: NONE, DATASET, STUDY), and the allowlists come from that, not from Source. ACCESS_URL and IDENTIFIER are now DERIVED. The DERIVED Javadoc no longer says that raw inputs are never returned: a provider link that passes validation is returned as stored. testPropertySourcesReadTheirTable checks that a field labeled as a property row reads that table. The allowlists are unchanged: [numberOfParticipants, url, consentGroup.url] and [species].

rushtong · 2026-10-27T17:57:46Z

+  @Test
+  void testSerializedKeysEqualAllowlist() {
+    ShowcaseDatasetSummary summary =
+        new ShowcaseDatasetSummary(null, null, null, null, null, null, null, null, null);


This test builds a record in which all fields are null, including identifier, title and consentCodes, which the enum marks nullable=false. The test still passes. No test ties nullable or type to the record components. If participantCount changes to String, or consentCodes becomes null, no test fails, and the approved field table no longer matches the code.

Suggestion: assert that each type string matches the Java type of its record component. Or remove the metadata that no test checks.

In 1ae0900, testRecordComponentTypesMatchAllowlist checks each field's type against its record component. The serialization test leaves null only the fields the allowlist allows. For nullable, #3184's DAO test now asserts (31b7180) on every result that each non-nullable field has a value.

rushtong · 2026-10-27T17:57:46Z

+        new ShowcaseDatasetSummary(null, null, null, null, null, null, null, null, null);
+    JsonObject json = gson.toJsonTree(summary).getAsJsonObject();
+
+    assertEquals(ShowcasePublicField.jsonNames(), List.copyOf(json.keySet()));


This assertion depends on key order. Gson gets the order from Class.getDeclaredFields(), and the Java specification does not define that order. A different JDK can change the order and fail this test, although the record and the enum still match.

Suggestion: compare the keys as sets here. testRecordComponentsEqualAllowlist already checks the order with getRecordComponents(), which has a defined order.

Fixed in 1ae0900. The keys are compared as a set. testRecordComponentsEqualAllowlist still checks the order.

rushtong · 2026-10-27T17:57:46Z

+ * A scope with several keys matches rows that satisfy every key; the values of one key are
+ * alternatives.
+ */
+public enum ShowcaseScopeKey {


Nothing calls this enum, in this PR or in #3184. The contracts doc already records the vocabulary. Can we add the enum in S2.2, together with the query that uses it and its tests?

Agreed, I'll move it to S2.2. The contracts doc keeps the agreed vocabulary (dac, exact DAC names), which is what DT-4274 asks for. The enum will come with the query that uses it and its tests.

rushtong · 2026-10-27T17:57:46Z

+    return jsonName;
+  }
+
+  public String getColumn() {


getColumn() returns SQL text (dac.name). This makes it easy for S2.2 to put the value into a query with @Define or a string template. That moves SQL out of the DAO, where reviewers look for it. It is safe today because the value is a constant. But a later key with a computed column (LOWER(...) or a JSON path) can then go into SQL without review.

Suggestion: keep the map from key to column in the DAO query, and keep only the JSON key in this enum.

Moving the enum to S2.2 removes getColumn() from this PR. In S2.2 the key-to-column mapping will live in the DAO query, as you suggest.

rushtong · 2026-10-27T17:57:46Z

+    return jsonName;
+  }
+
+  public Source getSource() {


Nothing calls getSource(), getType(), getUnits() or isNullable(), in this PR or in #3184. These getters look like the source of truth, but nothing makes sure that they are correct. Remove them, or add a test or doc generator that reads them.

Removed units and getUnits() in 1ae0900. Tests now read the other metadata: getType() in the type test, getSource() and getPropertyTable() in the table test, and isNullable() in #3184's DAO test (31b7180).

rushtong · 2026-10-27T17:57:46Z

+
+Record for [DT-4274](https://broadworkbench.atlassian.net/browse/DT-4274). `ShowcasePublicField`
+defines the fields, `ShowcaseDatasetSummary` returns them in this order, and
+`DatasetDAO.findShowcaseSummaries` reads them.


DatasetDAO.findShowcaseSummaries arrives only in #3184. The merge order is #3189, this PR, then #3184. Between those merges, develop has a doc that describes a method and query rules (case-insensitive matching, the lowest-id row, the 200-identifier cap) that do not exist yet. Move these sentences to #3184, or mark them as pending.

Marked the section as pending in 1ae0900, with a link to #3184. Until #3184 merges, the doc says its query rules describe planned behavior.

rushtong · 2026-10-27T17:57:46Z

+  }
+
+  public static List<String> jsonNames() {
+    return List.of(values()).stream().map(ShowcasePublicField::getJsonName).toList();


Nit: Arrays.stream(values()) does the same job and does not make the extra list. This also applies at line 133.

Fixed in 1ae0900, at both places.

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>

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>

rushtong

Thank you for the updates, they all look great 👍🏽

fboulnois

👍

otchet-broad requested a review from a team as a code owner October 27, 2026 17:40

otchet-broad mentioned this pull request Oct 27, 2026

[DT-4274][risk=low] Add consent-code summaries and a null-serializing Gson (1/3) #3189

Merged

otchet-broad requested review from fboulnois and rushtong and removed request for a team October 27, 2026 17:41

otchet-broad mentioned this pull request Oct 27, 2026

[DT-4274][risk=low] Add the showcase catalog projection query and eligibility filter (3/3) #3184

Merged

rushtong reviewed Oct 27, 2026

View reviewed changes

rushtong self-requested a review October 27, 2026 18:02

otchet-broad and others added 2 commits October 27, 2026 18:24

[DT-4274][risk=low] Merge the #3189 review fixes

0dae68a

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

otchet-broad added a commit that referenced this pull request Oct 27, 2026

[DT-4274][risk=low] Merge the #3189 and #3190 review fixes

f7e5eac

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

otchet-broad changed the title ~~[DT-4274][risk=low] Define the showcase public fields, summary record and scope vocabulary (2/3)~~ [DT-4274][risk=low] Define the showcase public fields and summary record (2/3) Oct 27, 2026

rushtong approved these changes Oct 27, 2026

View reviewed changes

fboulnois approved these changes Oct 27, 2026

View reviewed changes

otchet-broad merged commit 6986500 into otchet-dt-4274-utils Oct 27, 2026
4 checks passed

otchet-broad deleted the otchet-dt-4274-contract branch October 27, 2026 22:03

Conversation

otchet-broad commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Addresses

Summary

Reviewers: data stewards and AppSec

Testing

Uh oh!

rushtong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushtong left a comment

Choose a reason for hiding this comment

Uh oh!

fboulnois left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

otchet-broad commented Oct 27, 2026 •

edited

Loading