Skip to content

[BI-2628] Replace Static ObsUnitID column to Dynamic Sub-entity ObsUnitID column in APpend Workflow#469

Merged
dmeidlin merged 40 commits into
developfrom
feature/BI-2628
Jun 27, 2025
Merged

[BI-2628] Replace Static ObsUnitID column to Dynamic Sub-entity ObsUnitID column in APpend Workflow#469
dmeidlin merged 40 commits into
developfrom
feature/BI-2628

Conversation

@dmeidlin
Copy link
Copy Markdown
Contributor

@dmeidlin dmeidlin commented Jun 9, 2025

Description

Story:
This code changes in this PR cover the scope of two cards:

This update makes the append/overwrite experiment import workflow more robust and maintainable by modularizing dynamic column validation, especially for observation unit IDs, and by improving error handling and reporting.

This pull request introduces a new, extensible validation framework for dynamic columns in the experiment import workflow, with a focus on Observation Unit (OU) ID validation. The main changes include:

New Validators for Observation Unit IDs

  • Introduced the DynamicColumnValidator interface to support pluggable and ordered validation of dynamic columns.
  • Added modular validators for OU ID columns: duplicate check, blank check, column name check, and format check.
  • Each validator is implemented as a separate class for clarity and reusability.
  • Integrated the new validators into the append/overwrite experiment import workflow.
  • Updated constructors and dependency injection to support the new validator architecture.

Enhanced Experiment Utilities

  • Refactored utility methods to support dynamic column validation and improved error handling.
  • Added methods to check for unique IDs and handle missing column names with clear exceptions.

Files Added:

  • Dynamic column validator interface
  • multiple OU ID validator classes
  • a db migration script for the import map.

Files Deleted:

  • The legacy ExperimentProcessor.java class was deleted since it had numerous references to the static obs unit id column that would have been necessary to delete in order to build the project.

Files Modified:

  • Experiment utility classes, append workflow context, and append middleware to integrate the new validation system.
  • The methods in TrialService.java for exporting experimental datasets was modifed as required by BI-2631.
  • In the create workflow, PopulateExistingImportObjectsStep.java and ValidatePendingImportObjectsStep.java have TODOs inserted where sections of the code had to be commented out. Completing these TODOs is captured in the scope of BI-2641.

Dependencies

none

Testing

Create an Experiment. Download a dataset for the experiment. Add or Modify observation data for the data set and import using the append workflow. Verify the import succeeded.

Checklist:

  • I have performed a self-review of my own code
  • I have tested my code and ensured it meets the acceptance criteria of the story
  • I have tested that my code works with both the brapi-java-server and BreedBase
  • I have create/modified unit tests to cover this change
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to documentation
  • I have run TAF: <please include a link to TAF run>

dmeidlin and others added 30 commits May 15, 2025 10:35
@dmeidlin dmeidlin requested review from Copilot and removed request for Copilot June 9, 2025 19:12
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the static ObsUnitID column with dynamic sub-entity ObsUnitID handling in both import and export workflows, introduces validators for dynamic ObsUnitID columns, and refactors related services to use the new dynamic column suffix. It also bumps the Postgres image and cleans up outdated code paths.

  • Added dynamic ObsUnitID validators and updated middleware to detect a single or nested ObsUnitID column.
  • Refactored import/export services, constants, and utilities to use OBSERVATION_UNIT_ID_SUFFIX and removed static ObsUnitID references.
  • Updated Docker Compose to use Postgres 17.5 and removed/commented out unused methods.

Reviewed Changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
ExpImportProcessConstants.java Introduced OBSERVATION_UNIT_ID_SUFFIX, SUB_UNIT_NUMBER, and new error codes for dynamic ObsUnitID
ExperimentTrialService.java Removed static ObsUnitID filter when collecting unique trial names
TrialService.java Deleted original initializeTrialByNameNoScope logic but left an empty stub
ValidatePendingImportObjectsStep.java Commented out per-row ObsUnitID checks pending column-level validation
PopulateNewPendingImportObjectsStep.java Adjusted trial-population logic around missing ObsUnitID
PopulateExistingPendingImportObjectsStep.java Commented out entire observation-unit initializer method
CreateNewExperimentWorkflow.java Changed imports to wildcard and added dynamic ObsUnitID detection
ImportTableProcess.java Separated phenotype vs ObsUnitID columns, updated column‐name validation, and replaced static ObsUnitID logic
BrAPITrialService.java Updated export to use dynamic ObsUnitID suffix and altered column-replacement logic
docker-compose.yml Upgraded Postgres image from 11.4 to 17.5
ExperimentUtilities.java Refactored ID-collation to read from dynamic column rather than model field
Multiple ObservationUnitID*Validator.java New validators for column name, blank values, format, and duplicates
Comments suppressed due to low confidence (9)

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/services/ExperimentTrialService.java:179

  • Removing the filter on blank ObsUnitID causes trials to be initialized even when an ObsUnitID is present; reintroduce logic (or adapt to dynamic column detection) so only rows without an ObsUnitID column are used to collect new trial names.
.map(ExperimentObservation::getExpTitle)

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/service/TrialService.java:309

  • The stubbed initializeTrialsForExistingObservationUnits and commented-out initializeTrialByNameNoScope leave dead code; remove unused methods or implement their logic to avoid confusion.
private Map<String, PendingImportObject<BrAPITrial>> initializeTrialByNameNoScope(Program program, Map<String, PendingImportObject<BrAPIObservationUnit>> observationUnitByNameNoScope,

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/model/ExpImportProcessConstants.java:42

  • [nitpick] The enum constants OZEX, VVCN, and BITB are not self-descriptive; consider renaming them to more meaningful identifiers like MISSING_OBS_UNIT_COLUMN, DUPLICATE_OBS_UNIT_ID, and INVALID_OR_MISSING_OBS_UNIT_ID.
OZEX("Missing ObsUnitID column. Import cannot proceed"),

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/create/workflow/steps/ValidatePendingImportObjectsStep.java:403

  • [nitpick] The error message no longer identifies which ObsUnitID has the duplicate observation; consider including the ObsUnitID or row number for clearer debugging.
String.format("Value already exists for Phenotype: %s", phenoCol.name()),

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/create/workflow/steps/PopulateNewPendingImportObjectsStep.java:312

  • The original check for blank ObsUnitID was removed, causing existing trials to always throw UnprocessableEntityException; restore the blank-check or adjust logic to only reject new units when ObsUnitID is missing.
if  (trialPio!=null &&  ImportObjectState.EXISTING==trialPio.getState() &&

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/appendoverwrite/middleware/process/ImportTableProcess.java:318

  • This always appends a new ObsUnitID column rather than replacing the existing one; use the original column’s index to replace it in place to preserve header order.
columns.add(ObsUnitIDCol);

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/appendoverwrite/middleware/process/ImportTableProcess.java:112

  • [nitpick] The comment calls out 'phenotypic columns' but the code still uses getDynamicColumnNames(); update the comment or variable names to clearly differentiate phenotype vs. ObsUnitID columns.
// Get all the phenotypic columns of the import

src/main/java/org/breedinginsight/brapi/v2/services/BrAPITrialService.java:319

  • Local variable ObsUnitIDCol starts with a capital letter; use lowerCamelCase (obsUnitIDCol) to follow Java naming conventions.
Column ObsUnitIDCol = new Column(dynamicLabel, Column.ColumnDataType.STRING);

src/main/java/org/breedinginsight/brapps/importer/services/processors/experiment/appendoverwrite/validator/dynamicColumns/observationUnitID/ObservationUnitIDFormatValidator.java:18

  • New dynamic-column validators lack dedicated unit tests; add tests for ObservationUnitIDColumnNameValidator, FormatValidator, BlankValidator, and DuplicateIDValidator to ensure each behavior is covered.
package org.breedinginsight.brapps.importer.services.processors.experiment.appendoverwrite.validator.dynamicColumns.observationUnitID;

@Breeding-Insight Breeding-Insight deleted a comment from Copilot AI Jun 9, 2025
@Breeding-Insight Breeding-Insight deleted a comment from Copilot AI Jun 9, 2025
@Breeding-Insight Breeding-Insight deleted a comment from Copilot AI Jun 9, 2025
@Breeding-Insight Breeding-Insight deleted a comment from Copilot AI Jun 9, 2025
@Breeding-Insight Breeding-Insight deleted a comment from Copilot AI Jun 9, 2025
@Breeding-Insight Breeding-Insight deleted a comment from Copilot AI Jun 9, 2025
@dmeidlin dmeidlin requested review from a team, HMS17 and davedrp and removed request for a team June 11, 2025 13:47
@dmeidlin dmeidlin assigned davedrp and unassigned nickpalladino and HMS17 Jun 11, 2025
@dmeidlin dmeidlin requested a review from davedrp June 24, 2025 15:18
@dmeidlin dmeidlin merged commit 2700fde into develop Jun 27, 2025
1 check passed
@dmeidlin dmeidlin deleted the feature/BI-2628 branch June 27, 2025 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants