Skip to content

[BI-2134] Append Experimental Dataset Workflow#359

Merged
dmeidlin merged 211 commits intodevelopfrom
feature/BI-2134
Jul 11, 2024
Merged

[BI-2134] Append Experimental Dataset Workflow#359
dmeidlin merged 211 commits intodevelopfrom
feature/BI-2134

Conversation

@dmeidlin
Copy link
Contributor

@dmeidlin dmeidlin commented May 22, 2024

Description

Story: BI-2134

  • Workflow selection code (BI-2122) will be shown as changes in here until it is merged into develop

  • Updated experiment import integration tests for appending or overwriting observations

  • Features and fixes merged into develop since the refactor started are not included

  • BI-2128 has been addressed in the refactor---blank values will not overwrite existing observation values

  • When exceptions are caught in the workflow, any compensating steps to restore the initial state are executed before re-throwing the exception at the ExperimentFileImportService level.

    Workflow Overview

  • Main entrypoint is AppendOverwritePhenotypesWorkflow

    • context is passed in as argument, containing import context and workflow context for storing hashmaps for caching workflow data during processing
  • Middleware class is used to break down the workflow into composable steps that are joined as a linked list

    • each link has a method to perform an action
    • each link has a method to possible revert an action, restoring previous state
    • the workflow steps are organized into validation, initialize, process, and commit modules
  • Factories are used to encapsulate the entity, actions, and processed data

    • Pending entities classes define the interaction with the workflow cache and the brapi service
    • Actions define calls to the brapi service and capture state information
  • Common methods are saved into various service and injected as dependencies to the factories

  • FieldValidator is a @FunctionalInterface used to validate observation data against trait properties

Dependencies

Testing

  • verify all experiment file import integration tests pass
  • create an experiment, then download experiment, then add observation variables and data and import

some variations to check

  • missing OUIds
  • duplicate OUIds
  • add observation variables with no observation data
  • add data to empty dataset
  • overwrite existing data with new values
  • confirm overwriting existing value with a blank is silently ignored
  • check that validations against ordinal, nominal, numerical, and min/max range are caught

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>

nickpalladino and others added 30 commits April 26, 2024 09:49
@dmeidlin dmeidlin changed the title [BI-2134] Append Experimental Dataset Workflow [BI-2134] Append Experimental Dataset Workflows Jul 9, 2024
@dmeidlin dmeidlin changed the title [BI-2134] Append Experimental Dataset Workflows [BI-2134] Append Experimental Dataset Workflow Jul 9, 2024
@dmeidlin dmeidlin requested a review from davedrp July 9, 2024 20:02
private WorkflowUpdate<BrAPIObservation> brAPIObservationUpdate;
private Optional<WorkflowCreation.BrAPICreationState> createdBrAPIObservations;
private Optional<WorkflowUpdate.BrAPIUpdateState> priorBrAPIObservations;
private Optional<WorkflowUpdate.BrAPIUpdateState> updatedObservations;
Copy link
Contributor

Choose a reason for hiding this comment

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

this field gets initialized, but is never used.


public abstract class Middleware<T> {

Middleware next;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use type arguments (see suggestions below)

Suggested change
Middleware next;
Middleware<T> next;

public abstract class Middleware<T> {

Middleware next;
Middleware prior;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Middleware prior;
Middleware<T> prior;

Copy link
Contributor

Choose a reason for hiding this comment

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

Other change in this file may also need to be modified for this to work

return this.next == null ? this : this.next.getLastLink();
}

private Middleware getFirstLink() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is not used.

* @return A PendingImportObject containing the dataset with the existing list and reference ID
* @throws IllegalStateException if external references weren't found for the list
*/
public PendingImportObject<BrAPIListDetails> constructPIOFromDataset(BrAPIListDetails dataset, Program program) {
Copy link
Contributor

Choose a reason for hiding this comment

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

the parameter program is not used

* @return An Optional containing a ValidationError if validation fails, or Optional.empty() if validation passes.
*/
@Override
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwritten @nonnull parameters should be @nonnull

Suggested change
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
public Optional<ValidationError> validateField(@NotNull String fieldName, @NotNull String value, Trait variable) {

* @return Optional&lt;ValidationError&gt; Optional containing the first validation error encountered, if any
*/
@Override
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwritten @nonnull parameters should be @nonnull

Suggested change
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
public Optional<ValidationError> validateField(@nonnull String fieldName, @nonnull String value, Trait variable) {

* @return an Optional containing a ValidationError if validation fails, otherwise an empty Optional
*/
@Override
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwritten @nonnull parameters should be @nonnull

Suggested change
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
public Optional<ValidationError> validateField(@nonnull String fieldName, @nonnull String value, Trait variable) {

* @return Optional containing a ValidationError if validation fails, or Optional.empty() if successful.
*/
@Override
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwritten @nonnull parameters should be @nonnull

Suggested change
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
public Optional<ValidationError> validateField(@nonnull String fieldName, @nonnull String value, Trait variable) {

* @return an Optional containing a ValidationError if validation fails, empty otherwise
*/
@Override
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Overwritten @nonnull parameters should be @nonnull

Suggested change
public Optional<ValidationError> validateField(String fieldName, String value, Trait variable) {
public Optional<ValidationError> validateField(@nonnull String fieldName, @nonnull String value, Trait variable) {

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.

4 participants