diff --git a/src/main/java/org/broadinstitute/consent/http/ConsentModule.java b/src/main/java/org/broadinstitute/consent/http/ConsentModule.java index 524058fef0..a30f118e01 100644 --- a/src/main/java/org/broadinstitute/consent/http/ConsentModule.java +++ b/src/main/java/org/broadinstitute/consent/http/ConsentModule.java @@ -655,8 +655,13 @@ 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, + ExecutorService executorService) { + return new DACAutomationRuleService( + jdbi, voteServiceDAO, voteService, elasticSearchService, executorService); } @Provides diff --git a/src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java b/src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java index ccf58b41e5..9c1320e76f 100644 --- a/src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java +++ b/src/main/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAO.java @@ -4,8 +4,10 @@ import java.util.List; import org.broadinstitute.consent.http.db.mapper.DACAutomationRuleAuditMapper; import org.broadinstitute.consent.http.db.mapper.DACAutomationRuleMapper; +import org.broadinstitute.consent.http.db.mapper.DACRuleAssignmentMapper; import org.broadinstitute.consent.http.rules.DACAutomationRule; import org.broadinstitute.consent.http.rules.DACAutomationRuleAudit; +import org.broadinstitute.consent.http.rules.DACRuleAssignment; import org.broadinstitute.consent.http.rules.RuleAuditAction; import org.jdbi.v3.core.Handle; import org.jdbi.v3.sqlobject.config.RegisterRowMapper; @@ -155,6 +157,26 @@ INSERT INTO dac_rule_audit (action, dac_id, rule_id, user_id, action_date) """) List findAllDACAutomationRulesByDACId(@Bind("dacId") int dacId); + /** + * Every enabled DAC-to-rule pairing, in one pass. Keyed by DAC rather than by dataset — unlike + * {@code DatasetDAO.filterDatasetIdsByAutomationRuleType} — so indexing the whole corpus does not + * need an unbounded IN list. + * + *

Both columns are nullable, so both are checked: a settings row naming no user is what + * findAllDACAutomationRulesByDACId reports as disabled, and a null dac_id has no pairing to key. + */ + @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' + AND settings.dac_id IS NOT NULL + AND settings.user_id IS NOT NULL + """) + List findEnabledRuleAssignments(); + @RegisterRowMapper(DACAutomationRuleAuditMapper.class) @SqlQuery( """ diff --git a/src/main/java/org/broadinstitute/consent/http/db/mapper/DACRuleAssignmentMapper.java b/src/main/java/org/broadinstitute/consent/http/db/mapper/DACRuleAssignmentMapper.java new file mode 100644 index 0000000000..e37cd40397 --- /dev/null +++ b/src/main/java/org/broadinstitute/consent/http/db/mapper/DACRuleAssignmentMapper.java @@ -0,0 +1,17 @@ +package org.broadinstitute.consent.http.db.mapper; + +import java.sql.ResultSet; +import java.sql.SQLException; +import org.broadinstitute.consent.http.rules.DACAutomationRuleType; +import org.broadinstitute.consent.http.rules.DACRuleAssignment; +import org.jdbi.v3.core.mapper.RowMapper; +import org.jdbi.v3.core.statement.StatementContext; + +public class DACRuleAssignmentMapper implements RowMapper { + + @Override + public DACRuleAssignment map(ResultSet rs, StatementContext ctx) throws SQLException { + return new DACRuleAssignment( + rs.getInt("dac_id"), DACAutomationRuleType.valueOf(rs.getString("rule"))); + } +} diff --git a/src/main/java/org/broadinstitute/consent/http/enumeration/SoApprovalModel.java b/src/main/java/org/broadinstitute/consent/http/enumeration/SoApprovalModel.java new file mode 100644 index 0000000000..b1d89ea30f --- /dev/null +++ b/src/main/java/org/broadinstitute/consent/http/enumeration/SoApprovalModel.java @@ -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. + * + *

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 access request must approve that request before the DAC reviews it. */ + @SerializedName("PER_REQUEST") + PER_REQUEST, + + /** The SO authorizes researchers in advance; no per-request SO approval is needed. */ + @SerializedName("PRE_AUTHORIZED") + PRE_AUTHORIZED +} diff --git a/src/main/java/org/broadinstitute/consent/http/models/datause/DataUsePrimaryClassifier.java b/src/main/java/org/broadinstitute/consent/http/models/datause/DataUsePrimaryClassifier.java index 8d392fe41c..f0a3bb9692 100644 --- a/src/main/java/org/broadinstitute/consent/http/models/datause/DataUsePrimaryClassifier.java +++ b/src/main/java/org/broadinstitute/consent/http/models/datause/DataUsePrimaryClassifier.java @@ -9,6 +9,21 @@ public final class DataUsePrimaryClassifier { private DataUsePrimaryClassifier() {} + /** + * Whether a Data Use has the single-primary shape DAC automation supports. {@code Shape.SINGLE} + * also covers an Other-only primary category, which is non-canonical and excluded here to match + * the abstention policy in {@code DataUseMatcherV5}. Shared by the approval engine and dataset + * indexing, which both gate on it before consulting a rule. + */ + public static boolean hasCanonicalSinglePrimary(DataUse dataUse) { + if (dataUse == null) { + return false; + } + DataUsePrimaryClassification classification = classify(dataUse); + return classification.shape() == DataUsePrimaryClassification.Shape.SINGLE + && !classification.categories().contains(DataUsePrimaryCategory.OTHER); + } + public static DataUsePrimaryClassification classify(DataUse dataUse) { return classify( dataUse.getGeneralUse(), diff --git a/src/main/java/org/broadinstitute/consent/http/models/elastic_search/DatasetTerm.java b/src/main/java/org/broadinstitute/consent/http/models/elastic_search/DatasetTerm.java index 1cbf73de0b..ff09a94a10 100644 --- a/src/main/java/org/broadinstitute/consent/http/models/elastic_search/DatasetTerm.java +++ b/src/main/java/org/broadinstitute/consent/http/models/elastic_search/DatasetTerm.java @@ -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 { @@ -24,6 +25,8 @@ public class DatasetTerm { private UserTerm submitter; private UserTerm updateUser; private DacTerm dac; + private SoApprovalModel soApprovalModel; + private Boolean instantApprovalEligible; private Boolean hasInstitutionCertification; private Map data; @@ -171,6 +174,22 @@ public void setDac(DacTerm dac) { this.dac = dac; } + public SoApprovalModel getSoApprovalModel() { + return soApprovalModel; + } + + public void setSoApprovalModel(SoApprovalModel soApprovalModel) { + this.soApprovalModel = soApprovalModel; + } + + public Boolean getInstantApprovalEligible() { + return instantApprovalEligible; + } + + public void setInstantApprovalEligible(Boolean instantApprovalEligible) { + this.instantApprovalEligible = instantApprovalEligible; + } + public Boolean getHasInstitutionCertification() { return hasInstitutionCertification; } diff --git a/src/main/java/org/broadinstitute/consent/http/rules/DACRuleAssignment.java b/src/main/java/org/broadinstitute/consent/http/rules/DACRuleAssignment.java new file mode 100644 index 0000000000..39b8696acd --- /dev/null +++ b/src/main/java/org/broadinstitute/consent/http/rules/DACRuleAssignment.java @@ -0,0 +1,4 @@ +package org.broadinstitute.consent.http.rules; + +/** A single DAC-to-rule pairing: this DAC currently has this automation rule enabled. */ +public record DACRuleAssignment(Integer dacId, DACAutomationRuleType ruleType) {} diff --git a/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseV1.java b/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseV1.java index df5aba6703..7208d3a859 100644 --- a/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseV1.java +++ b/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseV1.java @@ -6,9 +6,12 @@ public class GeneralResearchUseV1 implements RuleImplementationInterface { public boolean compare(Dataset dataset, DataAccessRequest dataAccessRequest) { - return Boolean.TRUE.equals(dataset.getDataUse().getGeneralUse()) - && hasNoModifiers(dataset.getDataUse()) - && requestIsOnlyHMB(dataAccessRequest.getData()); + return datasetQualifies(dataset) && requestIsOnlyHMB(dataAccessRequest.getData()); + } + + @Override + public boolean datasetQualifies(Dataset dataset) { + return datasetIsUnmodifiedGeneralUse(dataset); } public DACAutomationRuleType getRuleType() { diff --git a/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseWithDiseaseSpecificV1.java b/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseWithDiseaseSpecificV1.java index 776c367a9d..dede45bfee 100644 --- a/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseWithDiseaseSpecificV1.java +++ b/src/main/java/org/broadinstitute/consent/http/rules/GeneralResearchUseWithDiseaseSpecificV1.java @@ -12,8 +12,11 @@ public DACAutomationRuleType getRuleType() { @Override public boolean compare(Dataset dataset, DataAccessRequest dataAccessRequest) { - return Boolean.TRUE.equals(dataset.getDataUse().getGeneralUse()) - && hasNoModifiers(dataset.getDataUse()) - && requestHasDiseases(dataAccessRequest.getData()); + return datasetQualifies(dataset) && requestHasDiseases(dataAccessRequest.getData()); + } + + @Override + public boolean datasetQualifies(Dataset dataset) { + return datasetIsUnmodifiedGeneralUse(dataset); } } diff --git a/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalV1.java b/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalV1.java index b662876177..9fc44cd9d1 100644 --- a/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalV1.java +++ b/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalV1.java @@ -12,8 +12,11 @@ public DACAutomationRuleType getRuleType() { @Override public boolean compare(Dataset dataset, DataAccessRequest dataAccessRequest) { - return Boolean.TRUE.equals(dataset.getDataUse().getHmbResearch()) - && hasNoModifiers(dataset.getDataUse()) - && requestIsOnlyHMB(dataAccessRequest.getData()); + return datasetQualifies(dataset) && requestIsOnlyHMB(dataAccessRequest.getData()); + } + + @Override + public boolean datasetQualifies(Dataset dataset) { + return datasetIsUnmodifiedHmbResearch(dataset); } } diff --git a/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalWithDiseaseSpecificV1.java b/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalWithDiseaseSpecificV1.java index dadddfde86..06bc865a9c 100644 --- a/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalWithDiseaseSpecificV1.java +++ b/src/main/java/org/broadinstitute/consent/http/rules/HealthMedicalBioMedicalWithDiseaseSpecificV1.java @@ -12,8 +12,11 @@ public DACAutomationRuleType getRuleType() { @Override public boolean compare(Dataset dataset, DataAccessRequest dataAccessRequest) { - return Boolean.TRUE.equals(dataset.getDataUse().getHmbResearch()) - && hasNoModifiers(dataset.getDataUse()) - && requestHasDiseases(dataAccessRequest.getData()); + return datasetQualifies(dataset) && requestHasDiseases(dataAccessRequest.getData()); + } + + @Override + public boolean datasetQualifies(Dataset dataset) { + return datasetIsUnmodifiedHmbResearch(dataset); } } diff --git a/src/main/java/org/broadinstitute/consent/http/rules/RuleImplementationInterface.java b/src/main/java/org/broadinstitute/consent/http/rules/RuleImplementationInterface.java index 70e98bada2..589175b0ca 100644 --- a/src/main/java/org/broadinstitute/consent/http/rules/RuleImplementationInterface.java +++ b/src/main/java/org/broadinstitute/consent/http/rules/RuleImplementationInterface.java @@ -12,6 +12,33 @@ public interface RuleImplementationInterface { boolean compare(Dataset dataset, DataAccessRequest dataAccessRequest); + /** + * Whether the dataset's own data use qualifies it for automatic approval under this rule, + * independent of any request. {@link #compare} layers the request-side conditions on top, and + * dataset indexing applies this half alone. Rules that never auto-approve return false. + */ + default boolean datasetQualifies(Dataset dataset) { + return false; + } + + /** Reached during indexing for datasets that may carry no data use, so absence is a non-match. */ + default boolean datasetIsUnmodifiedGeneralUse(Dataset dataset) { + DataUse dataUse = dataset.getDataUse(); + return dataUse != null + && Boolean.TRUE.equals(dataUse.getGeneralUse()) + && hasNoModifiers(dataUse); + } + + /** + * @see #datasetIsUnmodifiedGeneralUse + */ + default boolean datasetIsUnmodifiedHmbResearch(Dataset dataset) { + DataUse dataUse = dataset.getDataUse(); + return dataUse != null + && Boolean.TRUE.equals(dataUse.getHmbResearch()) + && hasNoModifiers(dataUse); + } + default boolean hasNoModifiers(DataUse data) { if (Boolean.TRUE.equals(data.getCollaboratorRequired())) { return false; diff --git a/src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java b/src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java index 1e149876ea..df7762c0e4 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java +++ b/src/main/java/org/broadinstitute/consent/http/service/DACAutomationRuleService.java @@ -4,12 +4,17 @@ 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; +import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.Set; +import java.util.concurrent.ExecutorService; import org.broadinstitute.consent.http.db.DACAutomationRuleDAO; import org.broadinstitute.consent.http.db.DataAccessRequestDAO; import org.broadinstitute.consent.http.db.DatasetDAO; @@ -23,12 +28,9 @@ import org.broadinstitute.consent.http.exceptions.UnprocessableEntityException; import org.broadinstitute.consent.http.models.AutomationRuleToggleResponse; import org.broadinstitute.consent.http.models.DataAccessRequest; -import org.broadinstitute.consent.http.models.DataUse; import org.broadinstitute.consent.http.models.Dataset; import org.broadinstitute.consent.http.models.User; import org.broadinstitute.consent.http.models.Vote; -import org.broadinstitute.consent.http.models.datause.DataUsePrimaryCategory; -import org.broadinstitute.consent.http.models.datause.DataUsePrimaryClassification.Shape; import org.broadinstitute.consent.http.models.datause.DataUsePrimaryClassifier; import org.broadinstitute.consent.http.rules.AuditPageResults; import org.broadinstitute.consent.http.rules.DACAutomationRule; @@ -52,10 +54,26 @@ public class DACAutomationRuleService implements ConsentLogger { private final VoteDAO voteDAO; private final VoteService voteService; private final VoteServiceDAO voteServiceDAO; + private final ElasticSearchService elasticSearchService; + private final ExecutorService executorService; + + /** Guards the queue and the running flag; see {@link #reindexDatasetsForRuleChange}. */ + private final Object reindexLock = new Object(); + + /** DACs awaiting a reindex, in toggle order. A Set so a DAC queued twice is reindexed once. */ + private final Set pendingDacIds = new LinkedHashSet<>(); + + private boolean reindexRunning = false; @Inject public DACAutomationRuleService( - Jdbi jdbi, VoteServiceDAO voteServiceDAO, VoteService voteService) { + Jdbi jdbi, + VoteServiceDAO voteServiceDAO, + VoteService voteService, + ElasticSearchService elasticSearchService, + ExecutorService executorService) { + this.elasticSearchService = elasticSearchService; + this.executorService = executorService; this.dataAccessRequestDAO = jdbi.onDemand(DataAccessRequestDAO.class); this.datasetDAO = jdbi.onDemand(DatasetDAO.class); this.ruleDAO = jdbi.onDemand(DACAutomationRuleDAO.class); @@ -89,26 +107,113 @@ public List findAllByDacId(Integer dacId) { public AutomationRuleToggleResponse toggleRule(Integer dacId, Integer ruleId, User user) throws ConsentConflictException, UnprocessableEntityException { List dacRules = ruleDAO.findAllDACAutomationRulesByDACId(dacId); - Optional 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()); + reindexDatasetsForRuleChange(dacId); return new AutomationRuleToggleResponse(ruleId, false, -1, null, null); - } else { - Optional 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); + reindexDatasetsForRuleChange(dacId); return new AutomationRuleToggleResponse( ruleId, true, insertTime.toEpochMilli(), user.getDisplayName(), user.getEmail()); } + /** + * Indexed datasets carry state derived from their DAC's automation rules, so a toggle leaves that + * DAC's documents stale. Only they are reindexed — most of the corpus is external entries with no + * DAC, which no rule change can affect. It runs off the request thread so toggle latency does not + * track DAC size. + * + *

Queued FIFO, so DACs are reindexed in the order they were toggled. A DAC already queued is + * not queued twice: the pending pass has not started and will read the newest state when it does. + */ + private void reindexDatasetsForRuleChange(Integer dacId) { + synchronized (reindexLock) { + pendingDacIds.add(dacId); + if (reindexRunning) { + return; + } + reindexRunning = true; + } + try { + executorService.submit(this::drainPendingReindexes); + } catch (RuntimeException e) { + // Released so a rejected submission does not leave every later toggle queueing behind a drain + // that never runs. The queue keeps its entries, so the next toggle picks this one up. + synchronized (reindexLock) { + reindexRunning = false; + } + logException("Unable to schedule dataset reindex after DAC rule toggle", e); + } + } + + /** + * Reindexes queued DACs until the queue is empty. The queue and {@code reindexRunning} are read + * and written under {@code reindexLock}, so a toggle cannot have its DAC dropped by clearing it. + */ + private void drainPendingReindexes() { + boolean released = false; + try { + while (true) { + Integer dacId; + synchronized (reindexLock) { + Iterator queued = pendingDacIds.iterator(); + if (!queued.hasNext()) { + reindexRunning = false; + released = true; + return; + } + dacId = queued.next(); + queued.remove(); + } + reindexDatasetsForDac(dacId); + } + } finally { + // Reached only when an Error escapes reindexDatasetsForDac, which catches every Exception. + // The guard must still be released; the flag is local because another drain may own it by + // now. + if (!released) { + synchronized (reindexLock) { + reindexRunning = false; + } + logWarn("Dataset reindex after DAC rule toggle terminated unexpectedly"); + } + } + } + + /** + * Failures are logged rather than raised: the toggle that triggered this is already committed and + * audited, so nothing here may fail it, and the next reindex corrects the documents either way. + */ + private void reindexDatasetsForDac(Integer dacId) { + try { + // Covers datasets carrying the DAC as a property as well as those assigned to it directly + List datasetIds = + datasetDAO.findDatasetsAssociatedWithDac(dacId).stream() + .map(Dataset::getDatasetId) + .distinct() + .toList(); + if (datasetIds.isEmpty()) { + return; + } + try (Response response = elasticSearchService.indexDatasets(datasetIds)) { + if (response.getStatus() >= 400) { + logWarn( + "Error reindexing datasets for DAC %d after rule toggle: status %d" + .formatted(dacId, response.getStatus())); + } + } + } catch (Exception e) { + logException("Unable to reindex datasets for DAC %d after rule toggle".formatted(dacId), e); + } + } + public Integer removeChairpersonFromDAC(Integer dacId, Integer userId, Integer auditUserId) { return ruleDAO.auditedDeleteDACRuleSettingByUser(dacId, userId, auditUserId); } @@ -163,7 +268,7 @@ public void triggerDACRuleSettings( @VisibleForTesting protected Optional applyRule( DACAutomationRule rule, Dataset dataset, DataAccessRequest dar, ContainerRequest request) { - if (dataset.getDataUse() == null || !hasCanonicalSinglePrimaryDataUse(dataset.getDataUse())) { + if (!DataUsePrimaryClassifier.hasCanonicalSinglePrimary(dataset.getDataUse())) { logInfo( String.format( "Rule %s not triggered for DAC id: %s and dataset id: %s because the dataset does not have a canonical single primary Data Use", @@ -190,17 +295,6 @@ protected Optional applyRule( return Optional.empty(); } - /** - * DAC automation only supports canonical single-primary Data Use shapes. {@code Shape.SINGLE} - * also covers an Other-only primary category, which is non-canonical and must be excluded here to - * match the abstention policy in {@code DataUseMatcherV5}. - */ - private boolean hasCanonicalSinglePrimaryDataUse(DataUse dataUse) { - var classification = DataUsePrimaryClassifier.classify(dataUse); - return classification.shape() == Shape.SINGLE - && !classification.categories().contains(DataUsePrimaryCategory.OTHER); - } - @VisibleForTesting protected Vote openElectionAndApprove( DACAutomationRule rule, diff --git a/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java b/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java index d832854daa..5e7a58309a 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java +++ b/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchService.java @@ -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; @@ -41,6 +43,7 @@ import org.broadinstitute.consent.http.models.Study; import org.broadinstitute.consent.http.models.StudyProperty; import org.broadinstitute.consent.http.models.User; +import org.broadinstitute.consent.http.models.datause.DataUsePrimaryClassifier; import org.broadinstitute.consent.http.models.elastic_search.DacTerm; import org.broadinstitute.consent.http.models.elastic_search.DatasetTerm; import org.broadinstitute.consent.http.models.elastic_search.ElasticSearchHits; @@ -50,6 +53,9 @@ 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.rules.DACRuleAssignment; +import org.broadinstitute.consent.http.rules.Rules; import org.broadinstitute.consent.http.service.dao.DatasetServiceDAO; import org.broadinstitute.consent.http.util.ConsentLogger; import org.broadinstitute.consent.http.util.gson.GsonUtil; @@ -62,6 +68,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; @@ -81,6 +88,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); @@ -404,8 +412,14 @@ public Response indexDatasets(List datasetIds) throws IOException { } public Response indexDatasetList(List datasets) throws IOException { + // Resolved once for the whole batch rather than per dataset + Map> enabledRulesByDacId = + resolveEnabledRulesByDacId().orElse(null); List datasetTerms = - datasets.parallelStream().filter(Objects::nonNull).map(this::toDatasetTerm).toList(); + datasets.parallelStream() + .filter(Objects::nonNull) + .map(dataset -> toDatasetTerm(dataset, enabledRulesByDacId)) + .toList(); if (datasetTerms.isEmpty()) { return Response.status(Status.NOT_FOUND).build(); } @@ -433,7 +447,49 @@ public StreamingOutput indexDatasetIds(List datasetIds) { }; } - public DatasetTerm toDatasetTerm(Dataset dataset) { + /** + * The automation rules each DAC currently has enabled, keyed by DAC id. {@code Optional.empty()} + * means they could not be resolved at all, unlike an empty map, which means no DAC has any rule + * enabled. Indexing continues either way, but unresolved rules are not reported as dataset state. + */ + Optional>> resolveEnabledRulesByDacId() { + try { + List assignments = + Objects.requireNonNullElse(dacAutomationRuleDAO.findEnabledRuleAssignments(), List.of()); + return Optional.of( + assignments.stream() + .collect( + Collectors.groupingBy( + DACRuleAssignment::dacId, + Collectors.mapping( + DACRuleAssignment::ruleType, Collectors.toUnmodifiableSet())))); + } catch (Exception e) { + logWarn("Unable to resolve enabled DAC automation rules", e); + return Optional.empty(); + } + } + + /** + * Whether the dataset's DAC has an enabled rule that would automatically approve a matching + * request for it. Only the dataset half of each rule applies; the request half is unknowable at + * indexing time. Mirrors {@code DACAutomationRuleService.applyRule}, shape gate first. + */ + private boolean isInstantApprovalEligible(Dataset dataset, Set dacRules) { + if (!DataUsePrimaryClassifier.hasCanonicalSinglePrimary(dataset.getDataUse())) { + return false; + } + return Rules.implementationList.stream() + .filter(rule -> dacRules.contains(rule.getRuleType())) + .anyMatch(rule -> rule.datasetQualifies(dataset)); + } + + /** + * @param enabledRulesByDacId resolved DAC rules, or {@code null} when they could not be resolved + * — the rule-derived fields are then left unset so clients render nothing rather than being + * told the wrong approval process + */ + DatasetTerm toDatasetTerm( + Dataset dataset, Map> enabledRulesByDacId) { if (Objects.isNull(dataset)) { return null; } @@ -472,6 +528,22 @@ public DatasetTerm toDatasetTerm(Dataset dataset) { term.setDac(toDacTerm(dac)); }); + // A dataset with no DAC has no per-request approval step to satisfy and no DAC rule that could + // auto-approve it, which holds whether or not the rules resolved; only datasets whose state + // depends on the unresolved rules are left unset + if (Objects.isNull(dataset.getDacId())) { + term.setSoApprovalModel(SoApprovalModel.PRE_AUTHORIZED); + term.setInstantApprovalEligible(false); + } else if (Objects.nonNull(enabledRulesByDacId)) { + Set dacRules = + enabledRulesByDacId.getOrDefault(dataset.getDacId(), Set.of()); + term.setSoApprovalModel( + dacRules.contains(DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL) + ? SoApprovalModel.PER_REQUEST + : SoApprovalModel.PRE_AUTHORIZED); + term.setInstantApprovalEligible(isInstantApprovalEligible(dataset, dacRules)); + } + if (Objects.nonNull(dataset.getDataUse())) { DataUseSummary summary = ontologyService.translateDataUseSummary(dataset.getDataUse()); if (summary != null) { diff --git a/src/main/resources/assets/paths/datasetSearchIndex.yaml b/src/main/resources/assets/paths/datasetSearchIndex.yaml index 4be3acd385..0914643134 100644 --- a/src/main/resources/assets/paths/datasetSearchIndex.yaml +++ b/src/main/resources/assets/paths/datasetSearchIndex.yaml @@ -67,6 +67,8 @@ post: description: Local ethics committee approval is required. dataLocation: Not Determined dacId: 3 + soApprovalModel: PRE_AUTHORIZED + instantApprovalEligible: false accessManagement: controlled study: description: Test study details diff --git a/src/main/resources/assets/paths/datasetSearchIndexV2.yaml b/src/main/resources/assets/paths/datasetSearchIndexV2.yaml index 7788ead5c0..123402c966 100644 --- a/src/main/resources/assets/paths/datasetSearchIndexV2.yaml +++ b/src/main/resources/assets/paths/datasetSearchIndexV2.yaml @@ -79,6 +79,8 @@ post: - code: NCU dacId: 2 dacApproval: true + soApprovalModel: PER_REQUEST + instantApprovalEligible: false approvedUserIds: [1,2,3] submitter: userId: 10 diff --git a/src/main/resources/assets/schemas/DatasetSearch.yaml b/src/main/resources/assets/schemas/DatasetSearch.yaml index ae92323047..c12862a87d 100644 --- a/src/main/resources/assets/schemas/DatasetSearch.yaml +++ b/src/main/resources/assets/schemas/DatasetSearch.yaml @@ -24,6 +24,25 @@ properties: dacId: type: integer description: The unique identifier for a DAC + soApprovalModel: + type: string + enum: + - PER_REQUEST + - PRE_AUTHORIZED + description: >- + Which Signing Official authorization model the dataset's DAC uses. PER_REQUEST means the + SO named in each 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. + instantApprovalEligible: + type: boolean + description: >- + Whether the dataset's DAC has an automation rule enabled that would automatically approve a + matching access request for it. Only the dataset half of each rule is evaluated here, so a + true value means the dataset qualifies, not that any given request will be approved. Absent + when the DAC's rules 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 as false. study: type: object title: DatasetSearchStudy diff --git a/src/test/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAOTest.java b/src/test/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAOTest.java index 445308a9fc..3ef90b5af0 100644 --- a/src/test/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAOTest.java +++ b/src/test/java/org/broadinstitute/consent/http/db/DACAutomationRuleDAOTest.java @@ -11,6 +11,7 @@ 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.DACRuleAssignment; import org.broadinstitute.consent.http.rules.RuleAuditAction; import org.jdbi.v3.core.statement.UnableToExecuteStatementException; import org.junit.jupiter.api.Assertions; @@ -29,6 +30,79 @@ void testFindAll() { rules.stream().anyMatch(rule -> rule.ruleType().equals(DACAutomationRuleType.GRU_V1))); } + private DACAutomationRule findRule(DACAutomationRuleType ruleType) { + return dacAutomationRuleDAO.findAll().stream() + .filter(r -> r.ruleType().equals(ruleType)) + .findFirst() + .orElseThrow(); + } + + @Test + void testFindEnabledRuleAssignments() { + User user = createUser(); + Integer enabledDacId = createRandomDAC(); + Integer untouchedDacId = createRandomDAC(); + DACAutomationRule soRule = findRule(DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL); + dacAutomationRuleDAO.auditedInsertDACRuleSetting( + enabledDacId, soRule.id(), user.getUserId(), Instant.now()); + + List assignments = dacAutomationRuleDAO.findEnabledRuleAssignments(); + + assertTrue( + assignments.contains( + new DACRuleAssignment(enabledDacId, DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL))); + Assertions.assertFalse( + assignments.stream().anyMatch(a -> Objects.equals(a.dacId(), untouchedDacId))); + } + + @Test + void testFindEnabledRuleAssignmentsReportsTheRuleThatIsEnabled() { + User user = createUser(); + Integer dacId = createRandomDAC(); + DACAutomationRule gruRule = findRule(DACAutomationRuleType.GRU_V1); + dacAutomationRuleDAO.auditedInsertDACRuleSetting( + dacId, gruRule.id(), user.getUserId(), Instant.now()); + + List assignments = dacAutomationRuleDAO.findEnabledRuleAssignments(); + + // Enabling one rule must not report the DAC as having any other rule enabled + assertEquals( + List.of(DACAutomationRuleType.GRU_V1), + assignments.stream() + .filter(a -> Objects.equals(a.dacId(), dacId)) + .map(DACRuleAssignment::ruleType) + .toList()); + } + + @Test + void testFindEnabledRuleAssignmentsIgnoresIncompleteSettingsRows() { + User user = createUser(); + Integer dacId = createRandomDAC(); + DACAutomationRule gruRule = findRule(DACAutomationRuleType.GRU_V1); + // dac_id and user_id are both nullable. A row naming no user is what + // findAllDACAutomationRulesByDACId reports as disabled, and a null dac_id has no pairing to key + jdbi.useHandle( + handle -> { + handle + .createUpdate( + "INSERT INTO dac_rule_settings (dac_id, rule_id, user_id) VALUES (:dacId, :ruleId, NULL)") + .bind("dacId", dacId) + .bind("ruleId", gruRule.id()) + .execute(); + handle + .createUpdate( + "INSERT INTO dac_rule_settings (dac_id, rule_id, user_id) VALUES (NULL, :ruleId, :userId)") + .bind("ruleId", gruRule.id()) + .bind("userId", user.getUserId()) + .execute(); + }); + + List assignments = dacAutomationRuleDAO.findEnabledRuleAssignments(); + + Assertions.assertFalse(assignments.stream().anyMatch(a -> Objects.equals(a.dacId(), dacId))); + Assertions.assertTrue(assignments.stream().allMatch(a -> Objects.nonNull(a.dacId()))); + } + @Test void testInsertDACRuleSetting() { User user = createUser(); diff --git a/src/test/java/org/broadinstitute/consent/http/service/DACAutomationRuleServiceTest.java b/src/test/java/org/broadinstitute/consent/http/service/DACAutomationRuleServiceTest.java index 37706b863a..807ce3156e 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/DACAutomationRuleServiceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/DACAutomationRuleServiceTest.java @@ -11,6 +11,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.after; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; @@ -18,13 +19,27 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.timeout; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.google.common.util.concurrent.MoreExecutors; +import jakarta.ws.rs.core.Response; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.Date; import java.util.List; import java.util.Optional; import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.broadinstitute.consent.http.AbstractTestHelper; import org.broadinstitute.consent.http.db.DACAutomationRuleDAO; import org.broadinstitute.consent.http.db.DataAccessRequestDAO; @@ -57,6 +72,7 @@ import org.glassfish.jersey.server.ContainerRequest; import org.jdbi.v3.core.Jdbi; import org.jdbi.v3.sqlobject.transaction.TransactionalCallback; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -82,10 +98,14 @@ class DACAutomationRuleServiceTest extends AbstractTestHelper { @Mock private VoteService voteService; + @Mock private ElasticSearchService elasticSearchService; + @Mock private ContainerRequest request; private DACAutomationRuleService service; + private ExecutorService executorService; + private static DACAutomationRule makeDacAutomationRuleGRU() { return new DACAutomationRule( 1, @@ -145,6 +165,10 @@ private static Dataset makeDataset() { return makeDataset(1, "Test Dataset", 0); } + private static List datasetsWithIds(int... datasetIds) { + return Arrays.stream(datasetIds).mapToObj(id -> makeDataset(id, "Dataset " + id, 1)).toList(); + } + @BeforeEach void setUp() { when(jdbi.onDemand(DataAccessRequestDAO.class)).thenReturn(dataAccessRequestDAO); @@ -153,7 +177,16 @@ void setUp() { when(jdbi.onDemand(ElectionDAO.class)).thenReturn(electionDAO); when(jdbi.onDemand(UserDAO.class)).thenReturn(userDAO); when(jdbi.onDemand(VoteDAO.class)).thenReturn(voteDAO); - service = new DACAutomationRuleService(jdbi, voteServiceDAO, voteService); + // Direct executor so the best-effort reindex runs inline and these tests can assert on it + executorService = MoreExecutors.newDirectExecutorService(); + service = + new DACAutomationRuleService( + jdbi, voteServiceDAO, voteService, elasticSearchService, executorService); + } + + @AfterEach + void tearDown() { + executorService.shutdownNow(); } @Test @@ -237,6 +270,269 @@ void testToggleRuleFromOnToOff() { assertEquals(-1, result.getEnabledTime()); } + @Test + void testToggleSoApprovalRuleReindexesTheDacsDatasets() throws Exception { + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(ruleDAO.auditedInsertDACRuleSetting(anyInt(), anyInt(), anyInt(), any())).thenReturn(1); + when(datasetDAO.findDatasetsAssociatedWithDac(1)).thenReturn(datasetsWithIds(10, 11)); + when(elasticSearchService.indexDatasets(List.of(10, 11))).thenReturn(Response.ok().build()); + + AutomationRuleToggleResponse result = service.toggleRule(1, 1, user); + + assertTrue(result.isRuleEnabled()); + verify(elasticSearchService).indexDatasets(List.of(10, 11)); + // The rest of the corpus is external entries with no DAC, which no rule change can affect + verify(datasetDAO, never()).findAllDatasetIds(); + } + + @Test + void testToggleAutoApprovalRuleAlsoReindexes() throws Exception { + // Auto-approve rules drive instantApprovalEligible, so their toggles stale documents too + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.GRU_V1, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(ruleDAO.auditedInsertDACRuleSetting(anyInt(), anyInt(), anyInt(), any())).thenReturn(1); + when(datasetDAO.findDatasetsAssociatedWithDac(1)).thenReturn(datasetsWithIds(10, 11)); + when(elasticSearchService.indexDatasets(List.of(10, 11))).thenReturn(Response.ok().build()); + + service.toggleRule(1, 1, user); + + verify(elasticSearchService).indexDatasets(List.of(10, 11)); + } + + @Test + void testToggleRuleSkipsReindexWhenThereAreNoDatasets() throws Exception { + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.GRU_V1, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(ruleDAO.auditedInsertDACRuleSetting(anyInt(), anyInt(), anyInt(), any())).thenReturn(1); + when(datasetDAO.findDatasetsAssociatedWithDac(1)).thenReturn(List.of()); + + service.toggleRule(1, 1, user); + + verify(elasticSearchService, never()).indexDatasets(any()); + } + + @Test + void testTogglesDuringAnInFlightReindexCoalesceIntoOneFollowUpPass() throws Exception { + // A real executor, so the reindex genuinely runs off-thread and can be held mid-flight + ExecutorService realExecutor = Executors.newVirtualThreadPerTaskExecutor(); + service = + new DACAutomationRuleService( + jdbi, voteServiceDAO, voteService, elasticSearchService, realExecutor); + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.GRU_V1, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(datasetDAO.findDatasetsAssociatedWithDac(1)).thenReturn(datasetsWithIds(10, 11)); + CountDownLatch firstPassStarted = new CountDownLatch(1); + CountDownLatch releaseFirstPass = new CountDownLatch(1); + AtomicInteger passes = new AtomicInteger(); + when(elasticSearchService.indexDatasets(any())) + .thenAnswer( + invocation -> { + if (passes.incrementAndGet() == 1) { + firstPassStarted.countDown(); + releaseFirstPass.await(5, TimeUnit.SECONDS); + } + return Response.ok().build(); + }); + + try { + service.toggleRule(1, 1, user); + assertTrue(firstPassStarted.await(5, TimeUnit.SECONDS)); + // Both land while the first pass is still running, so they share a single follow-up pass + service.toggleRule(1, 1, user); + service.toggleRule(1, 1, user); + releaseFirstPass.countDown(); + + // Waits for the follow-up pass rather than assuming a fixed window is long enough for it + verify(elasticSearchService, timeout(5000).times(2)).indexDatasets(List.of(10, 11)); + // Then holds to catch a third: the two toggles must share one pass, not get one each + verify(elasticSearchService, after(500).times(2)).indexDatasets(List.of(10, 11)); + assertEquals(2, passes.get()); + } finally { + releaseFirstPass.countDown(); + realExecutor.shutdownNow(); + } + } + + @Test + void testQueuedDacsAreReindexedInTheOrderTheyWereToggled() throws Exception { + ExecutorService realExecutor = Executors.newVirtualThreadPerTaskExecutor(); + service = + new DACAutomationRuleService( + jdbi, voteServiceDAO, voteService, elasticSearchService, realExecutor); + List dacIds = List.of(7, 8, 9); + dacIds.forEach( + dacId -> { + when(ruleDAO.findAllDACAutomationRulesByDACId(dacId)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.GRU_V1, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(datasetDAO.findDatasetsAssociatedWithDac(dacId)) + .thenReturn(List.of(makeDataset(dacId * 10, "Dataset", dacId))); + }); + CountDownLatch firstPassStarted = new CountDownLatch(1); + CountDownLatch releaseFirstPass = new CountDownLatch(1); + List> indexedInOrder = Collections.synchronizedList(new ArrayList<>()); + when(elasticSearchService.indexDatasets(any())) + .thenAnswer( + invocation -> { + List ids = invocation.getArgument(0); + indexedInOrder.add(ids); + if (indexedInOrder.size() == 1) { + firstPassStarted.countDown(); + releaseFirstPass.await(5, TimeUnit.SECONDS); + } + return Response.ok().build(); + }); + + try { + // The first toggle holds the drain, so the other two queue behind it while it runs + service.toggleRule(7, 1, user); + assertTrue(firstPassStarted.await(5, TimeUnit.SECONDS)); + service.toggleRule(8, 1, user); + service.toggleRule(9, 1, user); + releaseFirstPass.countDown(); + + verify(elasticSearchService, timeout(5000).times(3)).indexDatasets(any()); + assertEquals(List.of(List.of(70), List.of(80), List.of(90)), indexedInOrder); + } finally { + releaseFirstPass.countDown(); + realExecutor.shutdownNow(); + } + } + + @Test + void testToggleRuleSurvivesARejectedReindexAndStaysSchedulable() throws Exception { + // An executor shutting down rejects the submission. The toggle is already committed and + // audited by then, so it must not fail — and reindexRunning must be released, or the + // coalescing guard would swallow every later reindex for the life of the process. + ExecutorService rejecting = mock(ExecutorService.class); + when(rejecting.submit(any(Runnable.class))).thenThrow(new RejectedExecutionException("down")); + service = + new DACAutomationRuleService( + jdbi, voteServiceDAO, voteService, elasticSearchService, rejecting); + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.GRU_V1, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(ruleDAO.auditedInsertDACRuleSetting(anyInt(), anyInt(), anyInt(), any())).thenReturn(1); + + AutomationRuleToggleResponse result = service.toggleRule(1, 1, user); + assertTrue(result.isRuleEnabled()); + + // A second toggle must still attempt to schedule rather than coalescing into a dead pass + service.toggleRule(1, 1, user); + verify(rejecting, times(2)).submit(any(Runnable.class)); + } + + @Test + void testToggleRuleStaysSchedulableAfterAnErrorEscapesAReindexPass() throws Exception { + // An Error escapes the best-effort catch, which only handles Exception. The guard must still be + // released, or every later reindex would coalesce into a pass that never runs. + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.GRU_V1, + "Test Rule", + RuleState.AVAILABLE, + null, + null, + null, + null))); + when(ruleDAO.auditedInsertDACRuleSetting(anyInt(), anyInt(), anyInt(), any())).thenReturn(1); + when(datasetDAO.findDatasetsAssociatedWithDac(1)) + .thenThrow(new StackOverflowError("boom")) + .thenReturn(datasetsWithIds(10)); + when(elasticSearchService.indexDatasets(List.of(10))).thenReturn(Response.ok().build()); + + service.toggleRule(1, 1, user); + service.toggleRule(1, 1, user); + + verify(elasticSearchService).indexDatasets(List.of(10)); + } + + @Test + void testToggleRuleSucceedsWhenReindexFails() throws Exception { + when(ruleDAO.findAllDACAutomationRulesByDACId(1)) + .thenReturn( + List.of( + new DACAutomationRule( + 1, + DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL, + "Test Rule", + RuleState.AVAILABLE, + FIXED_TIMESTAMP, + 1, + "alice", + "alice@fake.org"))); + doNothing().when(ruleDAO).auditedDeleteDACRuleSetting(anyInt(), anyInt(), anyInt()); + when(datasetDAO.findDatasetsAssociatedWithDac(1)).thenReturn(datasetsWithIds(10)); + when(elasticSearchService.indexDatasets(List.of(10))).thenThrow(new IOException("es down")); + + // The rule change is already committed and audited, so a failed reindex must not surface + AutomationRuleToggleResponse result = service.toggleRule(1, 1, user); + + assertFalse(result.isRuleEnabled()); + } + @Test void testToggleRuleFromOnToOffInvalidRuleNumber() { when(ruleDAO.findAllDACAutomationRulesByDACId(1)) diff --git a/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchServiceTest.java b/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchServiceTest.java index 4fc1f2d4d3..abcad4d289 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchServiceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchServiceTest.java @@ -3,6 +3,7 @@ import static jakarta.ws.rs.core.Response.Status.fromStatusCode; import static org.broadinstitute.consent.http.models.dataset_registration_v1.builder.DatasetRegistrationSchemaV1Builder.assets; import static org.broadinstitute.consent.http.models.dataset_registration_v1.builder.DatasetRegistrationSchemaV1Builder.data; +import static org.broadinstitute.consent.http.rules.DACAutomationRuleType.REQUIRE_SO_DAR_APPROVAL; import static org.junit.Assert.assertThrows; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -50,12 +51,14 @@ import org.apache.http.nio.entity.NStringEntity; import org.broadinstitute.consent.http.AbstractTestHelper; 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.PropertyType; +import org.broadinstitute.consent.http.enumeration.SoApprovalModel; import org.broadinstitute.consent.http.models.Dac; import org.broadinstitute.consent.http.models.DataAccessRequest; import org.broadinstitute.consent.http.models.DataUse; @@ -71,6 +74,8 @@ import org.broadinstitute.consent.http.models.elastic_search.DatasetTerm; import org.broadinstitute.consent.http.models.ontology.DataUseSummary; import org.broadinstitute.consent.http.models.ontology.DataUseTerm; +import org.broadinstitute.consent.http.rules.DACAutomationRuleType; +import org.broadinstitute.consent.http.rules.DACRuleAssignment; import org.broadinstitute.consent.http.service.dao.DatasetServiceDAO; import org.broadinstitute.consent.http.util.TestAppender; import org.broadinstitute.consent.http.util.gson.GsonUtil; @@ -102,6 +107,8 @@ class ElasticSearchServiceTest extends AbstractTestHelper { @Mock private DacDAO dacDAO; + @Mock private DACAutomationRuleDAO dacAutomationRuleDAO; + @Mock private UserDAO userDao; @Mock private InstitutionDAO institutionDAO; @@ -117,6 +124,7 @@ class ElasticSearchServiceTest extends AbstractTestHelper { @BeforeEach void initService() { when(jdbi.onDemand(DacDAO.class)).thenReturn(dacDAO); + when(jdbi.onDemand(DACAutomationRuleDAO.class)).thenReturn(dacAutomationRuleDAO); when(jdbi.onDemand(UserDAO.class)).thenReturn(userDao); when(jdbi.onDemand(InstitutionDAO.class)).thenReturn(institutionDAO); when(jdbi.onDemand(DatasetDAO.class)).thenReturn(datasetDAO); @@ -231,6 +239,11 @@ private DataUseSummary createDataUseSummary() { return dataUseSummary; } + /** Mirrors indexDatasetList: rules resolved once, then applied to the dataset. */ + private DatasetTerm toDatasetTerm(Dataset dataset) { + return service.toDatasetTerm(dataset, service.resolveEnabledRulesByDacId().orElse(null)); + } + /** Private container record to consolidate dataset and associated object creation */ private record DatasetRecord( User createUser, User updateUser, Dac dac, Dataset dataset, Study study) {} @@ -376,7 +389,7 @@ void testToDatasetTerm_UserInfo() { .thenReturn(datasetRecord.updateUser.getInstitution()); when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); assertEquals(datasetRecord.createUser.getUserId(), term.getCreateUserId()); assertEquals(datasetRecord.createUser.getDisplayName(), term.getCreateUserDisplayName()); assertEquals(datasetRecord.createUser.getUserId(), term.getSubmitter().userId()); @@ -404,7 +417,7 @@ void testToDatasetTerm_StudyInfo() { .thenReturn(datasetRecord.updateUser); when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); assertEquals(datasetRecord.study.getDescription(), term.getStudy().getDescription()); assertEquals(datasetRecord.study.getName(), term.getStudy().getStudyName()); assertEquals(datasetRecord.study.getStudyId(), term.getStudy().getStudyId()); @@ -481,7 +494,7 @@ void testToDatasetTerm_JsonBlobs(String propKey) { .thenReturn(datasetRecord.updateUser); when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); switch (propKey) { case assets: assertEquals(refMap, term.getStudy().getAssets()); @@ -511,7 +524,7 @@ void testToDatasetTerm_DatasetInfo() { card2.setUserId(dar2.getUserId()); when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); when(ontologyService.translateDataUseSummary(any())).thenReturn(dataUseSummary); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); assertEquals(datasetRecord.dataset.getDatasetId(), term.getDatasetId()); assertEquals(datasetRecord.dataset.getDatasetIdentifier(), term.getDatasetIdentifier()); @@ -569,7 +582,7 @@ void testToDatasetTerm_Data() { datasetProperties.add(newProperty); dataset.setProperties(datasetProperties); - DatasetTerm term = service.toDatasetTerm(dataset); + DatasetTerm term = toDatasetTerm(dataset); Optional dataProp = dataset.getProperties().stream() .filter(p -> p.getSchemaProperty().equals(data)) @@ -578,6 +591,230 @@ void testToDatasetTerm_Data() { assertEquals(refMap, term.getData()); } + /** A data use that clears hasNoModifiers, so only the primary code decides which rules match. */ + private DataUse unmodifiedDataUse() { + DataUse dataUse = new DataUse(); + dataUse.setDiseaseRestrictions(List.of()); + return dataUse; + } + + @Test + void testToDatasetTermSoApprovalModelPerRequest() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, REQUIRE_SO_DAR_APPROVAL))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + + DatasetTerm term = toDatasetTerm(dataset); + + assertEquals(SoApprovalModel.PER_REQUEST, term.getSoApprovalModel()); + } + + @Test + void testToDatasetTermSoApprovalModelPreAuthorized() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(99, REQUIRE_SO_DAR_APPROVAL))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + + DatasetTerm term = toDatasetTerm(dataset); + + assertEquals(SoApprovalModel.PRE_AUTHORIZED, term.getSoApprovalModel()); + } + + @Test + void testToDatasetTermSoApprovalModelWithNoDac() { + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + + DatasetTerm term = toDatasetTerm(dataset); + + assertEquals(SoApprovalModel.PRE_AUTHORIZED, term.getSoApprovalModel()); + assertFalse(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermRuleDerivedFieldsUnsetWhenRuleLookupFails() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenThrow(new RuntimeException("db down")); + when(dacDAO.findById(7)).thenReturn(new Dac()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + + // Indexing continues, but unresolved rules must not be reported as dataset state + DatasetTerm term = toDatasetTerm(dataset); + + assertNull(term.getSoApprovalModel()); + assertNull(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermTreatsNullRuleLookupAsNoRulesEnabled() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()).thenReturn(null); + when(dacDAO.findById(7)).thenReturn(new Dac()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + + DatasetTerm term = toDatasetTerm(dataset); + + assertEquals(SoApprovalModel.PRE_AUTHORIZED, term.getSoApprovalModel()); + assertFalse(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermInstantApprovalEligibleForMatchingRule() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, DACAutomationRuleType.GRU_V1))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + DataUse dataUse = unmodifiedDataUse(); + dataUse.setGeneralUse(true); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + dataset.setDataUse(dataUse); + + DatasetTerm term = toDatasetTerm(dataset); + + assertTrue(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermInstantApprovalIneligibleWhenRuleCoversOtherCode() { + // The DAC auto-approves GRU, but this dataset is HMB + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, DACAutomationRuleType.GRU_V1))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + DataUse dataUse = unmodifiedDataUse(); + dataUse.setHmbResearch(true); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + dataset.setDataUse(dataUse); + + DatasetTerm term = toDatasetTerm(dataset); + + assertFalse(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermInstantApprovalIneligibleWhenDataUseCarriesModifier() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, DACAutomationRuleType.GRU_V1))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + DataUse dataUse = unmodifiedDataUse(); + dataUse.setGeneralUse(true); + dataUse.setEthicsApprovalRequired(true); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + dataset.setDataUse(dataUse); + + DatasetTerm term = toDatasetTerm(dataset); + + assertFalse(term.getInstantApprovalEligible()); + } + + /** + * The indexed document is duos-ui's only source for these two fields, and it reads them by the + * exact names and values asserted here (DT-3799). Serialized through the same GsonUtil the bulk + * indexer uses, so a rename on either side fails this rather than silently blanking the Data + * Library's SO Approval column and instant-approval badge. + */ + @Test + void testIndexedDocumentCarriesTheRuleDerivedFieldsClientsRead() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn( + List.of( + new DACRuleAssignment(7, REQUIRE_SO_DAR_APPROVAL), + new DACRuleAssignment(7, DACAutomationRuleType.GRU_V1))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + DataUse dataUse = unmodifiedDataUse(); + dataUse.setGeneralUse(true); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + dataset.setDataUse(dataUse); + + String json = GsonUtil.getInstance().toJson(toDatasetTerm(dataset)); + + assertTrue(json.contains("\"soApprovalModel\":\"PER_REQUEST\""), json); + assertTrue(json.contains("\"instantApprovalEligible\":true"), json); + } + + @Test + void testIndexedDocumentOmitsRuleDerivedFieldsWhenUnresolved() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenThrow(new RuntimeException("db down")); + when(dacDAO.findById(7)).thenReturn(new Dac()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + + String json = GsonUtil.getInstance().toJson(toDatasetTerm(dataset)); + + // Absent rather than false/null, so clients can tell "not eligible" from "not yet known" + assertFalse(json.contains("soApprovalModel"), json); + assertFalse(json.contains("instantApprovalEligible"), json); + } + + @Test + void testToDatasetTermInstantApprovalIneligibleForMultiplePrimaryDataUse() { + // Two primary categories is not a shape automation acts on, so the engine abstains before + // consulting any rule — the indexed flag has to abstain with it + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, DACAutomationRuleType.GRU_V1))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + DataUse dataUse = unmodifiedDataUse(); + dataUse.setGeneralUse(true); + dataUse.setHmbResearch(true); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + dataset.setDataUse(dataUse); + + DatasetTerm term = toDatasetTerm(dataset); + + assertFalse(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermInstantApprovalIneligibleForNonApprovingRule() { + // REQUIRE_SO_DAR_APPROVAL never auto-approves, so it cannot make a dataset eligible + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, REQUIRE_SO_DAR_APPROVAL))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + DataUse dataUse = unmodifiedDataUse(); + dataUse.setGeneralUse(true); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + dataset.setDataUse(dataUse); + + DatasetTerm term = toDatasetTerm(dataset); + + assertFalse(term.getInstantApprovalEligible()); + } + + @Test + void testToDatasetTermInstantApprovalIneligibleWithoutDataUse() { + when(dacAutomationRuleDAO.findEnabledRuleAssignments()) + .thenReturn(List.of(new DACRuleAssignment(7, DACAutomationRuleType.GRU_V1))); + when(dacDAO.findById(7)).thenReturn(new Dac()); + Dataset dataset = new Dataset(); + dataset.setDatasetId(1); + dataset.setDacId(7); + + DatasetTerm term = toDatasetTerm(dataset); + + assertFalse(term.getInstantApprovalEligible()); + } + @Test void testToDatasetTermUsesLegacyAccessManagementProperty() { Dataset dataset = new Dataset(); @@ -588,7 +825,7 @@ void testToDatasetTermUsesLegacyAccessManagementProperty() { property.setPropertyValue("open"); dataset.setProperties(Set.of(property)); - DatasetTerm term = service.toDatasetTerm(dataset); + DatasetTerm term = toDatasetTerm(dataset); assertEquals("open", term.getAccessManagement()); } @@ -599,7 +836,7 @@ void testToDatasetTerm_DacInfo() { when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); when(userDao.findUserById(datasetRecord.createUser.getUserId())) .thenReturn(datasetRecord.createUser); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); assertEquals(datasetRecord.dataset.getDacApproval(), term.getDacApproval()); assertEquals(datasetRecord.dac.getDacId(), term.getDacId()); @@ -613,7 +850,7 @@ void testToDatasetTerm_NIHInstitutionalCertification() { when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); when(userDao.findUserById(datasetRecord.createUser.getUserId())) .thenReturn(datasetRecord.createUser); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); assertEquals( datasetRecord.dataset.getNihInstitutionalCertificationFile() != null, term.getHasInstitutionCertification()); @@ -626,7 +863,7 @@ void testToDatasetTerm_Missing_NIHInstitutionalCertification() { when(userDao.findUserById(datasetRecord.createUser.getUserId())) .thenReturn(datasetRecord.createUser); datasetRecord.dataset.setNihInstitutionalCertificationFile(null); - DatasetTerm term = service.toDatasetTerm(datasetRecord.dataset); + DatasetTerm term = toDatasetTerm(datasetRecord.dataset); assertNull(term.getHasInstitutionCertification()); } @@ -652,7 +889,7 @@ void testToDatasetTerm_StringNumberOfParticipants() { when(dacDAO.findById(any())).thenReturn(datasetRecord.dac); when(userDao.findUserById(datasetRecord.createUser.getUserId())) .thenReturn(datasetRecord.createUser); - assertDoesNotThrow(() -> service.toDatasetTerm(dataset)); + assertDoesNotThrow(() -> toDatasetTerm(dataset)); } @Test @@ -663,7 +900,7 @@ void testToDatasetTermIncomplete() { dataset.setDatasetIdentifier(); dataset.setProperties(Set.of()); - DatasetTerm term = service.toDatasetTerm(dataset); + DatasetTerm term = toDatasetTerm(dataset); assertEquals(dataset.getDatasetId(), term.getDatasetId()); assertEquals(dataset.getDatasetIdentifier(), term.getDatasetIdentifier()); @@ -681,7 +918,7 @@ void testToDatasetTerm_RequestLocation() { dataset.setStudy(study); when(userDao.findUserById(user.getUserId())).thenReturn(user); - DatasetTerm term = service.toDatasetTerm(dataset); + DatasetTerm term = toDatasetTerm(dataset); Optional requestLocationProp = dataset.getProperties().stream() @@ -702,7 +939,7 @@ void testToDatasetTerm_RequestLocation_Missing() { dataset.setProperties(Set.of(createDatasetProperty("url", PropertyType.String, "url"))); when(userDao.findUserById(user.getUserId())).thenReturn(user); - DatasetTerm term = service.toDatasetTerm(dataset); + DatasetTerm term = toDatasetTerm(dataset); assertNull(term.getRequestLocation()); } @@ -710,7 +947,7 @@ void testToDatasetTerm_RequestLocation_Missing() { @Test void testToDatasetTermNullDatasetProps() { Dataset dataset = new Dataset(); - assertDoesNotThrow(() -> service.toDatasetTerm(dataset)); + assertDoesNotThrow(() -> toDatasetTerm(dataset)); } @Test @@ -721,7 +958,7 @@ void testToDatasetTermNullStudyProps() { study.setDescription(randomAlphabetic(20)); study.setStudyId(randomInt(1, 100)); dataset.setStudy(study); - assertDoesNotThrow(() -> service.toDatasetTerm(dataset)); + assertDoesNotThrow(() -> toDatasetTerm(dataset)); } @Captor ArgumentCaptor request;