Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BI-2374 Add delete catch-all to deltabreed brapi controller #427

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

BI-2374 Add delete catch-all to deltabreed brapi controller #427

wants to merge 2 commits into from

Conversation

dmeidlin
Copy link
Contributor

@dmeidlin dmeidlin commented Nov 6, 2024 •
edited by jira bot
Loading

Description

Story: BI-2374

This pull request introduces several changes to the BrAPIV2Controller. Here's a summary of the key modifications:

BrAPIGermplasmController

  • Changed the external reference domain from "breedinginsight.org" to "breeding-insight.net".

BrAPIV2Controller

  • Added support for DELETE requests in the BrAPI v2 endpoints.
  • Introduced a new deleteCatchall method to handle DELETE requests.
  • Implemented executeDeleteRequest method to process DELETE requests.
  • Removed unused imports and cleaned up code.

Other Changes

  • Optimized imports across the modified files.
  • Removed commented-out code and unused variables.
  • Made minor adjustments to improve code readability and maintainability.

Dependencies

The delete endpoints for the BrAPi Java Test Server, see the PR for delete endpoints

Testing

Same testing as described in PR for delete endpoints

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>

@dmeidlin dmeidlin marked this pull request as ready for review November 6, 2024 15:58
nickpalladino
nickpalladino requested changes Nov 7, 2024
View reviewed changes
src/main/java/org/breedinginsight/brapi/v2/BrAPIGermplasmController.java
@@ -178,7 +178,7 @@ private void batchProcessGermplasm(List<BrAPIGermplasm> germplasmList, String pr
Pattern programKeyPattern = Utilities.getRegexPatternMatchAllProgramKeysAnyAccession(programKey);
germplasmList.parallelStream().forEach(germplasm -> {
// Set dbId
germplasm.germplasmDbId(Utilities.getExternalReference(germplasm.getExternalReferences(), "breedinginsight.org")
germplasm.germplasmDbId(Utilities.getExternalReference(germplasm.getExternalReferences(), "breeding-insight.net")
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for this change? Should we be using the value defined in application.yml brapi.server.reference-source

src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java
@@ -186,6 +187,15 @@ public HttpResponse<?> getCatchall(@PathVariable("path") String path, @PathVaria
return executeRequest(path, programId, request, "GET");
}

@Delete("/${micronaut.bi.api.version}/programs/{programId}" + BrapiVersion.BRAPI_V2 + "/{+path}")
@Produces(MediaType.APPLICATION_JSON)
@ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES})
Copy link
Member

Choose a reason for hiding this comment

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

Is this something only a program or system admin should be allowed to do? Wondering if we should make a new role group that is system admin and program admin without readonly.

src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java
@@ -241,6 +251,22 @@ private HttpResponse<String> executeRequest(String path, UUID programId, HttpReq
throw new HttpStatusException(HttpStatus.UNAUTHORIZED, "Unauthorized BrAPI Request");
}

private HttpResponse<String> executeDeleteRequest(String path, UUID programId, HttpRequest<?> request) {
AuthenticatedUser actingUser = securityService.getUser();
Copy link
Member

Choose a reason for hiding this comment

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

actingUser isn't used

nickpalladino
nickpalladino requested changes Nov 7, 2024
View reviewed changes
src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java
@@ -241,6 +251,22 @@ private HttpResponse<String> executeRequest(String path, UUID programId, HttpReq
throw new HttpStatusException(HttpStatus.UNAUTHORIZED, "Unauthorized BrAPI Request");
}

private HttpResponse<String> executeDeleteRequest(String path, UUID programId, HttpRequest<?> request) {
AuthenticatedUser actingUser = securityService.getUser();

Copy link
Member

Choose a reason for hiding this comment

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

Maybe should add a logCall so behavior is consistent with executeByteRequest and executeRequest

Base automatically changed from release/1.0 to main November 8, 2024 18:51
@nickpalladino nickpalladino requested review from davedrp and removed request for HMS17 November 18, 2024 19:55
nickpalladino
nickpalladino reviewed Nov 18, 2024
View reviewed changes
src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java
@Delete("/${micronaut.bi.api.version}/programs/{programId}" + BrapiVersion.BRAPI_V2 + "/{+path}")
@Produces(MediaType.APPLICATION_JSON)
@ProgramSecured(roleGroups = {ProgramSecuredRoleGroup.PROGRAM_SCOPED_ROLES})
public HttpResponse<?> deleteCatchall(@PathVariable("path") String path,
Copy link
Member

Choose a reason for hiding this comment

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

When testing I could do DELETE http://localhost:8081/v1/programs/9d4719fe-bd89-4d54-bbf6-e71f33f87b04/brapi/v2/trials/ with no trialDbId and get success. Seems like it should give an error in that case.

src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java
@@ -186,6 +187,15 @@ public HttpResponse<?> getCatchall(@PathVariable("path") String path, @PathVaria
return executeRequest(path, programId, request, "GET");
}

@Delete("/${micronaut.bi.api.version}/programs/{programId}" + BrapiVersion.BRAPI_V2 + "/{+path}")
Copy link
Member

Choose a reason for hiding this comment

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

When the delete pass through is overridden by specific implementations in future cards, is the plan to use the DeltaBreed external reference id rather than the DbId of the brapi service?

nickpalladino
nickpalladino reviewed Nov 18, 2024
View reviewed changes
src/main/java/org/breedinginsight/brapi/v2/BrAPIV2Controller.java
@@ -186,6 +187,15 @@ public HttpResponse<?> getCatchall(@PathVariable("path") String path, @PathVaria
return executeRequest(path, programId, request, "GET");
}

@Delete("/${micronaut.bi.api.version}/programs/{programId}" + BrapiVersion.BRAPI_V2 + "/{+path}")
@Produces(MediaType.APPLICATION_JSON)
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed in standup, this currently won't delete the associated dataset and that should be captured in the the experiment delete endpoint implementation.

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

@nickpalladino nickpalladino nickpalladino requested changes

@davedrp davedrp davedrp approved these changes

Assignees

@nickpalladino nickpalladino

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmeidlin @davedrp @nickpalladino