From ba9cc28b1485abd82d592fed1aad32c3485c29ad Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Fri, 5 Dec 2025 16:28:49 -0500 Subject: [PATCH 1/8] codex first pass --- .../v1/controller/ExperimentController.java | 4 + .../v2/dao/BrAPIObservationLevelDAO.java | 90 ++++++++ .../brapi/v2/dao/BrAPIObservationUnitDAO.java | 30 +++ .../brapi/v2/services/BrAPITrialService.java | 207 +++++++++++++----- .../services/lock/DistributedLockService.java | 61 ++++++ .../SubEntityDatasetLockIntegrationTest.java | 108 +++++++++ .../lock/DistributedLockServiceTest.java | 57 +++++ 7 files changed, 499 insertions(+), 58 deletions(-) create mode 100644 src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java create mode 100644 src/main/java/org/breedinginsight/services/lock/DistributedLockService.java create mode 100644 src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java create mode 100644 src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java diff --git a/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java b/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java index 8619a140a..fe5436cd6 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java @@ -5,6 +5,7 @@ import io.micronaut.http.HttpStatus; import io.micronaut.http.MediaType; import io.micronaut.http.annotation.*; +import io.micronaut.http.exceptions.HttpStatusException; import io.micronaut.http.server.types.files.StreamedFile; import io.micronaut.security.annotation.Secured; import io.micronaut.security.rules.SecurityRule; @@ -134,6 +135,9 @@ public HttpResponse> createSubEntityDataset( Response response = new Response(experimentService.createSubEntityDataset(programOptional.get(), experimentId, datasetRequest)); return HttpResponse.ok(response); + } catch (HttpStatusException e) { + log.info(e.getMessage()); + return HttpResponse.status(e.getStatus(), e.getMessage()); } catch (Exception e){ log.info(e.getMessage()); return HttpResponse.status(HttpStatus.UNPROCESSABLE_ENTITY, e.getMessage()); diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java new file mode 100644 index 000000000..1af5aa33a --- /dev/null +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java @@ -0,0 +1,90 @@ +/* + * See the NOTICE file distributed with this work for additional information + * regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.breedinginsight.brapi.v2.dao; + +import com.google.gson.Gson; +import io.micronaut.http.HttpResponse; +import io.micronaut.http.HttpStatus; +import lombok.extern.slf4j.Slf4j; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.Request; +import okhttp3.RequestBody; +import org.brapi.client.v2.JSON; +import org.brapi.client.v2.model.exceptions.ApiException; +import org.brapi.v2.model.pheno.BrAPIObservationUnitHierarchyLevel; +import org.breedinginsight.model.DatasetLevel; +import org.breedinginsight.model.Program; +import org.breedinginsight.utilities.BrAPIDAOUtil; + +import javax.inject.Inject; +import javax.inject.Singleton; + +@Slf4j +@Singleton +public class BrAPIObservationLevelDAO { + + private static final MediaType JSON_MEDIA_TYPE = MediaType.get("application/json"); + private final BrAPIDAOUtil brAPIDAOUtil; + private final Gson gson = new JSON().getGson(); + + @Inject + public BrAPIObservationLevelDAO(BrAPIDAOUtil brAPIDAOUtil) { + this.brAPIDAOUtil = brAPIDAOUtil; + } + + public HttpResponse createObservationLevelName(Program program, String levelName, DatasetLevel levelOrder) throws ApiException { + HttpUrl url = HttpUrl.parse(brAPIDAOUtil.getProgramBrAPIBaseUrl(program.getId())) + .newBuilder() + .addPathSegment("observationlevelnames") + .build(); + BrAPIObservationUnitHierarchyLevel level = new BrAPIObservationUnitHierarchyLevel() + .levelName(levelName); + if (levelOrder != null) { + level.setLevelOrder(levelOrder.getValue()); + } + RequestBody body = RequestBody.create(gson.toJson(level), JSON_MEDIA_TYPE); + var request = new Request.Builder() + .url(url) + .post(body) + .addHeader("Content-Type", "application/json") + .build(); + return brAPIDAOUtil.makeCall(request); + } + + public void deleteObservationLevelName(Program program, String levelName) { + HttpUrl url = HttpUrl.parse(brAPIDAOUtil.getProgramBrAPIBaseUrl(program.getId())) + .newBuilder() + .addPathSegment("observationlevelnames") + .addPathSegment(levelName) + .build(); + var request = new Request.Builder() + .url(url) + .delete() + .addHeader("Content-Type", "application/json") + .build(); + try { + HttpResponse response = brAPIDAOUtil.makeCall(request); + if (response.getStatus() != HttpStatus.OK && response.getStatus() != HttpStatus.NO_CONTENT && response.getStatus() != HttpStatus.ACCEPTED) { + log.warn("Observation level delete returned status {} for {}", response.getStatus(), levelName); + } + } catch (Exception e) { + log.warn("Failed to delete observation level {} during rollback", levelName, e); + } + } +} diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java index 9749bf093..5a86c2337 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java @@ -26,6 +26,8 @@ import io.micronaut.scheduling.annotation.Scheduled; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; +import okhttp3.HttpUrl; +import okhttp3.Request; import org.brapi.client.v2.JSON; import org.brapi.client.v2.model.exceptions.ApiException; import org.brapi.client.v2.modules.phenotype.ObservationUnitsApi; @@ -442,4 +444,32 @@ private void preprocessObservationUnits(List brapiObservat } } } + + public void deleteObservationUnits(Collection observationUnitDbIds, UUID programId) { + if (observationUnitDbIds == null || observationUnitDbIds.isEmpty()) { + return; + } + String baseUrl = brAPIDAOUtil.getProgramBrAPIBaseUrl(programId); + for (String ouDbId : observationUnitDbIds) { + if (StringUtils.isBlank(ouDbId)) { + continue; + } + HttpUrl url = HttpUrl.parse(baseUrl) + .newBuilder() + .addPathSegment("observationunits") + .addPathSegment(ouDbId) + .build(); + Request request = new Request.Builder() + .url(url) + .delete() + .addHeader("Content-Type", "application/json") + .build(); + try { + brAPIDAOUtil.makeCall(request); + } catch (Exception e) { + log.warn("Failed to delete observation unit {} during rollback", ouDbId, e); + } + } + repopulateCache(programId); + } } diff --git a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java index 158ce952a..96337c026 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java +++ b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java @@ -6,7 +6,10 @@ import com.github.filosganga.geogson.model.positions.SinglePosition; import com.google.gson.JsonObject; import io.micronaut.context.annotation.Property; +import io.micronaut.http.HttpResponse; +import io.micronaut.http.HttpStatus; import io.micronaut.http.MediaType; +import io.micronaut.http.exceptions.HttpStatusException; import io.micronaut.http.server.exceptions.InternalServerException; import io.micronaut.http.server.types.files.StreamedFile; import lombok.extern.slf4j.Slf4j; @@ -41,12 +44,14 @@ import org.breedinginsight.utilities.FileUtil; import org.breedinginsight.utilities.Utilities; import org.jetbrains.annotations.NotNull; +import org.breedinginsight.services.lock.DistributedLockService; import javax.inject.Inject; import javax.inject.Singleton; import java.io.IOException; import java.io.PipedInputStream; import java.io.PipedOutputStream; +import java.time.Duration; import java.time.OffsetDateTime; import java.time.format.DateTimeFormatter; import java.util.*; @@ -54,6 +59,7 @@ import java.util.stream.Collectors; import java.util.zip.ZipEntry; import java.util.zip.ZipOutputStream; +import java.util.concurrent.TimeoutException; import static org.breedinginsight.brapps.importer.services.processors.experiment.model.ExpImportProcessConstants.OBSERVATION_UNIT_ID_SUFFIX; @@ -70,8 +76,10 @@ public class BrAPITrialService { private final BrAPIStudyDAO studyDAO; private final BrAPISeasonDAO seasonDAO; private final BrAPIObservationUnitDAO ouDAO; + private final BrAPIObservationLevelDAO observationLevelDAO; private final BrAPIGermplasmDAO germplasmDAO; private final FileMappingUtil fileMappingUtil; + private final DistributedLockService lockService; private static final String SHEET_NAME = "Data"; @Inject @@ -84,8 +92,10 @@ public BrAPITrialService(@Property(name = "brapi.server.reference-source") Strin BrAPIStudyDAO studyDAO, BrAPISeasonDAO seasonDAO, BrAPIObservationUnitDAO ouDAO, + BrAPIObservationLevelDAO observationLevelDAO, BrAPIGermplasmDAO germplasmDAO, - FileMappingUtil fileMappingUtil) { + FileMappingUtil fileMappingUtil, + DistributedLockService lockService) { this.referenceSource = referenceSource; this.trialDAO = trialDAO; @@ -96,8 +106,10 @@ public BrAPITrialService(@Property(name = "brapi.server.reference-source") Strin this.studyDAO = studyDAO; this.seasonDAO = seasonDAO; this.ouDAO = ouDAO; + this.observationLevelDAO = observationLevelDAO; this.germplasmDAO = germplasmDAO; this.fileMappingUtil = fileMappingUtil; + this.lockService = lockService; } public List getExperiments(UUID programId) throws ApiException, DoesNotExistException { @@ -189,7 +201,7 @@ public DownloadFile exportObservations( } //add obsUnitID as dynamic column with observation level appended to header - String observationLvl = ous.get(0).getAdditionalInfo().get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).getAsString(); + String observationLvl = requireObservationLevelName(ous.get(0)); columns = dynamicUpdateObsUnitIDLabel(columns, observationLvl); if (params.getDatasetId() != null) { @@ -394,57 +406,116 @@ public List getDatasetsMetadata(Program program, UUID experimen } public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEntityDatasetRequest request) throws ApiException, DoesNotExistException { - log.debug("creating sub-entity dataset: \"" + request.getName() + "\" for experiment: \"" + experimentId + "\" with: \"" + request.getRepeatedMeasures() + "\" repeated measures."); - UUID subEntityDatasetId = UUID.randomUUID(); - List subObsUnits = new ArrayList<>(); - BrAPITrial experiment = getExperiment(program, experimentId); - // Get top level dataset ObservationUnits. - DatasetMetadata topLevelDataset = DatasetUtil.getTopLevelDataset(experiment); - if (topLevelDataset == null) { - log.error("Experiment {} has no top level dataset.", experiment.getTrialDbId()); - throw new RuntimeException("Cannot create sub-entity dataset for experiment without top level dataset."); - } - - List expOUs = ouDAO.getObservationUnitsForDataset(topLevelDataset.getId().toString(), program); - for (BrAPIObservationUnit expUnit : expOUs) { - - // Get environment number from study. - String envSeqValue = studyDAO.getStudyByDbId(expUnit.getStudyDbId(), program).orElseThrow() - .getAdditionalInfo().get(BrAPIAdditionalInfoFields.ENVIRONMENT_NUMBER).getAsString(); - - for (int i=1; i<=request.getRepeatedMeasures(); i++) { - // Create subObsUnit and add to list. - subObsUnits.add( - createSubObservationUnit( - request.getName(), - Integer.toString(i), - program, - envSeqValue, - expUnit, - this.referenceSource, - subEntityDatasetId, - UUID.randomUUID() - ) - ); - } - } + final String datasetName = request.getName().trim(); + String lockKey = String.format("sub-entity-dataset:%s", experimentId); + try { + return lockService.withLock(lockKey, Duration.ofSeconds(30), Duration.ofMinutes(5), () -> { + log.debug("creating sub-entity dataset: \"{}\" for experiment: \"{}\" with: \"{}\" repeated measures.", datasetName, experimentId, request.getRepeatedMeasures()); + UUID subEntityDatasetId = UUID.randomUUID(); + List subObsUnits = new ArrayList<>(); + List createdObservationUnits = new ArrayList<>(); + boolean createdObservationLevel = false; + BrAPITrial experiment = getExperiment(program, experimentId); + DatasetMetadata topLevelDataset = DatasetUtil.getTopLevelDataset(experiment); + if (topLevelDataset == null) { + log.error("Experiment {} has no top level dataset.", experiment.getTrialDbId()); + throw new RuntimeException("Cannot create sub-entity dataset for experiment without top level dataset."); + } - List createdObservationUnits = observationUnitDAO.createBrAPIObservationUnits(subObsUnits, program.getId()); + List existingDatasets = DatasetUtil.datasetsFromJson(experiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); + if (existingDatasets.stream().anyMatch(dataset -> dataset.getName().equalsIgnoreCase(datasetName))) { + throw new HttpStatusException(HttpStatus.CONFLICT, "Dataset name already exists in this experiment"); + } - // Add the new dataset metadata to the datasets array in the trial's additionalInfo. - DatasetMetadata subEntityDatasetMetadata = DatasetMetadata.builder() - .id(subEntityDatasetId) - .name(request.getName()) - .level(DatasetLevel.SUB_OBS_UNIT) - .build(); - List datasets = DatasetUtil.datasetsFromJson(experiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); - datasets.add(subEntityDatasetMetadata); - experiment.getAdditionalInfo().add(BrAPIAdditionalInfoFields.DATASETS, DatasetUtil.jsonArrayFromDatasets(datasets)); - // Ask the DAO to persist the updated trial. - trialDAO.updateBrAPITrial(experiment.getTrialDbId(), experiment, program.getId()); + HttpResponse levelResponse = observationLevelDAO.createObservationLevelName(program, datasetName, DatasetLevel.SUB_OBS_UNIT); + if (levelResponse.getStatus() == HttpStatus.CONFLICT) { + throw new HttpStatusException(HttpStatus.CONFLICT, "Dataset name already exists in this experiment"); + } else if (levelResponse.getStatus().getCode() < 200 || levelResponse.getStatus().getCode() >= 300) { + throw new ApiException(levelResponse.getStatus().getCode(), "Unable to create observation level: " + levelResponse.getStatus().getReason()); + } + createdObservationLevel = true; + + try { + List expOUs = ouDAO.getObservationUnitsForDataset(topLevelDataset.getId().toString(), program); + for (BrAPIObservationUnit expUnit : expOUs) { + + String envSeqValue = studyDAO.getStudyByDbId(expUnit.getStudyDbId(), program).orElseThrow() + .getAdditionalInfo().get(BrAPIAdditionalInfoFields.ENVIRONMENT_NUMBER).getAsString(); + + for (int i=1; i<=request.getRepeatedMeasures(); i++) { + subObsUnits.add( + createSubObservationUnit( + datasetName, + Integer.toString(i), + program, + envSeqValue, + expUnit, + this.referenceSource, + subEntityDatasetId, + UUID.randomUUID() + ) + ); + } + } + + createdObservationUnits = observationUnitDAO.createBrAPIObservationUnits(subObsUnits, program.getId()); + + DatasetMetadata subEntityDatasetMetadata = DatasetMetadata.builder() + .id(subEntityDatasetId) + .name(datasetName) + .level(DatasetLevel.SUB_OBS_UNIT) + .build(); + + // Refresh experiment so we merge with the latest dataset metadata and avoid clobbering concurrent updates. + BrAPITrial latestExperiment = getExperiment(program, experimentId); + List datasets = DatasetUtil.datasetsFromJson(latestExperiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); + if (datasets.stream().anyMatch(dataset -> dataset.getName().equalsIgnoreCase(datasetName))) { + throw new HttpStatusException(HttpStatus.CONFLICT, "Dataset name already exists in this experiment"); + } + datasets.add(subEntityDatasetMetadata); + latestExperiment.getAdditionalInfo().add(BrAPIAdditionalInfoFields.DATASETS, DatasetUtil.jsonArrayFromDatasets(datasets)); + trialDAO.updateBrAPITrial(latestExperiment.getTrialDbId(), latestExperiment, program.getId()); + + return getDatasetData(program, experimentId, subEntityDatasetId, false); + } catch (Exception e) { + rollbackSubEntityDataset(program, datasetName, createdObservationUnits, createdObservationLevel); + throw e; + } + }); + } catch (TimeoutException e) { + throw new HttpStatusException(HttpStatus.SERVICE_UNAVAILABLE, "Dataset creation is busy, please retry"); + } catch (HttpStatusException e) { + throw e; + } catch (Exception e) { + if (e instanceof ApiException) { + throw (ApiException) e; + } + if (e instanceof DoesNotExistException) { + throw (DoesNotExistException) e; + } + throw new RuntimeException("Unexpected error creating sub-entity dataset", e); + } + } - // Return the new dataset. - return getDatasetData(program, experimentId, subEntityDatasetId, false); + private void rollbackSubEntityDataset(Program program, String datasetName, List createdObservationUnits, boolean createdObservationLevel) { + if (createdObservationUnits != null && !createdObservationUnits.isEmpty()) { + try { + List observationUnitDbIds = createdObservationUnits.stream() + .map(BrAPIObservationUnit::getObservationUnitDbId) + .filter(StringUtils::isNotBlank) + .collect(Collectors.toList()); + observationUnitDAO.deleteObservationUnits(observationUnitDbIds, program.getId()); + } catch (Exception err) { + log.warn("Failed to delete observation units for dataset {} during rollback", datasetName, err); + } + } + if (createdObservationLevel) { + try { + observationLevelDAO.deleteObservationLevelName(program, datasetName); + } catch (Exception err) { + log.warn("Failed to delete observation level {} during rollback", datasetName, err); + } + } } public BrAPIObservationUnit createSubObservationUnit( @@ -512,8 +583,6 @@ public BrAPIObservationUnit createSubObservationUnit( observationUnit.setTreatments(treatmentFactors); } - // Put level in additional info: keep this in case we decide to rename levels in future. - observationUnit.putAdditionalInfoItem(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL, subEntityDatasetName); // Put RTK in additional info. JsonElement rtk = expUnit.getAdditionalInfo().get(BrAPIAdditionalInfoFields.RTK); if (rtk != null) { @@ -533,14 +602,14 @@ public BrAPIObservationUnit createSubObservationUnit( // ObservationLevel entry for Sub-Obs Unit. BrAPIObservationUnitLevelRelationship level = new BrAPIObservationUnitLevelRelationship(); - // TODO: consider removing toLowerCase() after BI-2219 is implemented. - level.setLevelName(subEntityDatasetName.toLowerCase()); + level.setLevelName(subEntityDatasetName); level.setLevelCode(Utilities.appendProgramKey(subUnitId, program.getKey(), seqVal)); level.setLevelOrder(DatasetLevel.SUB_OBS_UNIT.getValue()); position.setObservationLevel(level); // ObservationLevelRelationships. List levelRelationships = new ArrayList<>(); + levelRelationships.add(level); // ObservationLevelRelationships for block. BrAPIObservationUnitLevelRelationship expBlockLevel = expUnit.getObservationUnitPosition() .getObservationLevelRelationships().stream() @@ -565,8 +634,7 @@ public BrAPIObservationUnit createSubObservationUnit( } // ObservationLevelRelationships for top-level Exp Unit linking. BrAPIObservationUnitLevelRelationship expUnitLevel = new BrAPIObservationUnitLevelRelationship(); - // TODO: consider removing toLowerCase() after BI-2219 is implemented. - expUnitLevel.setLevelName(expUnit.getAdditionalInfo().get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).getAsString().toLowerCase()); + expUnitLevel.setLevelName(requireObservationLevelName(expUnit)); String expUnitUUID = Utilities.getExternalReference(expUnit.getExternalReferences(), referenceSource, ExternalReferenceSource.OBSERVATION_UNITS).orElseThrow().getReferenceId(); expUnitLevel.setLevelCode(Utilities.appendProgramKey(expUnitUUID, program.getKey(), seqVal)); expUnitLevel.setLevelOrder(DatasetLevel.EXP_UNIT.getValue()); @@ -581,6 +649,29 @@ public BrAPIObservationUnit createSubObservationUnit( return observationUnit; } + private String getObservationLevelName(BrAPIObservationUnit observationUnit) { + if (observationUnit.getObservationUnitPosition() != null + && observationUnit.getObservationUnitPosition().getObservationLevel() != null + && StringUtils.isNotBlank(observationUnit.getObservationUnitPosition().getObservationLevel().getLevelName())) { + return observationUnit.getObservationUnitPosition().getObservationLevel().getLevelName(); + } + JsonObject additionalInfo = observationUnit.getAdditionalInfo(); + if (additionalInfo != null + && additionalInfo.has(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL) + && !additionalInfo.get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).isJsonNull()) { + return additionalInfo.get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).getAsString(); + } + return null; + } + + private String requireObservationLevelName(BrAPIObservationUnit observationUnit) { + String levelName = getObservationLevelName(observationUnit); + if (StringUtils.isBlank(levelName)) { + throw new RuntimeException("Observation level not found for observation unit " + observationUnit.getObservationUnitDbId()); + } + return levelName; + } + private void addBrAPIObsToRecords( List dataset, BrAPITrial experiment, @@ -750,7 +841,7 @@ private Map createExportRow( row.put(ExperimentObservation.Columns.TEST_CHECK, testCheck); row.put(ExperimentObservation.Columns.EXP_TITLE, Utilities.removeProgramKey(experiment.getTrialName(), program.getKey())); row.put(ExperimentObservation.Columns.EXP_DESCRIPTION, experiment.getTrialDescription()); - row.put(ExperimentObservation.Columns.EXP_UNIT, ou.getAdditionalInfo().getAsJsonObject().get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).getAsString()); + row.put(ExperimentObservation.Columns.EXP_UNIT, requireObservationLevelName(ou)); row.put(ExperimentObservation.Columns.EXP_TYPE, experiment.getAdditionalInfo().getAsJsonObject().get(BrAPIAdditionalInfoFields.EXPERIMENT_TYPE).getAsString()); row.put(ExperimentObservation.Columns.ENV, Utilities.removeProgramKeyAndUnknownAdditionalData(study.getStudyName(), program.getKey())); row.put(ExperimentObservation.Columns.ENV_LOCATION, Utilities.removeProgramKey(study.getLocationName(), program.getKey())); @@ -807,7 +898,7 @@ private Map createExportRow( } //Append observation level to obsUnitID - String observationLvl = ou.getAdditionalInfo().getAsJsonObject().get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).getAsString(); + String observationLvl = requireObservationLevelName(ou); row.put(observationLvl + " " + OBSERVATION_UNIT_ID_SUFFIX, ouId); return row; diff --git a/src/main/java/org/breedinginsight/services/lock/DistributedLockService.java b/src/main/java/org/breedinginsight/services/lock/DistributedLockService.java new file mode 100644 index 000000000..06f141f33 --- /dev/null +++ b/src/main/java/org/breedinginsight/services/lock/DistributedLockService.java @@ -0,0 +1,61 @@ +package org.breedinginsight.services.lock; + +import lombok.extern.slf4j.Slf4j; +import org.redisson.api.RLock; +import org.redisson.api.RedissonClient; + +import javax.inject.Inject; +import javax.inject.Singleton; +import java.time.Duration; +import java.util.concurrent.Callable; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +/** + * Small helper to provide a consistent pattern for distributed locks across the service layer. + */ +@Slf4j +@Singleton +public class DistributedLockService { + + private final RedissonClient redissonClient; + + @Inject + public DistributedLockService(RedissonClient redissonClient) { + this.redissonClient = redissonClient; + } + + /** + * Execute the given callback guarded by a distributed lock. + * + * @param lockKey the key for the distributed lock + * @param waitTime how long to wait to acquire the lock + * @param leaseTime how long before the lock automatically releases + * @param action the work to run while holding the lock + * @return result of the callback + * @throws TimeoutException if the lock cannot be acquired within the wait time + * @throws Exception bubbled up from the callback + */ + public T withLock(String lockKey, Duration waitTime, Duration leaseTime, Callable action) throws Exception { + RLock lock = redissonClient.getLock(lockKey); + boolean acquired = false; + try { + acquired = lock.tryLock(waitTime.toMillis(), leaseTime.toMillis(), TimeUnit.MILLISECONDS); + if (!acquired) { + throw new TimeoutException("Unable to acquire lock " + lockKey); + } + return action.call(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new TimeoutException("Interrupted while acquiring lock " + lockKey); + } finally { + if (acquired && lock.isHeldByCurrentThread()) { + try { + lock.unlock(); + } catch (Exception e) { + log.warn("Failed to release lock {}", lockKey, e); + } + } + } + } +} diff --git a/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java b/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java new file mode 100644 index 000000000..de48032bc --- /dev/null +++ b/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java @@ -0,0 +1,108 @@ +package org.breedinginsight.brapi.v2; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; +import io.micronaut.context.annotation.Property; +import io.micronaut.http.HttpRequest; +import io.micronaut.http.HttpResponse; +import io.micronaut.http.HttpStatus; +import io.micronaut.http.MediaType; +import io.micronaut.http.client.RxHttpClient; +import io.micronaut.http.client.annotation.Client; +import io.micronaut.test.extensions.junit5.annotation.MicronautTest; +import io.reactivex.Flowable; +import org.breedinginsight.BrAPITest; +import org.breedinginsight.model.Program; +import org.junit.jupiter.api.*; + +import javax.inject.Inject; +import java.time.OffsetDateTime; +import java.util.List; +import java.util.Objects; +import java.util.UUID; +import java.util.concurrent.*; +import java.util.stream.Collectors; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@MicronautTest +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +public class SubEntityDatasetLockIntegrationTest extends BrAPITest { + + private Program program; + private String experimentId; + + @Inject + private BrAPITestUtils brAPITestUtils; + + @Inject + @Client("/${micronaut.bi.api.version}") + private RxHttpClient client; + + private final Gson gson = new GsonBuilder().registerTypeAdapter(OffsetDateTime.class, (json, type, context) -> OffsetDateTime.parse(json.getAsString())).create(); + + @BeforeAll + void setup() throws Exception { + var setup = brAPITestUtils.setupTestProgram(super.getBrapiDsl(), gson); + program = setup.getV1(); + experimentId = setup.getV2().get(0); + } + + @Test + void concurrentDatasetCreateReturnsSingleSuccessAndConflict() throws Exception { + // Use a fresh name to avoid interference with other runs + String datasetName = "LockTest-" + UUID.randomUUID(); + JsonObject request = new JsonObject(); + request.addProperty("name", datasetName); + request.addProperty("repeatedMeasures", 1); + + ExecutorService executor = Executors.newFixedThreadPool(2); + CountDownLatch start = new CountDownLatch(1); + + Callable call = () -> { + start.await(1, TimeUnit.SECONDS); + Flowable> response = client.exchange( + HttpRequest.POST(String.format("/programs/%s/experiments/%s/dataset", program.getId(), experimentId), request.toString()) + .contentType(MediaType.APPLICATION_JSON) + .bearerAuth("test-registered-user"), + String.class + ); + return response.blockingFirst().getStatus(); + }; + + Future first = executor.submit(call); + Future second = executor.submit(call); + start.countDown(); + + HttpStatus status1 = first.get(10, TimeUnit.SECONDS); + HttpStatus status2 = second.get(10, TimeUnit.SECONDS); + executor.shutdownNow(); + + List statuses = List.of(status1, status2); + assertTrue(statuses.contains(HttpStatus.OK)); + assertTrue(statuses.contains(HttpStatus.CONFLICT)); + + // Confirm only one dataset with that name exists + Flowable> datasetsCall = client.exchange( + HttpRequest.GET(String.format("/programs/%s/experiments/%s/datasets", program.getId(), experimentId)) + .bearerAuth("test-registered-user"), + String.class + ); + HttpResponse datasetsResponse = datasetsCall.blockingFirst(); + assertEquals(HttpStatus.OK, datasetsResponse.getStatus()); + var datasetsJson = JsonParser.parseString(Objects.requireNonNull(datasetsResponse.body())).getAsJsonObject() + .getAsJsonObject("result") + .getAsJsonArray("data"); + long matching = 0; + for (int i = 0; i < datasetsJson.size(); i++) { + String name = datasetsJson.get(i).getAsJsonObject().get("name").getAsString(); + if (name.equalsIgnoreCase(datasetName)) { + matching++; + } + } + assertEquals(1, matching); + } +} diff --git a/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java b/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java new file mode 100644 index 000000000..4a0522c70 --- /dev/null +++ b/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java @@ -0,0 +1,57 @@ +package org.breedinginsight.services.lock; + +import org.breedinginsight.DatabaseTest; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class DistributedLockServiceTest extends DatabaseTest { + + private final ExecutorService executor = Executors.newFixedThreadPool(2); + + @AfterEach + void cleanup() { + executor.shutdownNow(); + } + + @Test + void secondLockAttemptTimesOutWhileFirstHolds() throws Exception { + DistributedLockService lockService = new DistributedLockService(super.getRedisConnection()); + String lockKey = "test-lock-key"; + + CountDownLatch firstAcquired = new CountDownLatch(1); + CountDownLatch releaseFirst = new CountDownLatch(1); + + Future firstCall = executor.submit(() -> + lockService.withLock(lockKey, Duration.ofMillis(500), Duration.ofSeconds(5), () -> { + firstAcquired.countDown(); + // keep the lock held until signaled + releaseFirst.await(2, TimeUnit.SECONDS); + return "first"; + }) + ); + + assertTrue(firstAcquired.await(1, TimeUnit.SECONDS), "First lock holder did not start in time"); + + assertThrows(TimeoutException.class, () -> + lockService.withLock(lockKey, Duration.ofMillis(100), Duration.ofSeconds(2), () -> "second") + ); + + releaseFirst.countDown(); + assertEquals("first", firstCall.get(2, TimeUnit.SECONDS)); + + String afterRelease = lockService.withLock(lockKey, Duration.ofMillis(500), Duration.ofSeconds(2), () -> "after-release"); + assertEquals("after-release", afterRelease); + } +} From b2785aef376aae487f391b834f4ab34ebfcb6bca Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Wed, 7 Jan 2026 14:35:59 -0500 Subject: [PATCH 2/8] Moved httpstatus codes to controller and fixed post /observationlevelnames --- .../v1/controller/ExperimentController.java | 10 ++- .../v2/dao/BrAPIObservationLevelDAO.java | 67 ++++++++++++++++--- .../brapi/v2/services/BrAPITrialService.java | 39 ++++++----- .../exceptions/CreationBusyException.java | 7 ++ 4 files changed, 92 insertions(+), 31 deletions(-) create mode 100644 src/main/java/org/breedinginsight/services/exceptions/CreationBusyException.java diff --git a/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java b/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java index fe5436cd6..5741e1313 100644 --- a/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java +++ b/src/main/java/org/breedinginsight/api/v1/controller/ExperimentController.java @@ -5,7 +5,6 @@ import io.micronaut.http.HttpStatus; import io.micronaut.http.MediaType; import io.micronaut.http.annotation.*; -import io.micronaut.http.exceptions.HttpStatusException; import io.micronaut.http.server.types.files.StreamedFile; import io.micronaut.security.annotation.Secured; import io.micronaut.security.rules.SecurityRule; @@ -29,7 +28,9 @@ import org.breedinginsight.services.ProgramService; import org.breedinginsight.services.ProgramUserService; import org.breedinginsight.services.RoleService; +import org.breedinginsight.services.exceptions.AlreadyExistsException; import org.breedinginsight.services.exceptions.DoesNotExistException; +import org.breedinginsight.services.exceptions.CreationBusyException; import org.breedinginsight.utilities.response.mappers.ExperimentQueryMapper; import javax.inject.Inject; @@ -135,9 +136,12 @@ public HttpResponse> createSubEntityDataset( Response response = new Response(experimentService.createSubEntityDataset(programOptional.get(), experimentId, datasetRequest)); return HttpResponse.ok(response); - } catch (HttpStatusException e) { + } catch (AlreadyExistsException e) { log.info(e.getMessage()); - return HttpResponse.status(e.getStatus(), e.getMessage()); + return HttpResponse.status(HttpStatus.CONFLICT, e.getMessage()); + } catch (CreationBusyException e) { + log.info(e.getMessage()); + return HttpResponse.status(HttpStatus.SERVICE_UNAVAILABLE, e.getMessage()); } catch (Exception e){ log.info(e.getMessage()); return HttpResponse.status(HttpStatus.UNPROCESSABLE_ENTITY, e.getMessage()); diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java index 1af5aa33a..5fad8981d 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java @@ -27,10 +27,13 @@ import okhttp3.RequestBody; import org.brapi.client.v2.JSON; import org.brapi.client.v2.model.exceptions.ApiException; -import org.brapi.v2.model.pheno.BrAPIObservationUnitHierarchyLevel; import org.breedinginsight.model.DatasetLevel; import org.breedinginsight.model.Program; import org.breedinginsight.utilities.BrAPIDAOUtil; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; import javax.inject.Inject; import javax.inject.Singleton; @@ -48,17 +51,22 @@ public BrAPIObservationLevelDAO(BrAPIDAOUtil brAPIDAOUtil) { this.brAPIDAOUtil = brAPIDAOUtil; } - public HttpResponse createObservationLevelName(Program program, String levelName, DatasetLevel levelOrder) throws ApiException { + public HttpResponse createObservationLevelName(Program program, String levelName, DatasetLevel levelOrder, String programDbId) throws ApiException { HttpUrl url = HttpUrl.parse(brAPIDAOUtil.getProgramBrAPIBaseUrl(program.getId())) .newBuilder() .addPathSegment("observationlevelnames") .build(); - BrAPIObservationUnitHierarchyLevel level = new BrAPIObservationUnitHierarchyLevel() - .levelName(levelName); + JsonObject levelJson = new JsonObject(); + levelJson.addProperty("levelName", levelName); if (levelOrder != null) { - level.setLevelOrder(levelOrder.getValue()); + levelJson.addProperty("levelOrder", levelOrder.getValue()); } - RequestBody body = RequestBody.create(gson.toJson(level), JSON_MEDIA_TYPE); + if (programDbId != null) { + levelJson.addProperty("programDbId", programDbId); + } + JsonArray bodyArray = new JsonArray(); + bodyArray.add(levelJson); + RequestBody body = RequestBody.create(gson.toJson(bodyArray), JSON_MEDIA_TYPE); var request = new Request.Builder() .url(url) .post(body) @@ -67,11 +75,11 @@ public HttpResponse createObservationLevelName(Program program, String l return brAPIDAOUtil.makeCall(request); } - public void deleteObservationLevelName(Program program, String levelName) { + public void deleteObservationLevelName(Program program, String levelDbId) { HttpUrl url = HttpUrl.parse(brAPIDAOUtil.getProgramBrAPIBaseUrl(program.getId())) .newBuilder() .addPathSegment("observationlevelnames") - .addPathSegment(levelName) + .addPathSegment(levelDbId) .build(); var request = new Request.Builder() .url(url) @@ -81,10 +89,49 @@ public void deleteObservationLevelName(Program program, String levelName) { try { HttpResponse response = brAPIDAOUtil.makeCall(request); if (response.getStatus() != HttpStatus.OK && response.getStatus() != HttpStatus.NO_CONTENT && response.getStatus() != HttpStatus.ACCEPTED) { - log.warn("Observation level delete returned status {} for {}", response.getStatus(), levelName); + log.warn("Observation level delete returned status {} for {}", response.getStatus(), levelDbId); + } + } catch (Exception e) { + log.warn("Failed to delete observation level {}", levelDbId, e); + } + } + + public String extractObservationLevelDbId(HttpResponse response) { + try { + String body = response.getBody().orElse(null); + if (body == null || body.isBlank()) { + return null; + } + JsonElement root = JsonParser.parseString(body); + JsonArray dataArray = null; + if (root.isJsonArray()) { + dataArray = root.getAsJsonArray(); + } else if (root.isJsonObject()) { + JsonObject rootObj = root.getAsJsonObject(); + if (rootObj.has("result") && rootObj.get("result").isJsonObject()) { + JsonObject resultObj = rootObj.getAsJsonObject("result"); + if (resultObj.has("data") && resultObj.get("data").isJsonArray()) { + dataArray = resultObj.getAsJsonArray("data"); + } + } else if (rootObj.has("data") && rootObj.get("data").isJsonArray()) { + dataArray = rootObj.getAsJsonArray("data"); + } + } + if (dataArray == null || dataArray.size() == 0) { + return null; + } + for (JsonElement element : dataArray) { + if (!element.isJsonObject()) { + continue; + } + JsonObject obj = element.getAsJsonObject(); + if (obj.has("levelNameDbId")) { + return obj.get("levelNameDbId").getAsString(); + } } } catch (Exception e) { - log.warn("Failed to delete observation level {} during rollback", levelName, e); + log.warn("Failed to parse level name id from response", e); } + return null; } } diff --git a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java index 96337c026..90cbb1118 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java +++ b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java @@ -9,7 +9,6 @@ import io.micronaut.http.HttpResponse; import io.micronaut.http.HttpStatus; import io.micronaut.http.MediaType; -import io.micronaut.http.exceptions.HttpStatusException; import io.micronaut.http.server.exceptions.InternalServerException; import io.micronaut.http.server.types.files.StreamedFile; import lombok.extern.slf4j.Slf4j; @@ -37,7 +36,9 @@ import org.breedinginsight.model.Program; import org.breedinginsight.model.*; import org.breedinginsight.services.TraitService; +import org.breedinginsight.services.exceptions.AlreadyExistsException; import org.breedinginsight.services.exceptions.DoesNotExistException; +import org.breedinginsight.services.exceptions.CreationBusyException; import org.breedinginsight.services.parsers.experiment.ExperimentFileColumns; import org.breedinginsight.utilities.DatasetUtil; import org.breedinginsight.utilities.IntOrderComparator; @@ -405,7 +406,8 @@ public List getDatasetsMetadata(Program program, UUID experimen return datasets; } - public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEntityDatasetRequest request) throws ApiException, DoesNotExistException { + public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEntityDatasetRequest request) + throws ApiException, DoesNotExistException, AlreadyExistsException, CreationBusyException { final String datasetName = request.getName().trim(); String lockKey = String.format("sub-entity-dataset:%s", experimentId); try { @@ -415,6 +417,7 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt List subObsUnits = new ArrayList<>(); List createdObservationUnits = new ArrayList<>(); boolean createdObservationLevel = false; + String createdObservationLevelDbId = null; BrAPITrial experiment = getExperiment(program, experimentId); DatasetMetadata topLevelDataset = DatasetUtil.getTopLevelDataset(experiment); if (topLevelDataset == null) { @@ -424,16 +427,18 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt List existingDatasets = DatasetUtil.datasetsFromJson(experiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); if (existingDatasets.stream().anyMatch(dataset -> dataset.getName().equalsIgnoreCase(datasetName))) { - throw new HttpStatusException(HttpStatus.CONFLICT, "Dataset name already exists in this experiment"); + throw new AlreadyExistsException("Dataset name already exists in this experiment"); } - HttpResponse levelResponse = observationLevelDAO.createObservationLevelName(program, datasetName, DatasetLevel.SUB_OBS_UNIT); + String programDbId = program.getBrapiProgram() != null ? program.getBrapiProgram().getProgramDbId() : null; + HttpResponse levelResponse = observationLevelDAO.createObservationLevelName(program, datasetName, DatasetLevel.SUB_OBS_UNIT, programDbId); if (levelResponse.getStatus() == HttpStatus.CONFLICT) { - throw new HttpStatusException(HttpStatus.CONFLICT, "Dataset name already exists in this experiment"); + throw new AlreadyExistsException("Dataset name already exists in this experiment"); } else if (levelResponse.getStatus().getCode() < 200 || levelResponse.getStatus().getCode() >= 300) { throw new ApiException(levelResponse.getStatus().getCode(), "Unable to create observation level: " + levelResponse.getStatus().getReason()); } createdObservationLevel = true; + createdObservationLevelDbId = observationLevelDAO.extractObservationLevelDbId(levelResponse); try { List expOUs = ouDAO.getObservationUnitsForDataset(topLevelDataset.getId().toString(), program); @@ -470,7 +475,7 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt BrAPITrial latestExperiment = getExperiment(program, experimentId); List datasets = DatasetUtil.datasetsFromJson(latestExperiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); if (datasets.stream().anyMatch(dataset -> dataset.getName().equalsIgnoreCase(datasetName))) { - throw new HttpStatusException(HttpStatus.CONFLICT, "Dataset name already exists in this experiment"); + throw new AlreadyExistsException("Dataset name already exists in this experiment"); } datasets.add(subEntityDatasetMetadata); latestExperiment.getAdditionalInfo().add(BrAPIAdditionalInfoFields.DATASETS, DatasetUtil.jsonArrayFromDatasets(datasets)); @@ -478,26 +483,20 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt return getDatasetData(program, experimentId, subEntityDatasetId, false); } catch (Exception e) { - rollbackSubEntityDataset(program, datasetName, createdObservationUnits, createdObservationLevel); + rollbackSubEntityDataset(program, datasetName, createdObservationUnits, createdObservationLevel, createdObservationLevelDbId); throw e; } }); } catch (TimeoutException e) { - throw new HttpStatusException(HttpStatus.SERVICE_UNAVAILABLE, "Dataset creation is busy, please retry"); - } catch (HttpStatusException e) { + throw new CreationBusyException("Dataset creation is busy, please retry"); + } catch (ApiException | DoesNotExistException | AlreadyExistsException | CreationBusyException e) { throw e; } catch (Exception e) { - if (e instanceof ApiException) { - throw (ApiException) e; - } - if (e instanceof DoesNotExistException) { - throw (DoesNotExistException) e; - } throw new RuntimeException("Unexpected error creating sub-entity dataset", e); } } - private void rollbackSubEntityDataset(Program program, String datasetName, List createdObservationUnits, boolean createdObservationLevel) { + private void rollbackSubEntityDataset(Program program, String datasetName, List createdObservationUnits, boolean createdObservationLevel, String createdObservationLevelDbId) { if (createdObservationUnits != null && !createdObservationUnits.isEmpty()) { try { List observationUnitDbIds = createdObservationUnits.stream() @@ -511,9 +510,13 @@ private void rollbackSubEntityDataset(Program program, String datasetName, List< } if (createdObservationLevel) { try { - observationLevelDAO.deleteObservationLevelName(program, datasetName); + if (StringUtils.isNotBlank(createdObservationLevelDbId)) { + observationLevelDAO.deleteObservationLevelName(program, createdObservationLevelDbId); + } else { + log.warn("Observation level id missing for dataset {} rollback; skipping level delete", datasetName); + } } catch (Exception err) { - log.warn("Failed to delete observation level {} during rollback", datasetName, err); + log.warn("Failed to delete observation level {} during rollback", createdObservationLevelDbId, err); } } } diff --git a/src/main/java/org/breedinginsight/services/exceptions/CreationBusyException.java b/src/main/java/org/breedinginsight/services/exceptions/CreationBusyException.java new file mode 100644 index 000000000..d32536159 --- /dev/null +++ b/src/main/java/org/breedinginsight/services/exceptions/CreationBusyException.java @@ -0,0 +1,7 @@ +package org.breedinginsight.services.exceptions; + +public class CreationBusyException extends Exception { + public CreationBusyException(String message) { + super(message); + } +} From 36b67b99ff6432d8bf244d98903f26ca4be1dc73 Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Thu, 8 Jan 2026 10:59:44 -0500 Subject: [PATCH 3/8] Removed rollback code until brapi server supports observation unit deletes --- .../brapi/v2/dao/BrAPIObservationUnitDAO.java | 28 ---- .../brapi/v2/services/BrAPITrialService.java | 124 ++++++++---------- 2 files changed, 52 insertions(+), 100 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java index 5a86c2337..4b761654a 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java @@ -444,32 +444,4 @@ private void preprocessObservationUnits(List brapiObservat } } } - - public void deleteObservationUnits(Collection observationUnitDbIds, UUID programId) { - if (observationUnitDbIds == null || observationUnitDbIds.isEmpty()) { - return; - } - String baseUrl = brAPIDAOUtil.getProgramBrAPIBaseUrl(programId); - for (String ouDbId : observationUnitDbIds) { - if (StringUtils.isBlank(ouDbId)) { - continue; - } - HttpUrl url = HttpUrl.parse(baseUrl) - .newBuilder() - .addPathSegment("observationunits") - .addPathSegment(ouDbId) - .build(); - Request request = new Request.Builder() - .url(url) - .delete() - .addHeader("Content-Type", "application/json") - .build(); - try { - brAPIDAOUtil.makeCall(request); - } catch (Exception e) { - log.warn("Failed to delete observation unit {} during rollback", ouDbId, e); - } - } - repopulateCache(programId); - } } diff --git a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java index 90cbb1118..7669e56fc 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java +++ b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java @@ -406,6 +406,21 @@ public List getDatasetsMetadata(Program program, UUID experimen return datasets; } + /** + * Creates sub-entity dataset + * TODO: Handle compensating transactions in event of failure. Currently brapi server does not support + * deleting observation units. Will need to add batch delete support for observation units before this + * can be done. + * + * @param program + * @param experimentId + * @param request + * @return + * @throws ApiException + * @throws DoesNotExistException + * @throws AlreadyExistsException + * @throws CreationBusyException + */ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEntityDatasetRequest request) throws ApiException, DoesNotExistException, AlreadyExistsException, CreationBusyException { final String datasetName = request.getName().trim(); @@ -415,9 +430,6 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt log.debug("creating sub-entity dataset: \"{}\" for experiment: \"{}\" with: \"{}\" repeated measures.", datasetName, experimentId, request.getRepeatedMeasures()); UUID subEntityDatasetId = UUID.randomUUID(); List subObsUnits = new ArrayList<>(); - List createdObservationUnits = new ArrayList<>(); - boolean createdObservationLevel = false; - String createdObservationLevelDbId = null; BrAPITrial experiment = getExperiment(program, experimentId); DatasetMetadata topLevelDataset = DatasetUtil.getTopLevelDataset(experiment); if (topLevelDataset == null) { @@ -437,55 +449,48 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt } else if (levelResponse.getStatus().getCode() < 200 || levelResponse.getStatus().getCode() >= 300) { throw new ApiException(levelResponse.getStatus().getCode(), "Unable to create observation level: " + levelResponse.getStatus().getReason()); } - createdObservationLevel = true; - createdObservationLevelDbId = observationLevelDAO.extractObservationLevelDbId(levelResponse); - - try { - List expOUs = ouDAO.getObservationUnitsForDataset(topLevelDataset.getId().toString(), program); - for (BrAPIObservationUnit expUnit : expOUs) { - - String envSeqValue = studyDAO.getStudyByDbId(expUnit.getStudyDbId(), program).orElseThrow() - .getAdditionalInfo().get(BrAPIAdditionalInfoFields.ENVIRONMENT_NUMBER).getAsString(); - - for (int i=1; i<=request.getRepeatedMeasures(); i++) { - subObsUnits.add( - createSubObservationUnit( - datasetName, - Integer.toString(i), - program, - envSeqValue, - expUnit, - this.referenceSource, - subEntityDatasetId, - UUID.randomUUID() - ) - ); - } + + List expOUs = ouDAO.getObservationUnitsForDataset(topLevelDataset.getId().toString(), program); + for (BrAPIObservationUnit expUnit : expOUs) { + + String envSeqValue = studyDAO.getStudyByDbId(expUnit.getStudyDbId(), program).orElseThrow() + .getAdditionalInfo().get(BrAPIAdditionalInfoFields.ENVIRONMENT_NUMBER).getAsString(); + + for (int i=1; i<=request.getRepeatedMeasures(); i++) { + subObsUnits.add( + createSubObservationUnit( + datasetName, + Integer.toString(i), + program, + envSeqValue, + expUnit, + this.referenceSource, + subEntityDatasetId, + UUID.randomUUID() + ) + ); } + } - createdObservationUnits = observationUnitDAO.createBrAPIObservationUnits(subObsUnits, program.getId()); + observationUnitDAO.createBrAPIObservationUnits(subObsUnits, program.getId()); - DatasetMetadata subEntityDatasetMetadata = DatasetMetadata.builder() - .id(subEntityDatasetId) - .name(datasetName) - .level(DatasetLevel.SUB_OBS_UNIT) - .build(); + DatasetMetadata subEntityDatasetMetadata = DatasetMetadata.builder() + .id(subEntityDatasetId) + .name(datasetName) + .level(DatasetLevel.SUB_OBS_UNIT) + .build(); - // Refresh experiment so we merge with the latest dataset metadata and avoid clobbering concurrent updates. - BrAPITrial latestExperiment = getExperiment(program, experimentId); - List datasets = DatasetUtil.datasetsFromJson(latestExperiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); - if (datasets.stream().anyMatch(dataset -> dataset.getName().equalsIgnoreCase(datasetName))) { - throw new AlreadyExistsException("Dataset name already exists in this experiment"); - } - datasets.add(subEntityDatasetMetadata); - latestExperiment.getAdditionalInfo().add(BrAPIAdditionalInfoFields.DATASETS, DatasetUtil.jsonArrayFromDatasets(datasets)); - trialDAO.updateBrAPITrial(latestExperiment.getTrialDbId(), latestExperiment, program.getId()); - - return getDatasetData(program, experimentId, subEntityDatasetId, false); - } catch (Exception e) { - rollbackSubEntityDataset(program, datasetName, createdObservationUnits, createdObservationLevel, createdObservationLevelDbId); - throw e; + // Refresh experiment so we merge with the latest dataset metadata and avoid clobbering concurrent updates. + BrAPITrial latestExperiment = getExperiment(program, experimentId); + List datasets = DatasetUtil.datasetsFromJson(latestExperiment.getAdditionalInfo().getAsJsonArray(BrAPIAdditionalInfoFields.DATASETS)); + if (datasets.stream().anyMatch(dataset -> dataset.getName().equalsIgnoreCase(datasetName))) { + throw new AlreadyExistsException("Dataset name already exists in this experiment"); } + datasets.add(subEntityDatasetMetadata); + latestExperiment.getAdditionalInfo().add(BrAPIAdditionalInfoFields.DATASETS, DatasetUtil.jsonArrayFromDatasets(datasets)); + trialDAO.updateBrAPITrial(latestExperiment.getTrialDbId(), latestExperiment, program.getId()); + + return getDatasetData(program, experimentId, subEntityDatasetId, false); }); } catch (TimeoutException e) { throw new CreationBusyException("Dataset creation is busy, please retry"); @@ -496,31 +501,6 @@ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEnt } } - private void rollbackSubEntityDataset(Program program, String datasetName, List createdObservationUnits, boolean createdObservationLevel, String createdObservationLevelDbId) { - if (createdObservationUnits != null && !createdObservationUnits.isEmpty()) { - try { - List observationUnitDbIds = createdObservationUnits.stream() - .map(BrAPIObservationUnit::getObservationUnitDbId) - .filter(StringUtils::isNotBlank) - .collect(Collectors.toList()); - observationUnitDAO.deleteObservationUnits(observationUnitDbIds, program.getId()); - } catch (Exception err) { - log.warn("Failed to delete observation units for dataset {} during rollback", datasetName, err); - } - } - if (createdObservationLevel) { - try { - if (StringUtils.isNotBlank(createdObservationLevelDbId)) { - observationLevelDAO.deleteObservationLevelName(program, createdObservationLevelDbId); - } else { - log.warn("Observation level id missing for dataset {} rollback; skipping level delete", datasetName); - } - } catch (Exception err) { - log.warn("Failed to delete observation level {} during rollback", createdObservationLevelDbId, err); - } - } - } - public BrAPIObservationUnit createSubObservationUnit( String subEntityDatasetName, String subUnitId, From fdf5f2e24c43aa67e19780553e5e895dbdd28fde Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Thu, 8 Jan 2026 14:17:21 -0500 Subject: [PATCH 4/8] Removed unused code --- .../v2/dao/BrAPIObservationLevelDAO.java | 38 ------------------- .../brapi/v2/dao/BrAPIObservationUnitDAO.java | 2 - .../brapi/v2/services/BrAPITrialService.java | 2 +- 3 files changed, 1 insertion(+), 41 deletions(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java index 5fad8981d..e37e23dbe 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationLevelDAO.java @@ -96,42 +96,4 @@ public void deleteObservationLevelName(Program program, String levelDbId) { } } - public String extractObservationLevelDbId(HttpResponse response) { - try { - String body = response.getBody().orElse(null); - if (body == null || body.isBlank()) { - return null; - } - JsonElement root = JsonParser.parseString(body); - JsonArray dataArray = null; - if (root.isJsonArray()) { - dataArray = root.getAsJsonArray(); - } else if (root.isJsonObject()) { - JsonObject rootObj = root.getAsJsonObject(); - if (rootObj.has("result") && rootObj.get("result").isJsonObject()) { - JsonObject resultObj = rootObj.getAsJsonObject("result"); - if (resultObj.has("data") && resultObj.get("data").isJsonArray()) { - dataArray = resultObj.getAsJsonArray("data"); - } - } else if (rootObj.has("data") && rootObj.get("data").isJsonArray()) { - dataArray = rootObj.getAsJsonArray("data"); - } - } - if (dataArray == null || dataArray.size() == 0) { - return null; - } - for (JsonElement element : dataArray) { - if (!element.isJsonObject()) { - continue; - } - JsonObject obj = element.getAsJsonObject(); - if (obj.has("levelNameDbId")) { - return obj.get("levelNameDbId").getAsString(); - } - } - } catch (Exception e) { - log.warn("Failed to parse level name id from response", e); - } - return null; - } } diff --git a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java index 4b761654a..9749bf093 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java +++ b/src/main/java/org/breedinginsight/brapi/v2/dao/BrAPIObservationUnitDAO.java @@ -26,8 +26,6 @@ import io.micronaut.scheduling.annotation.Scheduled; import lombok.extern.slf4j.Slf4j; import org.apache.commons.lang3.StringUtils; -import okhttp3.HttpUrl; -import okhttp3.Request; import org.brapi.client.v2.JSON; import org.brapi.client.v2.model.exceptions.ApiException; import org.brapi.client.v2.modules.phenotype.ObservationUnitsApi; diff --git a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java index 7669e56fc..3648e2505 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java +++ b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java @@ -423,7 +423,7 @@ public List getDatasetsMetadata(Program program, UUID experimen */ public Dataset createSubEntityDataset(Program program, UUID experimentId, SubEntityDatasetRequest request) throws ApiException, DoesNotExistException, AlreadyExistsException, CreationBusyException { - final String datasetName = request.getName().trim(); + final String datasetName = request.getName().trim().toLowerCase(); String lockKey = String.format("sub-entity-dataset:%s", experimentId); try { return lockService.withLock(lockKey, Duration.ofSeconds(30), Duration.ofMinutes(5), () -> { From 10423985a5b15d6ead58de51fd6a595afddb0184 Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Fri, 9 Jan 2026 14:10:53 -0500 Subject: [PATCH 5/8] Fix missing observationLevel in additionalInfo --- .../breedinginsight/brapi/v2/services/BrAPITrialService.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java index df7570c64..4c217ca66 100644 --- a/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java +++ b/src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java @@ -587,6 +587,9 @@ public BrAPIObservationUnit createSubObservationUnit( observationUnit.setTreatments(treatmentFactors); } + // Put level in additional info: keep this in case we decide to rename levels in future. + observationUnit.putAdditionalInfoItem(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL, subEntityDatasetName); + // Put RTK in additional info. JsonElement rtk = expUnit.getAdditionalInfo().get(BrAPIAdditionalInfoFields.RTK); if (rtk != null) { @@ -638,7 +641,7 @@ public BrAPIObservationUnit createSubObservationUnit( } // ObservationLevelRelationships for top-level Exp Unit linking. BrAPIObservationUnitLevelRelationship expUnitLevel = new BrAPIObservationUnitLevelRelationship(); - expUnitLevel.setLevelName(requireObservationLevelName(expUnit)); + expUnitLevel.setLevelName(expUnit.getAdditionalInfo().get(BrAPIAdditionalInfoFields.OBSERVATION_LEVEL).getAsString()); String expUnitUUID = Utilities.getExternalReference(expUnit.getExternalReferences(), referenceSource, ExternalReferenceSource.OBSERVATION_UNITS).orElseThrow().getReferenceId(); expUnitLevel.setLevelCode(Utilities.appendProgramKey(expUnitUUID, program.getKey(), seqVal)); expUnitLevel.setLevelOrder(DatasetLevel.EXP_UNIT.getValue()); From 50146d7bd0af247ffceda058f84c5d5939b7047d Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Fri, 9 Jan 2026 17:18:33 -0500 Subject: [PATCH 6/8] Fix test --- .../services/lock/DistributedLockServiceTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java b/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java index 4a0522c70..bb3275beb 100644 --- a/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java +++ b/src/test/java/org/breedinginsight/services/lock/DistributedLockServiceTest.java @@ -3,6 +3,7 @@ import org.breedinginsight.DatabaseTest; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; import java.time.Duration; import java.util.concurrent.CountDownLatch; @@ -16,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +@TestInstance(TestInstance.Lifecycle.PER_CLASS) public class DistributedLockServiceTest extends DatabaseTest { private final ExecutorService executor = Executors.newFixedThreadPool(2); From 0d0f374114c77efa26e4bcd9cc595f5619114234 Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Tue, 13 Jan 2026 16:05:17 -0500 Subject: [PATCH 7/8] Try changing fannypack url --- settings.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings.xml b/settings.xml index b5281bb65..9b5fb7aa5 100644 --- a/settings.xml +++ b/settings.xml @@ -52,7 +52,7 @@ github-fannypack FannyPack github repository - https://maven.pkg.github.com/Kowalski-IO/fannypack + https://maven.pkg.github.com/BrandonKowalski/fannypack true true From d4b85b0ad15e9cb48221cf4db1847316cc42fd6e Mon Sep 17 00:00:00 2001 From: nickpalladino Date: Thu, 15 Jan 2026 11:22:45 -0500 Subject: [PATCH 8/8] Fix test --- .../brapi/v2/BrAPITestUtils.java | 2 +- .../SubEntityDatasetLockIntegrationTest.java | 39 ++++++++++--------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/test/java/org/breedinginsight/brapi/v2/BrAPITestUtils.java b/src/test/java/org/breedinginsight/brapi/v2/BrAPITestUtils.java index 936bd401c..5f41de572 100644 --- a/src/test/java/org/breedinginsight/brapi/v2/BrAPITestUtils.java +++ b/src/test/java/org/breedinginsight/brapi/v2/BrAPITestUtils.java @@ -308,7 +308,7 @@ public Map makeExpImportRow(String environment, String expTitle) row.put(ExperimentObservation.Columns.GERMPLASM_GID, "1"); row.put(ExperimentObservation.Columns.TEST_CHECK, "T"); row.put(ExperimentObservation.Columns.EXP_TITLE, expTitle); - row.put(ExperimentObservation.Columns.EXP_UNIT, "Plot"); + row.put(ExperimentObservation.Columns.EXP_UNIT, "plot"); row.put(ExperimentObservation.Columns.EXP_TYPE, "Phenotyping"); row.put(ExperimentObservation.Columns.ENV, environment); row.put(ExperimentObservation.Columns.ENV_LOCATION, "Location A"); diff --git a/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java b/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java index c3fcbd1c9..d958d0b44 100644 --- a/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java +++ b/src/test/java/org/breedinginsight/brapi/v2/SubEntityDatasetLockIntegrationTest.java @@ -1,10 +1,6 @@ package org.breedinginsight.brapi.v2; -import com.google.gson.Gson; -import com.google.gson.GsonBuilder; -import com.google.gson.JsonDeserializer; -import com.google.gson.JsonObject; -import com.google.gson.JsonParser; +import com.google.gson.*; import io.micronaut.context.annotation.Property; import io.micronaut.http.HttpRequest; import io.micronaut.http.HttpResponse; @@ -26,8 +22,7 @@ import java.util.concurrent.*; import java.util.stream.Collectors; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.*; @MicronautTest @TestInstance(TestInstance.Lifecycle.PER_CLASS) @@ -67,13 +62,17 @@ void concurrentDatasetCreateReturnsSingleSuccessAndConflict() throws Exception { Callable call = () -> { start.await(1, TimeUnit.SECONDS); - Flowable> response = client.exchange( - HttpRequest.POST(String.format("/programs/%s/experiments/%s/dataset", program.getId(), experimentId), request.toString()) - .contentType(MediaType.APPLICATION_JSON) - .bearerAuth("test-registered-user"), - String.class - ); - return response.blockingFirst().getStatus(); + try { + Flowable> response = client.exchange( + HttpRequest.POST(String.format("/programs/%s/experiments/%s/dataset", program.getId(), experimentId), request.toString()) + .contentType(MediaType.APPLICATION_JSON) + .bearerAuth("test-registered-user"), + String.class + ); + return response.blockingFirst().getStatus(); + } catch (io.micronaut.http.client.exceptions.HttpClientResponseException e) { + return e.getStatus(); + } }; Future first = executor.submit(call); @@ -96,12 +95,14 @@ void concurrentDatasetCreateReturnsSingleSuccessAndConflict() throws Exception { ); HttpResponse datasetsResponse = datasetsCall.blockingFirst(); assertEquals(HttpStatus.OK, datasetsResponse.getStatus()); - var datasetsJson = JsonParser.parseString(Objects.requireNonNull(datasetsResponse.body())).getAsJsonObject() - .getAsJsonObject("result") - .getAsJsonArray("data"); + JsonObject parsed = JsonParser.parseString(Objects.requireNonNull(datasetsResponse.body())).getAsJsonObject(); + JsonArray resultArray = parsed.has("result") && parsed.get("result").isJsonArray() + ? parsed.getAsJsonArray("result") + : null; long matching = 0; - for (int i = 0; i < datasetsJson.size(); i++) { - String name = datasetsJson.get(i).getAsJsonObject().get("name").getAsString(); + assertNotEquals(null, resultArray); + for (int i = 0; i < resultArray.size(); i++) { + String name = resultArray.get(i).getAsJsonObject().get("name").getAsString(); if (name.equalsIgnoreCase(datasetName)) { matching++; }