Skip to content

Conversation

@jloux-brapi
Copy link

@jloux-brapi jloux-brapi commented Aug 12, 2025

The following features have been added to support this implementation:

  • New table observation_unit_level_name added to allow for custom level names columns to the following dependent tables:
    • observation_unit_position (Referenced in ObservationUnitPositionEntity). This is the top-level level name for observation units.
    • observation_unit_level (Referenced in ObservationUnitLevelRelationshipEntity). 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 in ObservationLevelEntity). 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_name has three columns (other than id):
    • 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 umbrella
    • program_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.

  • ObservationUnitLevelNamesApiController and 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 ObservationUnitService and 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 ObservationUnitService to 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, or trialDbId can 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:

  • To break an existing mold of CRUD related operations being entirely beholden to the BrAPIRepository impl, I've added a second repo configuration, and moved config files to a relevant package.
    Essentially, I had the choice of doubling down on making the new ObservationUnitLevelNameEntity a BrAPIPrimaryEntity child, which required adding auth_user_id, additional_info, and a whole external references table, OR using the BrAPIBaseEntity class 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.

@jloux-brapi jloux-brapi marked this pull request as ready for review August 13, 2025 16:39
Comment on lines +255 to +305
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;
}
Copy link
Author

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.

@nickpalladino nickpalladino removed the request for review from mlm483 November 3, 2025 16:28
@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 = {
Copy link
Collaborator

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 = {
Copy link
Collaborator

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);
Copy link
Collaborator

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")
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants