[DT-3888] Surface SO authorization model on indexed datasets

[kevinmarete]

Addresses

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

Summary

Datasets now carry the DAC automation state clients need, so duos-ui no longer resolves DAC rules itself — it was issuing one /api/dac/{id}/rules request per unique DAC per page of results and re-deriving the rules in JS.

DatasetTerm gains two fields:

• soApprovalModel (PER_REQUEST | PRE_AUTHORIZED), from whether the DAC has REQUIRE_SO_DAR_APPROVAL enabled.
• instantApprovalEligible, from whether the DAC has an auto-approve rule enabled that this dataset's own data use satisfies.

Eligibility runs the rule implementations themselves rather than restating the policy. The dataset-side half of each rule is split out as datasetQualifies(Dataset), so compare() becomes datasetQualifies(dataset) && requestIsOnlyHMB(...) — unchanged otherwise, and the existing rule tests pass untouched. hasCanonicalSinglePrimary moved to DataUsePrimaryClassifier so applyRule and the indexer share one definition of the shape gate rather than each holding a copy.

This changes which datasets can show the instant-approval badge — measured impact on prod: 3 datasets. The client-side re-derivation matched the translated DataUseSummary and rejected four secondary codes (IRB, COL, GSO, NPU); hasNoModifiers rejects roughly twenty. A GRU dataset carrying a publication moratorium (MOR), a geographic restriction (GS), or required publication of results (PUB) could therefore advertise instant approval that no DAR against it would ever receive.

Measured against prod: 3 datasets lose the badge — DUOS-000151, DUOS-000157 and DUOS-000266, all in DAC 1, all via secondaryOther (free text, rendered as secondary code OTHER, which the old frontend check never looked at). Dev showed 0, so this is confined to one DAC. No approval outcome changes: hasNoModifiers already rejects a non-empty secondaryOther, so compare() returns false for all three today and the engine has never auto-approved them — the UI simply stops claiming otherwise.

One of the three carries a genuine restriction ("Possible restriction on gamete research"). The other two carry an audit note rather than a restriction, and are being excluded from auto-approval today by that stray text — a pre-existing data issue this change surfaces rather than causes, raised separately with @ncalvanese1 and not a blocker here.

Rules are resolved once per index batch via findEnabledRuleAssignments(), returning every enabled (dac_id, rule) pair for AVAILABLE rules in one query. Still DAC-keyed rather than reusing filterDatasetIdsByAutomationRuleType, because indexing walks the entire dataset corpus and a dataset-keyed query would mean an unbounded IN list. A dataset with no DAC resolves to PRE_AUTHORIZED and ineligible without consulting the query at all.

Both fields are left absent, not false, when the rules cannot be resolved, so clients can tell "not eligible" from "not yet known" and render nothing rather than being told the wrong approval process. Two tests assert the serialized document through the same GsonUtil the bulk indexer uses, since those field names and values are duos-ui's only contract here and a rename on either side should fail a test rather than silently blank the Data Library.

Any rule toggle now reindexes that DAC's datasets — every rule affects an indexed field, so the previous rule-type check no longer described anything. Scoped to the DAC rather than the whole corpus, since prod is now mostly external entries with no DAC, which no rule change can affect. findDatasetsAssociatedWithDac rather than findDatasetIdsByDacIds, so datasets carrying the DAC as a dataAccessCommitteeId property are covered too; it returns full rows and its property joins can repeat a dataset, so ids are mapped and deduped. The IN list is bounded by DAC size rather than corpus size.

It runs off the request thread so toggle latency does not track DAC size, and toggles are queued FIFO so DACs reindex in the order they were toggled. A DAC already queued is not queued twice — the pending pass has not started and reads the newest state when it does. Failures are logged rather than raised: the toggle is already committed and audited by the time the reindex runs.

Deploy order matters. This PR, then POST /api/dataset/index, then DataBiosphere/duos-ui#3787. Unlike the SO Approval column, the instant-approval badge is an existing feature, so shipping the UI first would make it vanish rather than degrade.

---

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

[Copilot]

Pull request overview

Adds a Signing Official (SO) approval model onto indexed dataset documents so clients can render SO authorization requirements without issuing per-DAC rule lookups, and wires rule toggles to refresh affected index documents.

Changes:

• Introduces SoApprovalModel (PER_DAR | PRE_AUTHORIZED) and surfaces it on DatasetTerm during indexing.
• Adds a DAC-keyed DAO query to resolve which DACs have REQUIRE_SO_DAR_APPROVAL enabled and resolves it once per indexing batch.
• Triggers best-effort dataset reindexing when REQUIRE_SO_DAR_APPROVAL is toggled for a DAC.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java	Resolves SO approval model during dataset term creation; batch-resolves DAC rule state.
src/main/java/org/broadinstitute/consent/http/models/elastic_search/DatasetTerm.java	Adds soApprovalModel field to indexed dataset term model.
src/main/java/org/broadinstitute/consent/http/enumeration/SoApprovalModel.java	New enum representing SO authorization model.
src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java	Adds DAC-keyed query to find DACs with a rule enabled.
src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java	Reindexes datasets after toggling SO approval rule (best-effort).
src/main/java/org/broadinstitute/consent/http/ConsentModule.java	Wires ElasticSearchService into DACAutomationRuleService provider.
src/main/resources/assets/paths/datasetSearchIndex.yaml	Updates API example payload to include soApprovalModel.
src/main/resources/assets/paths/datasetSearchIndexV2.yaml	Updates streaming search example payload to include soApprovalModel.
src/test/java/org/broadinstitute/consent/http/service/ElasticSearchServiceTest.java	Adds unit tests validating soApprovalModel mapping behavior and failure behavior.
src/test/java/org/broadinstitute/consent/http/service/DACAutomationRuleServiceTest.java	Adds tests to verify reindexing behavior on rule toggles.
src/test/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAOTest.java	Adds DAO test coverage for the new DAC-keyed rule lookup query.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java:139

• Reindexing can involve a large number of datasets for a DAC, but this currently calls elasticSearchService.indexDatasets(datasetIds) in one bulk operation. ElasticSearchService documents indexDatasets as efficient for small batches (<~25) and uses an IN-list query plus a single ES bulk request; for large DACs this risks oversized SQL IN lists, large request payloads, and long request latency/timeouts. Consider batching the datasetIds into smaller chunks and indexing each batch best-effort.

    List<Integer> datasetIds = datasetDAO.findDatasetIdsByDacIds(List.of(dacId));
    if (datasetIds.isEmpty()) {
      return;
    }
    try (Response response = elasticSearchService.indexDatasets(datasetIds)) {

src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java:451

• The Javadoc here says the result is "Empty" when the rule cannot be resolved, but the method returns Optional<Set> where Optional.empty() indicates a lookup failure and an empty Set indicates a successful lookup with no matching DACs. Clarifying this distinction in the comment will avoid misuse.

  /**
   * Ids of DACs that require the Signing Official named in a DAR to approve that request before DAC
   * review. Empty when the rule could not be resolved at all, which is distinct from resolving to
   * no DACs: indexing continues either way, but an unresolved rule must not be reported as an
   * authorization model.
   */

src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java:513

• This inline comment is duplicated/redundant across multiple lines, which makes the intent harder to scan. It can be collapsed into a single concise sentence without changing meaning.

    // Left unset when the rule is unresolved; a dataset with no DAC has no per-DAR step to satisfy
    // A dataset with no DAC has no per-DAR approval step to satisfy, which holds whether or not
    // the rule resolved; only datasets whose model depends on the unresolved rule are left unset

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java:463

• resolveDacIdsRequiringSoDarApproval() uses Optional.of(...) around the DAO result. If findDacIdsWithRuleEnabled(...) ever returns null (e.g., a mock default or unexpected DAO behavior), this will throw a NullPointerException and can break indexing. Treat a null result as an empty set and only use Optional.empty() for the exception path.

  private Optional<Set<Integer>> resolveDacIdsRequiringSoDarApproval() {
    try {
      return Optional.of(
          dacAutomationRuleDAO.findDacIdsWithRuleEnabled(
              DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL));

src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java:139

• Reindexing after toggling REQUIRE_SO_DAR_APPROVAL calls elasticSearchService.indexDatasets(datasetIds) with the full DAC dataset ID list. This list can be large, but ElasticSearchService explicitly documents indexDatasets as intended for small sets (<~25) and provides a sequential path for large lists. Consider batching the IDs to avoid oversized bulk requests / long request times during rule toggles.

    List<Integer> datasetIds = datasetDAO.findDatasetIdsByDacIds(List.of(dacId));
    if (datasetIds.isEmpty()) {
      return;
    }
    try (Response response = elasticSearchService.indexDatasets(datasetIds)) {
      if (response.getStatus() >= 400) {

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java:139

• reindexDatasetsForSoApprovalChange reindexes all datasets for a DAC via a single indexDatasets(datasetIds) call. ElasticSearchService#indexDatasetIds docs indicate indexDatasets is intended for small sets (<~25), and a DAC can have a large number of datasets; passing a large ID list here can lead to an unbounded IN query and high memory use when loading all datasets/studies at once. Consider batching the IDs into smaller chunks before calling indexDatasets (still best-effort, but avoids pathological DAC sizes).

    List<Integer> datasetIds = datasetDAO.findDatasetIdsByDacIds(List.of(dacId));
    if (datasetIds.isEmpty()) {
      return;
    }
    try (Response response = elasticSearchService.indexDatasets(datasetIds)) {
      if (response.getStatus() >= 400) {
        logWarn(
            "Error reindexing datasets after SO approval rule toggle for DAC %d: status %d"
                .formatted(dacId, response.getStatus()));
      }
    } catch (Exception e) {
      logException(
          "Unable to reindex datasets after SO approval rule toggle for DAC %d".formatted(dacId),
          e);
    }

src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java:175

• findDacIdsWithRuleEnabled casts the rules.rule enum column to text (rules.rule::text = :ruleType). Since the underlying DB type is an enum (dac_rule_type), casting the column can prevent index usage and is less type-safe. Prefer casting the bind parameter to the enum instead (or binding it as the enum type) and comparing without casting the column.

      SELECT DISTINCT settings.dac_id
      FROM dac_rule_settings settings
      INNER JOIN dac_automation_rules rules ON rules.id = settings.rule_id
      WHERE rules.rule::text = :ruleType AND rules.state = 'AVAILABLE'
      """)

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java:140

• reindexDatasetsForSoApprovalChange is intended to be best-effort (reindex failures should be logged rather than surfaced), but datasetDAO.findDatasetIdsByDacIds(...) is currently outside the try/catch. If that query throws after the rule toggle has already been committed/audited, the exception will propagate and toggleRule will fail even though the rule change succeeded.

  private void reindexDatasetsForSoApprovalChange(Integer dacId, DACAutomationRule rule) {
    if (!DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL.equals(rule.ruleType())) {
      return;
    }
    List<Integer> datasetIds = datasetDAO.findDatasetIdsByDacIds(List.of(dacId));
    if (datasetIds.isEmpty()) {
      return;
    }

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java:474

• The overload toDatasetTerm(Dataset, Set<Integer>) is only used internally (the only external entry point is toDatasetTerm(Dataset)), so keeping this overload public unnecessarily expands the service API surface and exposes a nullable-parameter contract to other callers.

  public DatasetTerm toDatasetTerm(Dataset dataset, Set<Integer> dacIdsRequiringSoDarApproval) {

src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java:162

• The Javadoc says the rule is enabled solely based on the presence of a dac_rule_settings row, but the query additionally filters rules.state = 'AVAILABLE'. Either drop the state filter or update the Javadoc to match the actual behavior (only AVAILABLE rules are considered enabled for this method).

  /**
   * Ids of every DAC that currently has the given rule enabled. A rule is enabled for a DAC when a
   * dac_rule_settings row exists for that pairing.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java:467

• toDatasetTerm(Dataset) always calls resolveDacIdsRequiringSoDarApproval(), even when dataset.getDacId() is null. That does an unnecessary DAO query (and potentially logs warnings) for datasets that will always resolve to PRE_AUTHORIZED regardless of the rule.

  public DatasetTerm toDatasetTerm(Dataset dataset) {
    return toDatasetTerm(dataset, resolveDacIdsRequiringSoDarApproval().orElse(null));
  }

src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java:463

• resolveDacIdsRequiringSoDarApproval() wraps the DAO result with Optional.of(...). If the DAO ever returns null (e.g., no rows / unexpected mapper behavior), this will throw and be treated as an unresolved rule, causing soApprovalModel to be omitted incorrectly.

  private Optional<Set<Integer>> resolveDacIdsRequiringSoDarApproval() {
    try {
      return Optional.of(
          dacAutomationRuleDAO.findDacIdsWithRuleEnabled(
              DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL));

[jlaw-codes]

Approve and appreciate the clear documentation, thanks!

…

On Wed, Aug 12, 2026 at 5:43 PM Kevin Marete ***@***.***> wrote: *kevinmarete* left a comment (DataBiosphere/consent#3014) <#3014 (comment)> @jlaw-codes <https://github.com/jlaw-codes> — PO sign-off needed on one small user-visible change, should be quick. *What changes.* Three datasets in the Data Library currently show an "instant approval eligible" badge. After this ships they won't. *Why.* The badge was wrong on those three. It's meant to mean "a request for this dataset can be auto-approved without DAC review" — but the system was never going to auto-approve them, because each carries a free-text restriction that no automated rule can evaluate. The badge has been promising something the system wouldn't deliver. This change makes the badge match what actually happens. *What does not change.* No researcher loses access to anything. No request is approved or rejected differently. These three were already going to normal DAC review, and still will. The only difference is the Data Library stops advertising otherwise. *Realistically this is one dataset, not three:* Dataset Restriction text Note DUOS-000151 "Possible restriction on gamete research" A genuine restriction — badge correctly removed DUOS-000157 "Previously categorized with 'other data use terms', now reclassified to GRU" Looks like an audit note, not a restriction DUOS-000266 "Previously categorized with 'other data use terms', now reclassified to GRU" Same The last two are a data-entry issue @ncalvanese1 <https://github.com/ncalvanese1> is looking at separately. If that text is cleared, both would legitimately regain the badge — and would also start being auto-approved, which they should have been all along. All three are in DAC 1, so the blast radius is one DAC. *The ask:* confirm you're OK with the badge being corrected on these three. Happy to walk through it if useful, but the short version is that it removes a promise the system was never keeping. — Reply to this email directly, view it on GitHub <#3014?email_source=notifications&email_token=CD6LX555SDKEQMBWSDXQQWL5JTQHDA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMRXGMYTEOJWGEY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5273129611>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/CD6LX56TWPZ7RIQXA7AA33T5JTQHDAVCNFSNUABEKJSXA33TNF2G64TZHMZDGMJZG4YDSNB3JFZXG5LFHM2TCMRVHEZDGMZYGCQXMAQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rushtong]

Looks good. A couple minor comments inline 👍🏽

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/test/java/org/broadinstitute/consent/http/service/DACAutomationRuleServiceTest.java:378

• This test uses a 1s after() window to assert coalescing; under CI load the second pass may legitimately take >1s to schedule/execute, making the test flaky. Consider increasing the verification window (or using a latch/await on passes) so the test waits deterministically for the second pass.

      // after() waits the full window before verifying, so an uncoalesced third pass fails this;
      // timeout().times(2) would return as soon as it saw the second and never notice
      verify(elasticSearchService, after(1000).times(2)).indexDatasets(List.of(10, 11));
      assertEquals(2, passes.get());

src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java:197

• reindexAllDatasets() fetches all dataset IDs and then calls elasticSearchService.indexDatasets(datasetIds), which ultimately expands WHERE d.dataset_id in (<datasetIds>). For large corpora this can exceed SQL parameter limits and/or create a very large in-memory fetch, making rule toggles operationally risky. Consider chunking the IDs into bounded batches before calling indexDatasets (or switching to a batch/streaming reindex API) so a single toggle can't generate an unbounded IN list.

      List<Integer> datasetIds = datasetDAO.findAllDatasetIds();
      if (datasetIds.isEmpty()) {
        return;
      }
      try (Response response = elasticSearchService.indexDatasets(datasetIds)) {
        if (response.getStatus() >= 400) {

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java:173

• findEnabledRuleAssignments() currently treats any row in dac_rule_settings as enabled. Since dac_rule_settings.user_id/dac_id are nullable (Liquibase schema) and enablement elsewhere is inferred from a non-null enabledByUserId, this query should explicitly filter out rows with null user_id (and ideally null dac_id) to avoid reporting rules as enabled due to incomplete/invalid rows.

  @RegisterRowMapper(DACRuleAssignmentMapper.class)
  @SqlQuery(
      """
      SELECT DISTINCT settings.dac_id, rules.rule
      FROM dac_rule_settings settings
      INNER JOIN dac_automation_rules rules ON rules.id = settings.rule_id
      WHERE rules.state = 'AVAILABLE'
      """)
  List<DACRuleAssignment> findEnabledRuleAssignments();

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

[fboulnois]

👍

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[otchet-broad]

Awesome! Thanks for the changes!