Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
85d178f
[DT-3888] Surface SO authorization model on indexed datasets
kevinmarete Aug 11, 2026
340633e
[DT-3888] Address review: no-DAC fallback, exception logging, OpenAPI…
kevinmarete Aug 12, 2026
9a33245
[DT-3888] Address review: clarify Optional semantics, drop duplicate …
kevinmarete Aug 12, 2026
f23283f
[DT-3888] Address review: drop dead enum scaffolding, cast the bind p…
kevinmarete Aug 12, 2026
e5e7535
[DT-3888] Keep the dataset lookup inside the best-effort reindex try
kevinmarete Aug 12, 2026
e73f961
[DT-3888] Narrow toDatasetTerm overload, document the AVAILABLE filter
kevinmarete Aug 12, 2026
d2fbfda
[DT-3888] Extract the dataset half of each automation rule
kevinmarete Aug 12, 2026
c6c95ee
[DT-3888] Resolve every DAC rule in one query, surface instant approval
kevinmarete Aug 12, 2026
941f8c4
[DT-3888] Reindex all datasets on any rule toggle, off the request th…
kevinmarete Aug 12, 2026
c1b6443
[DT-3888] Release the reindex guard when scheduling is rejected
kevinmarete Aug 12, 2026
50545d4
[DT-3888] Drop the test-only toDatasetTerm overload, guard the reinde…
kevinmarete Aug 13, 2026
d65e59c
[DT-3888] Wait for the follow-up reindex pass instead of a fixed window
kevinmarete Aug 13, 2026
0b4e3ac
[DT-3888] Release the reindex guard in a finally rather than catching…
kevinmarete Aug 13, 2026
6d44739
[DT-3888] Ignore incomplete dac_rule_settings rows when resolving ena…
kevinmarete Aug 13, 2026
0ac8ad2
[DT-3888] Scope the toggle reindex to the DAC, queue toggles FIFO
kevinmarete Aug 13, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,11 @@ private VoteService providesVoteService(
@Provides
@Singleton
private DACAutomationRuleService providesRuleService(
Jdbi jdbi, VoteServiceDAO voteServiceDAO, VoteService voteService) {
return new DACAutomationRuleService(jdbi, voteServiceDAO, voteService);
Jdbi jdbi,
VoteServiceDAO voteServiceDAO,
VoteService voteService,
ElasticSearchService elasticSearchService) {
return new DACAutomationRuleService(jdbi, voteServiceDAO, voteService, elasticSearchService);
}

@Provides
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

import java.time.Instant;
import java.util.List;
import java.util.Set;
import org.broadinstitute.consent.http.db.mapper.DACAutomationRuleAuditMapper;
import org.broadinstitute.consent.http.db.mapper.DACAutomationRuleMapper;
import org.broadinstitute.consent.http.rules.DACAutomationRule;
import org.broadinstitute.consent.http.rules.DACAutomationRuleAudit;
import org.broadinstitute.consent.http.rules.DACAutomationRuleType;
import org.broadinstitute.consent.http.rules.RuleAuditAction;
import org.jdbi.v3.core.Handle;
import org.jdbi.v3.sqlobject.config.RegisterRowMapper;
Expand Down Expand Up @@ -155,6 +157,24 @@ INSERT INTO dac_rule_audit (action, dac_id, rule_id, user_id, action_date)
""")
List<DACAutomationRule> findAllDACAutomationRulesByDACId(@Bind("dacId") int dacId);

/**
* 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.
*
* <p>Deliberately keyed by DAC rather than by dataset — unlike {@code
* DatasetDAO.filterDatasetIdsByAutomationRuleType}, which takes a dataset id list. Indexing walks
* the entire dataset corpus, so a dataset-keyed query would mean an unbounded IN list; the result
* here is bounded by the number of DACs no matter how many datasets are being indexed.
*/
@SqlQuery(
"""
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 = :ruleType::dac_rule_type AND rules.state = 'AVAILABLE'
""")
Set<Integer> findDacIdsWithRuleEnabled(@Bind("ruleType") DACAutomationRuleType ruleType);

@RegisterRowMapper(DACAutomationRuleAuditMapper.class)
@SqlQuery(
"""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.broadinstitute.consent.http.enumeration;

import com.google.gson.annotations.SerializedName;

/**
* Which Signing Official authorization model applies to a dataset, derived from whether the
* dataset's DAC has the REQUIRE_SO_DAR_APPROVAL automation rule enabled. Surfaced on indexed
* datasets so clients do not have to resolve DAC rules themselves.
*
* <p>The wire values are pinned with {@link SerializedName} so renaming a constant cannot silently
* change the published contract.
*/
public enum SoApprovalModel {
/** The SO named in each DAR must approve that request before the DAC reviews it. */
@SerializedName("PER_DAR")
Comment thread
kevinmarete marked this conversation as resolved.
Outdated
PER_DAR,

/** The SO authorizes researchers in advance; no per-request SO approval is needed. */
@SerializedName("PRE_AUTHORIZED")
PRE_AUTHORIZED
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.broadinstitute.consent.http.models.elastic_search;

import java.util.Map;
import org.broadinstitute.consent.http.enumeration.SoApprovalModel;
import org.broadinstitute.consent.http.models.ontology.DataUseSummary;

public class DatasetTerm {
Expand All @@ -24,6 +25,7 @@ public class DatasetTerm {
private UserTerm submitter;
private UserTerm updateUser;
private DacTerm dac;
private SoApprovalModel soApprovalModel;
private Boolean hasInstitutionCertification;
private Map<String, Object> data;

Expand Down Expand Up @@ -171,6 +173,14 @@ public void setDac(DacTerm dac) {
this.dac = dac;
}

public SoApprovalModel getSoApprovalModel() {
return soApprovalModel;
}

public void setSoApprovalModel(SoApprovalModel soApprovalModel) {
this.soApprovalModel = soApprovalModel;
}

public Boolean getHasInstitutionCertification() {
return hasInstitutionCertification;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.inject.Inject;
import jakarta.ws.rs.core.Response;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Date;
Expand Down Expand Up @@ -52,10 +53,15 @@ public class DACAutomationRuleService implements ConsentLogger {
private final VoteDAO voteDAO;
private final VoteService voteService;
private final VoteServiceDAO voteServiceDAO;
private final ElasticSearchService elasticSearchService;

@Inject
public DACAutomationRuleService(
Jdbi jdbi, VoteServiceDAO voteServiceDAO, VoteService voteService) {
Jdbi jdbi,
VoteServiceDAO voteServiceDAO,
VoteService voteService,
ElasticSearchService elasticSearchService) {
this.elasticSearchService = elasticSearchService;
this.dataAccessRequestDAO = jdbi.onDemand(DataAccessRequestDAO.class);
this.datasetDAO = jdbi.onDemand(DatasetDAO.class);
this.ruleDAO = jdbi.onDemand(DACAutomationRuleDAO.class);
Expand Down Expand Up @@ -89,26 +95,54 @@ public List<DACAutomationRule> findAllByDacId(Integer dacId) {
public AutomationRuleToggleResponse toggleRule(Integer dacId, Integer ruleId, User user)
throws ConsentConflictException, UnprocessableEntityException {
List<DACAutomationRule> dacRules = ruleDAO.findAllDACAutomationRulesByDACId(dacId);
Optional<DACAutomationRule> matchingRule =
DACAutomationRule ruleBeingToggled =
dacRules.stream()
.filter(r -> Objects.equals(r.id(), ruleId) && !isNull(r.enabledByUserId()))
.findFirst();
if (matchingRule.isPresent()) {
.filter(r -> Objects.equals(r.id(), ruleId))
.findFirst()
.orElseThrow(() -> new UnprocessableEntityException("Rule ID not found."));
if (!isNull(ruleBeingToggled.enabledByUserId())) {
ruleDAO.auditedDeleteDACRuleSetting(dacId, ruleId, user.getUserId());
reindexDatasetsForSoApprovalChange(dacId, ruleBeingToggled);
return new AutomationRuleToggleResponse(ruleId, false, -1, null, null);
} else {
Optional<DACAutomationRule> optionalRuleBeingUpdated =
dacRules.stream().filter(r -> Objects.equals(r.id(), ruleId)).findFirst();
if (optionalRuleBeingUpdated.isEmpty()) {
throw new UnprocessableEntityException("Rule ID not found.");
}
}
Instant insertTime = Instant.now();
ruleDAO.auditedInsertDACRuleSetting(dacId, ruleId, user.getUserId(), insertTime);
reindexDatasetsForSoApprovalChange(dacId, ruleBeingToggled);
return new AutomationRuleToggleResponse(
ruleId, true, insertTime.toEpochMilli(), user.getDisplayName(), user.getEmail());
}

/**
* Indexed datasets carry their DAC's Signing Official authorization model, so toggling
* REQUIRE_SO_DAR_APPROVAL leaves those documents stale. Reindex failures are logged rather than
* raised: the toggle itself is already committed and audited, and the next reindex corrects the
* documents.
*/
private void reindexDatasetsForSoApprovalChange(Integer dacId, DACAutomationRule rule) {
Comment thread
kevinmarete marked this conversation as resolved.
Outdated
if (!DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL.equals(rule.ruleType())) {
return;
}
// The dataset lookup is inside the try as well: the rule change is already committed and
// audited by this point, so nothing here may fail the toggle
try {
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);
}
}

public Integer removeChairpersonFromDAC(Integer dacId, Integer userId, Integer auditUserId) {
return ruleDAO.auditedDeleteDACRuleSettingByUser(dacId, userId, auditUserId);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,13 @@
import org.apache.http.entity.ContentType;
import org.apache.http.nio.entity.NStringEntity;
import org.broadinstitute.consent.http.configurations.ElasticSearchConfiguration;
import org.broadinstitute.consent.http.db.DACAutomationRuleDAO;
import org.broadinstitute.consent.http.db.DacDAO;
import org.broadinstitute.consent.http.db.DatasetDAO;
import org.broadinstitute.consent.http.db.InstitutionDAO;
import org.broadinstitute.consent.http.db.StudyDAO;
import org.broadinstitute.consent.http.db.UserDAO;
import org.broadinstitute.consent.http.enumeration.SoApprovalModel;
import org.broadinstitute.consent.http.models.Dac;
import org.broadinstitute.consent.http.models.Dataset;
import org.broadinstitute.consent.http.models.DatasetProperty;
Expand All @@ -50,6 +52,7 @@
import org.broadinstitute.consent.http.models.elastic_search.StudyTerm;
import org.broadinstitute.consent.http.models.elastic_search.UserTerm;
import org.broadinstitute.consent.http.models.ontology.DataUseSummary;
import org.broadinstitute.consent.http.rules.DACAutomationRuleType;
import org.broadinstitute.consent.http.service.dao.DatasetServiceDAO;
import org.broadinstitute.consent.http.util.ConsentLogger;
import org.broadinstitute.consent.http.util.gson.GsonUtil;
Expand All @@ -62,6 +65,7 @@ public class ElasticSearchService implements ConsentLogger {
private final RestClient esClient;
private final ElasticSearchConfiguration esConfig;
private final DacDAO dacDAO;
private final DACAutomationRuleDAO dacAutomationRuleDAO;
private final UserDAO userDAO;
private final OntologyService ontologyService;
private final InstitutionDAO institutionDAO;
Expand All @@ -81,6 +85,7 @@ public ElasticSearchService(
this.esClient = esClient;
this.esConfig = esConfig;
this.dacDAO = jdbi.onDemand(DacDAO.class);
this.dacAutomationRuleDAO = jdbi.onDemand(DACAutomationRuleDAO.class);
this.userDAO = jdbi.onDemand(UserDAO.class);
this.ontologyService = ontologyService;
this.institutionDAO = jdbi.onDemand(InstitutionDAO.class);
Expand Down Expand Up @@ -404,8 +409,13 @@ public Response indexDatasets(List<Integer> datasetIds) throws IOException {
}

public Response indexDatasetList(List<Dataset> datasets) throws IOException {
// Resolved once for the whole batch rather than per dataset
Set<Integer> dacIdsRequiringSoDarApproval = resolveDacIdsRequiringSoDarApproval().orElse(null);
List<DatasetTerm> datasetTerms =
datasets.parallelStream().filter(Objects::nonNull).map(this::toDatasetTerm).toList();
datasets.parallelStream()
.filter(Objects::nonNull)
.map(dataset -> toDatasetTerm(dataset, dacIdsRequiringSoDarApproval))
.toList();
if (datasetTerms.isEmpty()) {
return Response.status(Status.NOT_FOUND).build();
}
Expand Down Expand Up @@ -433,7 +443,35 @@ public StreamingOutput indexDatasetIds(List<Integer> datasetIds) {
};
}

/**
* Ids of DACs that require the Signing Official named in a DAR to approve that request before DAC
* review.
*
* <p>{@code Optional.empty()} means the rule could not be resolved at all; an empty {@code Set}
* inside the Optional means it resolved successfully and no DAC has the rule enabled. Indexing
* continues either way, but an unresolved rule must not be reported as an authorization model.
*/
private Optional<Set<Integer>> resolveDacIdsRequiringSoDarApproval() {
try {
return Optional.of(
dacAutomationRuleDAO.findDacIdsWithRuleEnabled(
DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL));
} catch (Exception e) {
logWarn("Unable to resolve DACs requiring SO DAR approval", e);
return Optional.empty();
}
}

public DatasetTerm toDatasetTerm(Dataset dataset) {
Comment thread
kevinmarete marked this conversation as resolved.
Outdated
return toDatasetTerm(dataset, resolveDacIdsRequiringSoDarApproval().orElse(null));
}

/**
* @param dacIdsRequiringSoDarApproval resolved DAC ids, or {@code null} when the rule could not
* be resolved — the SO approval model is then left unset so clients render nothing rather
* than being told the wrong approval process
*/
public DatasetTerm toDatasetTerm(Dataset dataset, Set<Integer> dacIdsRequiringSoDarApproval) {
if (Objects.isNull(dataset)) {
return null;
}
Expand Down Expand Up @@ -472,6 +510,17 @@ public DatasetTerm toDatasetTerm(Dataset dataset) {
term.setDac(toDacTerm(dac));
});

// 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
if (Objects.isNull(dataset.getDacId())) {
term.setSoApprovalModel(SoApprovalModel.PRE_AUTHORIZED);
} else if (Objects.nonNull(dacIdsRequiringSoDarApproval)) {
term.setSoApprovalModel(
dacIdsRequiringSoDarApproval.contains(dataset.getDacId())
? SoApprovalModel.PER_DAR
: SoApprovalModel.PRE_AUTHORIZED);
}

if (Objects.nonNull(dataset.getDataUse())) {
DataUseSummary summary = ontologyService.translateDataUseSummary(dataset.getDataUse());
if (summary != null) {
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/assets/paths/datasetSearchIndex.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ post:
description: Local ethics committee approval is required.
dataLocation: Not Determined
dacId: 3
soApprovalModel: PRE_AUTHORIZED
accessManagement: controlled
Comment thread
kevinmarete marked this conversation as resolved.
study:
description: Test study details
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ post:
- code: NCU
dacId: 2
dacApproval: true
soApprovalModel: PER_DAR
approvedUserIds: [1,2,3]
submitter:
userId: 10
Expand Down
11 changes: 11 additions & 0 deletions src/main/resources/assets/schemas/DatasetSearch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ properties:
dacId:
type: integer
description: The unique identifier for a DAC
soApprovalModel:
type: string
enum:
- PER_DAR
- PRE_AUTHORIZED
description: >-
Which Signing Official authorization model the dataset's DAC uses. PER_DAR means the SO
named in each Data Access Request must approve that request before the DAC reviews it;
PRE_AUTHORIZED means the SO authorizes researchers in advance. Absent when the model
could not be resolved at indexing time, or on documents indexed before this field
existed; clients should treat an absent value as unknown rather than assuming a model.
study:
type: object
title: DatasetSearchStudy
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import java.util.Comparator;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import org.broadinstitute.consent.http.models.User;
import org.broadinstitute.consent.http.rules.DACAutomationRule;
import org.broadinstitute.consent.http.rules.DACAutomationRuleAudit;
Expand All @@ -29,6 +30,47 @@ void testFindAll() {
rules.stream().anyMatch(rule -> rule.ruleType().equals(DACAutomationRuleType.GRU_V1)));
}

@Test
void testFindDacIdsWithRuleEnabled() {
User user = createUser();
Integer enabledDacId = createRandomDAC();
Integer untouchedDacId = createRandomDAC();
DACAutomationRule soRule =
dacAutomationRuleDAO.findAll().stream()
.filter(r -> r.ruleType().equals(DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL))
.findFirst()
.orElseThrow();
dacAutomationRuleDAO.auditedInsertDACRuleSetting(
enabledDacId, soRule.id(), user.getUserId(), Instant.now());

Set<Integer> dacIds =
dacAutomationRuleDAO.findDacIdsWithRuleEnabled(
DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL);

assertTrue(dacIds.contains(enabledDacId));
Assertions.assertFalse(dacIds.contains(untouchedDacId));
}

@Test
void testFindDacIdsWithRuleEnabledIsScopedToTheRequestedRule() {
User user = createUser();
Integer dacId = createRandomDAC();
DACAutomationRule gruRule =
dacAutomationRuleDAO.findAll().stream()
.filter(r -> r.ruleType().equals(DACAutomationRuleType.GRU_V1))
.findFirst()
.orElseThrow();
dacAutomationRuleDAO.auditedInsertDACRuleSetting(
dacId, gruRule.id(), user.getUserId(), Instant.now());

// Enabling an unrelated rule must not mark the DAC as requiring SO DAR approval
Set<Integer> dacIds =
dacAutomationRuleDAO.findDacIdsWithRuleEnabled(
DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL);

Assertions.assertFalse(dacIds.contains(dacId));
}

@Test
void testInsertDACRuleSetting() {
User user = createUser();
Expand Down
Loading
Loading