[BI-2355] - Non-informative error message: regression#423
Merged
HMS17 merged 5 commits intorelease/1.0from Nov 1, 2024
Merged
Conversation
6 tasks
mlm483
approved these changes
Nov 1, 2024
Contributor
mlm483
left a comment
There was a problem hiding this comment.
Tested, working.
I'm approving, but I'd recommend simplifying using the code in my comments. I tested the functionality before and after applying my change, it works either way.
Comment on lines
174
to
189
| // replace certain special characters with "" in column names to deal with json flattening issue in tablesaw | ||
| // this includes ".", "[", "[" | ||
| List<String> columnNames = df.columnNames(); | ||
| List<String> namesToReplace = new ArrayList<>(); | ||
| for (String name : columnNames) { | ||
| if (name.contains(".")) { | ||
| if (name.contains(".") || name.contains("[") || name.contains("]")) { | ||
| namesToReplace.add(name); | ||
| } | ||
| } | ||
|
|
||
| List<Column<?>> columns = df.columns(namesToReplace.stream().toArray(String[]::new)); | ||
| for (int i=0; i<columns.size(); i++) { | ||
| Column<?> column = columns.get(i); | ||
| column.setName(namesToReplace.get(i).replace(".","")); | ||
| //if more characters, could use replaceall and regex, but this works presently | ||
| column.setName(namesToReplace.get(i).replace(".","").replace("[","").replace("]","")); | ||
| } |
Contributor
There was a problem hiding this comment.
I might be missing something, but it seems to me that lines 174-189 could be replaced with the following with the same behavior and efficiency.
// Replace certain special characters with "" in column names to deal with json flattening issue in tablesaw,
// this includes ".", "[", "]".
df.columns().forEach(
(c) -> c.setName(c.name().replace(".","").replace("[","").replace("]",""))
);You could check with Nick about why the code was initially structured that way, but I think this way is more clear.
Contributor
Author
There was a problem hiding this comment.
Good point, it looks a lot more elegant that way!
src/main/java/org/breedinginsight/brapps/importer/services/FileImportService.java
Outdated
Show resolved
Hide resolved
| public ValidationError getPeriodObsVarNameMsg() { | ||
| return new ValidationError("observationVariableName", "Period in name is invalid", HttpStatus.BAD_REQUEST); | ||
| public ValidationError getInvalidCharObsVarNameMsg() { | ||
| return new ValidationError("observationVariableName", "Periods and brackets in name is invalid", HttpStatus.BAD_REQUEST); |
Contributor
There was a problem hiding this comment.
I'm often wrong on matters of grammar, but I think "...in name are invalid" sounds better.
Co-authored-by: mlm483 <128052931+mlm483@users.noreply.github.com>
dmeidlin
approved these changes
Nov 1, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
(New branch due to soiled changelist in previous attempt)
Story: BI-2355 - Non-informative error message: regression
This error was due to tablesaw processing brackets weirdly, resulting in the column name stored in data not matching the column name stored in dynamic column names. This was fixed in two parts:
Removing brackets from ontology term names on upload
Preventing ontology terms with brackets in the name from being created by extending the functionality created in BI-2328
Note that this means for the error message re ontology terms not found, periods and brackets will be removed from the invalid ontology term names
Dependencies
bi-web: bug/BI-2355.2
Testing
Test that on upload of an experiment file with invalid ontology term columns headings that contain brackets and/or periods that the unknown error message no longer shows and instead the invalid ontology term message shows
Test that upload of experiment file with valid ontology terms proceeds as expected
Test that the appropriate error is thrown when an ontology term contains ".", "[" and/or "]" and that that error is not thrown when the ontology term does not contain those characters for:
Checklist: