-
Notifications
You must be signed in to change notification settings - Fork 1
Implement Dynamic Observation Levels #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: brapi-server-v2
Are you sure you want to change the base?
Conversation
| private List<ObservationUnitLevelNameEntity> findObservationUnitLevelNames(List<String> programDbIds, | ||
| List<String> levelNameDbIds, | ||
| Boolean all) | ||
| throws BrAPIServerException { | ||
| List<ObservationUnitLevelNameEntity> foundOULevelNames = new ArrayList<>(); | ||
|
|
||
| if (all != null && all) { | ||
| return observationUnitLevelNameRepository.findAllObservationUnitLevelNames(); | ||
| } | ||
|
|
||
| if (levelNameDbIds != null && !levelNameDbIds.isEmpty()) { | ||
|
|
||
| List<ObservationUnitLevelNameEntity> result = observationUnitLevelNameRepository.findAllById(levelNameDbIds.stream().map(UUID::fromString).toList()); | ||
|
|
||
| if (result.size() != levelNameDbIds.size()) { | ||
|
|
||
| List<String> foundDbIds = result.stream() | ||
| .map(ouln -> ouln.getId().toString()) | ||
| .toList(); | ||
| List<String> levelNameDbIdsNotFound = levelNameDbIds.stream() | ||
| .filter(lnId -> !foundDbIds.contains(lnId)) | ||
| .toList(); | ||
|
|
||
| throw new BrAPIServerException(HttpStatus.BAD_REQUEST, | ||
| String.format("Level name DB Ids supplied [%s] were not found in the database. " + | ||
| "Utilize the /observationlevelnames GET method to find the level name DB Ids you would like to search, " + | ||
| "or forgo supplying level name dbIds and try looking up on a programDbId.", levelNameDbIdsNotFound)); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| if (programDbIds != null && !programDbIds.isEmpty()) { | ||
| // First look up all level names related to submitted programs if available | ||
| foundOULevelNames = observationUnitLevelNameRepository.findObservationUnitLevelNamesByProgram(programDbIds); | ||
| } | ||
|
|
||
| if (foundOULevelNames.isEmpty()) { | ||
| // None were found. Now try to see if there are globally available level names. | ||
| foundOULevelNames = observationUnitLevelNameRepository.findDefaultObservationUnitLevelNames(); | ||
| } | ||
|
|
||
| if (foundOULevelNames.isEmpty()) { | ||
| throw new BrAPIServerException(HttpStatus.BAD_REQUEST, | ||
| "No observation level names were found for the programDbId provided, nor are any global level" + | ||
| " names available. Please add via endpoint to attach level names to a program or define " + | ||
| "without a program to make them globally accessible."); | ||
| } | ||
|
|
||
| return foundOULevelNames; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very critical logic for the BI use case, and is now referenced by the ObservationUnitService when creating OUs that come in with level names.
Considering in your use case every OU submitted is related to a trial/study, the programDbId is always provided to this method. If no levelNameDbIds are provided, then the level name lookup will lookup all level names related to the programDbId provided.
I have spoken with @nickpalladino and @mlm483 before and the recommendation is that you guys grab the observation unit level name db ids using the level names api before populating your OU post, and when u want a certain level name for your use case, provide the levelNameDbIds to prevent looking up by program. This will allow y'all to bypass the program/global logic effortlessly.
Can meet up for a talk and example of this.
| @ApiParam(value = "HTTP HEADER - Token used for Authorization <strong> Bearer {token_string} </strong>") @RequestHeader(value = "Authorization", required = false) String authorization) | ||
| throws BrAPIServerException; | ||
|
|
||
| @ApiOperation(value = "Get the Observation Level Names", nickname = "observationlevelnamesGet", notes = "Call to save a list of observation level names", response = ObservationLevelListResponse.class, authorizations = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a missed copy/paste, should have value, nickname, and notes updated for POST instead of GET
| @ApiParam(value = "HTTP HEADER - Token used for Authorization <strong> Bearer {token_string} </strong>") @RequestHeader(value = "Authorization", required = false) String authorization) | ||
| throws BrAPIServerException; | ||
|
|
||
| @ApiOperation(value = "Get the Observation Level Names", nickname = "observationlevelnamesGet", notes = "Call to save a list of observation level names", response = ObservationLevelListResponse.class, authorizations = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a missed copy/paste, should have value, nickname, and notes updated for DELETE instead of GET
| log.debug("Request: " + request.getRequestURI()); | ||
| validateSecurityContext(request, "ROLE_ANONYMOUS", "ROLE_USER"); | ||
| validateAcceptHeader(request); | ||
| var data = observationUnitLevelNameService.save(body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I make a POST /brapi/v2/observationlevelnames with the body:
[
{
"levelName": "Berry",
"levelOrder": 1,
"programDbId": "c352c1d6-c28d-48c5-85e9-24a6a5109545"
}
]
for a program where I already created the Berry levelName I get a 500 Internal Server Error:
"Server Error:
org.springframework.orm.jpa.vendor.HibernateJpaDialect.convertHibernateAccessException(HibernateJpaDialect.java:290)
org.springframework.orm.jpa.vendor.HibernateJpaDialect.translateExceptionIfPossible(HibernateJpaDialect.java:241)
org.springframework.orm.jpa.JpaTransactionManager.doCommit(JpaTransactionManager.java:566)
..."
For this case I think the duplicate should be handled and not allowed server side and instead return a 409 status.
| private String programDbId = null; | ||
|
|
||
| // NOTE: This property is NOT used for lookups, only responses. | ||
| @JsonProperty("programName") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET /observationlevels now has additional programDbId and programName properties in the response objects (always null). That's a change from the spec. I guess in this case it's just an addition so assuming the client ignores new properties it should be backwards compatible. Thinking mostly for Field Book which uses the BrAPI java client. We do have some integration tests in the BrAPI java client repo. I'll add a card to create a github action so we can run the tests in there against brapi server PRs.
Also it would be nice to keep the BrAPI java client in sync with brapi server changes. We may have to have a version that is ahead of the brapi spec to support some of the stuff that's been added to the brapi server.
The following features have been added to support this implementation:
observation_unit_level_nameadded to allow for custom level names columns to the following dependent tables:observation_unit_position(Referenced inObservationUnitPositionEntity). This is the top-level level name for observation units.observation_unit_level(Referenced inObservationUnitLevelRelationshipEntity). This is a kind of branching level name inside the Position entity. Each position can be related to any number of these levels.study_observation_level(Referenced inObservationLevelEntity). The entity name here is a misnomer. The only real thing this table does is relate studies directly to level names. It's not really used at all in any search functions.observation_unit_level_namehas three columns (other thanid):level_name: The actual custom name of the level.level_order: The numerical order of the level. Previously, this was determined by the static enum's ordinance. This is likely not required for custom level names that fall outside of that umbrellaprogram_id: The program which this observation_unit_level_name relates to. This was a requirement for the BI use case.A migration was created for this table not only to add it, but to also capture the existing ENUM values as what we will refer to as global level names. That is, level names not correlated to any program.
ObservationUnitLevelNamesApiControllerand API has been added to provide CRUD operations. These CRUD operations should serve as the principal way to add level names into the system. Level names cannot be created through observation units operations. Moreover, error messages have been added to instruct the client how to use this new API in the event non level name creation operations try to reference level names that do not exist.Updates to
ObservationUnitServiceand related level name data models to support submitting programDbIds and the new levelNameDbIds when creating new ObservationUnits. This should allow for BI to grab the existing level names they require for their OUPost request and populate them for each observation unit required.Updates to
ObservationUnitServiceto maintain optimized performance when using and submitting observation units for creation that contain observation levels. Only one query to grab existing required observation unit levels which is then filtered upon while creating OUs.Updates to observations and observationunits table GET functions. Before, these tables returned static values of the level name enum if observations where present related to them. @BrapiCoordinatorSelby and I talked a while ago about how this seemed largely unnecessary and unused, so these columns and related header rows have been removed. There is currently no support in place for dynamic obs levels for these endpoints, and will not be until a use case is deemed appropriate.
Updates to the /observationlevels GET endpoint on the ObservationUnitsApiController. This method has been fundamentally changed to be optimized with the new dynamic observation levels, and allows clients to find what level names observation units are related to. A
programDbId,studyDbId, ortrialDbIdcan be submitted to find all observation levels related to OUs related to these params.Finally, one big update that explains the breadth of the scope in this MR:
Essentially, I had the choice of doubling down on making the new
ObservationUnitLevelNameEntityaBrAPIPrimaryEntitychild, which required addingauth_user_id,additional_info, and a whole external references table, OR using theBrAPIBaseEntityclass to just make a table without all of that stuff and making a new repo impl that just extends from the SimpleJpaRepo, which offers all the normal crud operations one would expect with Jpa. I chose the latter mostly because I saw this as a good opportunity to create this separation which seemed necessary. I think keeping CRUD ops locked behind an entity implementation that not every use case needs to support is a bit of a whacky design decision. I have gone down both paths and implemented both, but this is the one I stand behind because it makes more sense to me from a design perspective.A perfect design I think would be to have the BaseEntityRepo be the baseline for all entities, but it was extremely difficult enforcing this design decision with the existing dependencies, and Spring/Jpa had limited support for this option.