[BI-1465] 6.1 Observation Dataset Export File by dmeidlin · Pull Request #259 · Breeding-Insight/bi-api

dmeidlin · 2023-06-06T17:37:40Z

Description

Story: BI-1465

ExperimentController#datasetExport was created to add the GET endpoint /${micronaut.bi.api.version}/programs/{programId}/experiments/{experimentId}/export?{params}
BrAPITrialService#exportObservations and various helper functions in the same service were created to map existing observation data (if any) for the requested experiment and environments to table rows matching the structure of the experiment import template.
ExperimentControllerIntegrationTest class was created to test the various combinations of query param values to the export endpoint.

Dependencies

bi-web feature/BI-1465

Testing

go to the Experiments and observations table
click Download for any experiment
select different file formats and the timestamp checkbox
verify fidelity of downloaded file to request inputs and import data for that experiment

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>

timparsons · 2023-06-06T19:01:32Z

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

+    @Property(name = "brapi.server.reference-source")
+    private String referenceSource;


For testability, this should be included as a parameter to the constructor. See BrAPIObservationUnitDAO as an example

timparsons · 2023-06-06T19:03:45Z

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

+        List<BrAPIObservationVariable> obsVars = new ArrayList<>();
+        Map<String, Map<String, Object>> rowByOUId = new HashMap<>();
+        Map<String, BrAPIStudy> studyByDbId = new HashMap<>();
+        List<String> requestedEnvNames = StringUtils.isNotBlank(params.getEnvironments()) ?


It may be better for this code to assume that environments is a list of environment UUIDs instead of names. That can help the code further down in this method to be more succinct

agreed, and made the change

timparsons · 2023-06-06T19:06:46Z

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

+        } catch (ApiException err) {
+            log.error("Error fetching observation units for a study by its DbId" +
+                    Utilities.generateApiExceptionLogMessage(err), err);
+            err.printStackTrace();


This isn't needed as you've already passed the exception object to the log.error method (and that method will print the stack trace)

timparsons · 2023-06-06T19:13:15Z

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

+            BrAPIObservationUnit ou = ous.stream()
+                    .filter(unit -> obs.getObservationUnitDbId().equals(unit.getObservationUnitDbId()))
+                    .findAny()
+                    .orElseThrow(RuntimeException::new);


I think it would be worth creating a map outside of the for loop of ouByOUId to reduce the runtime complexity

timparsons · 2023-06-06T19:18:17Z

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

+        BrAPIObservationVariable var = obsVars.stream()
+                .filter(obsVar -> obs.getObservationVariableName().equals(obsVar.getObservationVariableName()))
+                .collect(Collectors.toList()).get(0);


It would also be worth changing this to make a map of variableByDbId, and have that map be created and held in addBrAPIObsToRecords. This will also reduce the runtime complexity. This method could then be updated to accept a single BrAPIObservationVariable instead of a list of BrAPIObservationVariables

timparsons · 2023-06-06T19:20:29Z

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

+
+    public BrAPITrial getExperiment(Program program, UUID experimentId) throws ApiException {
+        List<BrAPITrial> experiments = trialDAO.getTrialsByExperimentIds(List.of(experimentId), program);
+        return experiments.get(0);


May be worth having a check here to ensure the list returned has at least one element. If it doesn't, throw an error. This will prevent ArrayIndexOutOfBounds errors from occurring

timparsons · 2023-06-06T19:26:29Z

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

+            Column obsVarColumn = new Column();
+            obsVarColumn.setDataType(Column.ColumnDataType.STRING);
+            if (var.getScale().getDataType().equals(BrAPITraitDataType.ORDINAL)) {
+                obsVarColumn.setDataType(Column.ColumnDataType.INTEGER);


I think this has to be a string for ordinal. At least on the DeltaBreed UI, we don't enforce that the value of the ordinal be numeric.

timparsons · 2023-06-06T19:30:59Z

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

+        if (fileType.equals(FileType.CSV)){
+            downloadFile = CSVWriter.writeToDownload(columns, exportRows, fileType);
+        } else {
+            downloadFile = ExcelWriter.writeToDownload("Dataset Export", columns, exportRows, fileType);


I think the sheetName should be "Experiment Data" to match the experiment import template

made the change

timparsons · 2023-06-06T19:31:16Z

src/main/java/org/breedinginsight/utilities/FileUtil.java

    // For backward compatibility
    private static final String OLD_GERMPLASM_EXCEL_DATA_SHEET_NAME = "Germplasm Import";
    private static final String OLD_EXPERIMENT_EXCEL_DATA_SHEET_NAME = "Experiment Data";
+    private static final String DATASET_EXPORT_EXCEL_DATA_SHEET_NAME = "Dataset Export";


This isn't needed if the sheet name on export is "Experiment Data"

timparsons · 2023-06-06T19:31:31Z

src/main/java/org/breedinginsight/utilities/FileUtil.java

        //For backward compatibility allow old sheet names
        if( sheet == null){ sheet = workbook.getSheet(OLD_GERMPLASM_EXCEL_DATA_SHEET_NAME); }
        if( sheet == null){ sheet = workbook.getSheet(OLD_EXPERIMENT_EXCEL_DATA_SHEET_NAME); }
+        if( sheet == null){ sheet = workbook.getSheet(DATASET_EXPORT_EXCEL_DATA_SHEET_NAME); }


This isn't needed if the sheet name on export is "Experiment Data"

mlm483

This is a big feature, overall looks good! I found one bug, and I requested a few changes, mostly cleanup and documentation.

ExperimentControllerIntegrationTest looks good. I don't see any of the test files in src/test/resources/files/experiment_controller/ being used, you may want to remove them.
There seems to be an issue casting timestamps to strings when generating Excel files. BrAPI Java TestServer was backing service. To reproduce:
1. Import Germplasm: RILS and Parents.xls
2. Import Ontology: bi_lettuce_ontology_v1.xlsx
3. Import Experiment: SCY RIL exp2 TRUNC.xls
4. Download KRSP22-2 dataset With Timestamps, select .xlsx file type.
5. Error.
- in browser: An error occurred while generating the download file. Contact the development team at bidevteam@cornell.edu. (Good actionable message! 👍)
- bi-api stacktrace: o.b.brapi.v2.ExperimentController - class java.time.OffsetDateTime cannot be cast to class java.lang.String (java.time.OffsetDateTime and java.lang.String are in module java.base of loader 'bootstrap') java.lang.ClassCastException: class java.time.OffsetDateTime cannot be cast to class java.lang.String (java.time.OffsetDateTime and java.lang.String are in module java.base of loader 'bootstrap') at org.breedinginsight.services.writers.ExcelWriter.writeToWorkbook(ExcelWriter.java:58) at org.breedinginsight.services.writers.ExcelWriter.writeToDownload(ExcelWriter.java:77) at org.breedinginsight.brapi.v2.services.BrAPITrialService.exportObservations(BrAPITrialService.java:172) at org.breedinginsight.brapi.v2.ExperimentController.datasetExport(ExperimentController.java:101)

src/main/java/org/breedinginsight/utilities/FileUtil.java

src/main/java/org/breedinginsight/utilities/response/ResponseUtils.java

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

davedrp · 2023-06-06T21:16:48Z

Testing Results:
While testing, I found an inconsistency. The "import" of Experiments & Observations allows alphanumeric values for 'row' and 'column' ( for example; row='A'). But the download will error-out unless theses values are numeric.

dmeidlin · 2023-06-07T22:28:06Z

@davedrp Row and Column types were changed from double to string, and the download works now.

dmeidlin · 2023-06-08T13:13:03Z

@mlm483 Observation timestamps are being parsed to strings now, and the export works.

timparsons

Just a few small changes

timparsons · 2023-06-08T14:15:09Z

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

+        try {
+            for (BrAPIStudy study: expStudies) {
+                ous.addAll(ouDAO.getObservationUnitsForStudyDbId(study.getStudyDbId(), program));
+                ous.forEach(ou -> ouByOUDbId.put(ou.getObservationUnitDbId(), ou));


I think this will end up looping over all OUs for X number of studies. So during the first iteration, just the OUs for the first study will be added to the map. In the second iteration, all the OUs from the first study and the second study will be added to the map (although the OUs from the first study will just overwrite the existing records in the map), and so on until all studies have been iterated over.

Good catch. I changed the forEach to loop over the current study ous instead of all ous.

timparsons · 2023-06-08T14:15:34Z

src/main/java/org/breedinginsight/brapi/v2/ExperimentController.java

+            return response;
+        } catch (Exception e) {
+            log.info(e.getMessage(), e);
+            e.printStackTrace();


This can be removed as the log.info will print the stack trace

timparsons · 2023-06-08T14:21:26Z

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

+        if (var.getScale().getDataType().equals(BrAPITraitDataType.ORDINAL)) {
+            row.put(varName, Integer.parseInt(obs.getValue()));


I think this has to be set as a String type for the same reason specified on this comment

mlm483

Looks good. Approved, I would just recommend explaining the call to addBrAPIObsToRecords on line 161 of BrAPITrialService.java, see comment.

mlm483 · 2023-06-08T15:04:01Z

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

+        addBrAPIObsToRecords(
+                dataset,
+                experiment,
+                program,
+                ouByOUDbId,
+                studyByDbId,
+                rowByOUId,
+                params.isIncludeTimestamps(),
+                obsVars
+        );


Again, because there are so many parameters and it's not immediately obvious which is being mutated, I would add a comment above this method call.

np, added a comment that rowByOUId is being updated.

davedrp · 2023-06-08T15:30:23Z

Dev-Test Passed

dmeidlin and others added 22 commits June 6, 2023 11:56

make integration tests for ExperimentController

35ebe96

add download endpoint to experiments controller

d035bbf

add BrAPITrialService method

5c19985

check for datsetId in BrAPITrialService#getDataset

f8afcfc

add obsvar columns

19de7d9

cast column values

95f00b6

fix test parsing of download

db1f8e5

cache studies

03c35fb

remove debugging comments

c271876

remove program key from names

0d22f5f

remove key from names

b21a6ae

update

7f29b3a

[BI-1465] Adding debug statements to tests

66337c4

add createExportRow

fa6aadd

temp fix

2502ac7

handle export of empty datasets

8b30a11

add null checks

76abca4

remove keys from env and exp unit id

e44458b

clean up test

ff449c3

clean up TrialService and factor out duplicated code

43c09d4

clean up ExperimentController

817184e

fix merge conflicts

d0384fc

github-actions bot added the feature label Jun 6, 2023

dmeidlin requested review from a team, davedrp and mlm483 and removed request for a team June 6, 2023 17:37

dmeidlin assigned davedrp Jun 6, 2023

timparsons reviewed Jun 6, 2023

View reviewed changes

mlm483 requested changes Jun 6, 2023

View reviewed changes

dmeidlin added 2 commits June 7, 2023 15:03

improve code

3dde1bb

improve code and fix bugs

362e065

remove npe

17e3137

timparsons reviewed Jun 8, 2023

View reviewed changes

improve code

bddd2c5

dmeidlin requested review from mlm483 and timparsons June 8, 2023 14:52

timparsons approved these changes Jun 8, 2023

View reviewed changes

mlm483 approved these changes Jun 8, 2023

View reviewed changes

davedrp approved these changes Jun 8, 2023

View reviewed changes

add comment

bf3e994

dmeidlin merged commit b5fc289 into release/0.8 Jun 8, 2023

dmeidlin deleted the feature/BI-1465 branch June 8, 2023 15:51

		@Property(name = "brapi.server.reference-source")
		private String referenceSource;

		if (var.getScale().getDataType().equals(BrAPITraitDataType.ORDINAL)) {
		row.put(varName, Integer.parseInt(obs.getValue()));

Conversation

dmeidlin commented Jun 6, 2023 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Testing

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlm483 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davedrp commented Jun 6, 2023

Uh oh!

dmeidlin commented Jun 7, 2023

Uh oh!

dmeidlin commented Jun 8, 2023

Uh oh!

timparsons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlm483 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmeidlin commented Jun 6, 2023 •

edited by atlassian bot

Loading

davedrp commented Jun 8, 2023 •

edited

Loading