[DT-4274][risk=low] Add consent-code summaries and a null-serializing Gson (1/3) by otchet-broad · Pull Request #3189 · DataBiosphere/consent

otchet-broad · 2026-10-27T17:38:38Z

Addresses

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

Security risk: low — adds two helper methods. Nothing calls them yet.

Summary

Part 1 of 3 of the DT-4274 showcase catalog projection, split from #3184 at reviewer request. Merge order: this PR, then #3190, then #3184.

TranslationUtil.summaryCodes(DataUse) returns the consent codes of a DataUse (GRU, HMB, DS, OTHER, ...) in translateSummary order, without free-text values or disease labels. Two differences from translateSummary are deliberate and documented in the Javadoc: a code appears once (a primary and a secondary OTHER merge), and DS appears whenever there are disease restrictions, even when no ontology label resolves.
GsonUtil.getNullSerializingInstance() writes null fields as null instead of dropping them, so a response always has the full key set. [DT-4323][risk=no] Write registration enums as their documented values #3174 removed the earlier buildGsonNullSerializer().

The review discussion of summaryCodes is on #3184 (duplication with translateSummary, differences from the DUOS summary).

Testing

TranslationUtilTest (including testSummaryCodesMatchTranslateSummary, which compares both methods for a fully populated DataUse) and GsonUtilTest pass locally. The code is 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

… Gson TranslationUtil.summaryCodes returns the consent codes of a DataUse without free-text values. GsonUtil.getNullSerializingInstance writes null fields as null. Split from #3184 at reviewer request; the showcase catalog projection uses both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The isolated, additive helpers match their documented behavior, include targeted tests, and have no identified blocking issues.

No open issues 🎉

What changed in this PR

Adds two utility helpers for the upcoming showcase catalog projection without changing existing production call paths.

Changes:

Adds ordered, distinct consent-code summaries without free text or disease labels.
Adds a Gson instance that preserves null fields while leaving existing serializers unchanged.
Adds tests for summary-code parity, null input, and null-field serialization.

File	Description
`src/test/java/org/broadinstitute/consent/http/util/gson/GsonUtilTest.java`	Tests null-field preservation and existing serializer behavior.
`src/test/java/org/broadinstitute/consent/http/matching/TranslationUtilTest.java`	Tests summary-code parity and null input.
`src/main/java/org/broadinstitute/consent/http/util/gson/GsonUtil.java`	Adds a reusable null-serializing Gson instance.
`src/main/java/org/broadinstitute/consent/http/matching/TranslationUtil.java`	Adds code-only summaries with documented ordering and deduplication.

_{Review effort: Balanced}

rushtong

Review of the helpers. The code does what the Javadoc says. My main concern is drift between summaryCodes and translateSummary (two copies of the same 19 rules) and the test gaps for the two documented differences. The other comments are minor.

🤖 Generated with Claude Code

rushtong · 2026-10-27T17:59:36Z

+   */
+  // Suppress warning for method complexity (S3776). Mirrors translateSummary.
+  @SuppressWarnings({"java:S3776"})
+  public static List<String> summaryCodes(DataUse dataUse) {


Drift risk. This method copies the 19 conditions of translateSummary by hand. If someone adds a code (for example aiLlmUse) or changes a condition in one method only, the dataset page and the catalog disagree, and no compile error or test failure shows it.

Suggestion: put the rules in one table (predicate, code, description) and let both methods read it. Then summaryCodes cannot drift from translateSummary.

Fixed in 67ce583. translateSummary and summaryCodes now read one SUMMARY_RULES table. Each row holds the code, primary or secondary, the condition and the description. A code added to the table appears in both methods.

rushtong · 2026-10-27T17:59:36Z

+  // Suppress warning for method complexity (S3776). Mirrors translateSummary.
+  @SuppressWarnings({"java:S3776"})
+  public static List<String> summaryCodes(DataUse dataUse) {
+    Set<String> codes = new LinkedHashSet<>();


Nit: create the set after the null check. For a null DataUse, the method creates a set that it never uses.

Fixed in 67ce583. summaryCodes returns for a null DataUse before it builds anything. It is now a stream over SUMMARY_RULES.

rushtong · 2026-10-27T17:59:36Z

+      return List.of();
+    }
+    if (BooleanUtils.isTrue(dataUse.getGeneralUse())) {
+      codes.add("GRU");


Nit: the code strings ("GRU", "DS", "OTHER", "POP-M", ...) are literals here and in translateSummary. Catalog clients can filter on these values, so they are an API contract. Shared constants (or an enum) prevent a typo or a rename in one place only.

In 67ce583, each code string is now written once, in SUMMARY_RULES, which both methods read. OTHER is used by two rows, so it is a constant (this also clears Sonar S1192). I did not add a public enum yet, because no client reads these codes. S2.2 can promote them if the API needs one.

rushtong · 2026-10-27T17:59:36Z

+      codes.add("GRU");
+    }
+    if (dataUse.getDiseaseRestrictions() != null && !dataUse.getDiseaseRestrictions().isEmpty()) {
+      codes.add("DS");


The Javadoc says this difference is deliberate, but please confirm the product intent. For a disease ID that does not resolve (bad or retired ID, or an ontology lookup that returns nothing), the catalog shows DS but the dataset summary shows no DS row. Users then see two different data-use statements for one dataset.

Yes, this is intended. DS means the dataset has disease restrictions, and that is true whether or not the ontology service returns a label. Hiding DS would make a restricted dataset look less restricted in a public catalog. The dataset page leaves the row out only because it has no label to print. testSummaryCodesKeepDSWithoutLabel now pins this. I'll confirm with the PO during the S2.1 field review.

rushtong · 2026-10-27T17:59:37Z

+    if (BooleanUtils.isTrue(dataUse.getPopulation())) {
+      codes.add("POP");
+    }
+    return List.copyOf(codes);


DataUse has fields that neither this method nor translateSummary maps to a code: aiLlmUse, illegalBehavior, sexualDiseases, stigmatizeDiseases, vulnerablePopulations, psychologicalTraits, notHealth. The gap already exists, but this PR puts it on a new public surface. For example, a dataset that prohibits AI/LLM use shows no matching code in the catalog. Is that intended for the showcase?

Not decided for the showcase yet. These seven fields have no code in translateSummary either, so today the catalog and the dataset page agree. The new test lists them in FIELDS_WITHOUT_CODE and fails when a new DataUse field has no code and is not listed. I'll add this question to the steward and AppSec review of the S2.1 fields. If they want a code (for example for aiLlmUse), it is one row in SUMMARY_RULES.

rushtong · 2026-10-27T17:59:37Z

+  void testSummaryCodesMatchTranslateSummary(String gender) {
+    OntologyTerm term = initializeDiseaseTerm();
+    DataUse dataUse =
+        new DataUseBuilder()


This parity test sets a fixed list of builder fields. If someone adds a branch for a new field to translateSummary, this builder does not set that field. Both methods then omit the code, and the test still passes while real data disagrees. One option: a test that sets every DataUse field (by reflection), so that a new field makes the test fail.

Added in 67ce583: testEveryDataUseFieldHasACodeOrIsListed. It sets each DataUse field on its own by reflection and checks that both methods return the same codes. It fails if a field gives no code and is not in FIELDS_WITHOUT_CODE. I checked that it fails when notHealth is removed from that list.

rushtong · 2026-10-27T17:59:37Z

+  }
+
+  @Test
+  void testSummaryCodesNullDataUse() {


Please add tests for the two documented differences:

DS when no ontology label resolves (stub findByTermIds to return an empty list) — expect DS in summaryCodes and no DS in translateSummary.

Only secondaryOther set, and both other and secondaryOther set — expect one OTHER in the correct position.

Now the only test uses a label that resolves, and it checks the merge only through distinct().

Added in 67ce583: testSummaryCodesKeepDSWithoutLabel (blank label: DS in summaryCodes, nothing in translateSummary), testSummaryCodesSecondaryOtherOnly, and testSummaryCodesMergePrimaryAndSecondaryOther ([HMB, OTHER, NMDS]).

rushtong · 2026-10-27T17:59:37Z

          .registerTypeAdapterFactory(new JsonValueEnumTypeAdapterFactory())
          .create();

+  private static final Gson NULL_SERIALIZING_INSTANCE =


Minor: this is the third Gson variant. It differs from REGISTRATION_INSTANCE only by the @JsonValue enum adapter. Please say in the Javadoc when to use each one. If a caller serializes an enum with this instance, it gets the Java name, not the documented value.

In 67ce583, added Javadoc to getInstance and getNullSerializingInstance. It says which one to use when, and that only getRegistrationInstance writes enums by their @JsonValue.

fboulnois

👍

…d summaryCodes Address review on #3189: both methods now read SUMMARY_RULES, so a code cannot be added to one and not the other. A reflection test fails when a DataUse field has no code and is not listed as unmapped. New tests pin DS without an ontology label and the OTHER merge. GsonUtil documents when to use each instance. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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

rushtong

Thanks for the updates, looks great 👍🏽

…ord (2/3) (#3190) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

…gibility filter (3/3) (#3184) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

Sonar java:S1192: "string" appeared six times in ShowcasePublicField. The JSON type names move to a private JsonType holder class, because enum constants cannot refer to the enum's own static fields. Every getType() value is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

sonarqubecloud · 2026-10-27T23:37:04Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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

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

Copilot started reviewing on behalf of otchet-broad October 27, 2026 17:39

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

This was referenced Oct 27, 2026

[DT-4274][risk=low] Define the showcase public fields and summary record (2/3) #3190

Merged

[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

fboulnois approved these changes Oct 27, 2026

View reviewed changes

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

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

rushtong approved these changes Oct 27, 2026

View reviewed changes

otchet-broad and others added 3 commits October 27, 2026 18:03

[DT-4274][risk=low] Define the showcase public fields and summary rec…

6986500

…ord (2/3) (#3190) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

[DT-4274][risk=low] Add the showcase catalog projection query and eli…

62ab030

…gibility filter (3/3) (#3184) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

otchet-broad merged commit 2ed9be0 into develop Oct 28, 2026
11 checks passed

otchet-broad deleted the otchet-dt-4274-utils branch October 28, 2026 00:32

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

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

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!

fboulnois left a comment

Choose a reason for hiding this comment

Uh oh!

rushtong left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Oct 27, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

otchet-broad commented Oct 27, 2026 •

edited

Loading