Conversation
Jonas-Isr
commented
Mar 6, 2025
sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/controllers/GroundingController.java
Show resolved
Hide resolved
Jonas-Isr
commented
Mar 6, 2025
sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/controllers/GroundingTest.java
Outdated
Show resolved
Hide resolved
sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/controllers/GroundingTest.java
Outdated
Show resolved
Hide resolved
newtork
requested changes
Mar 10, 2025
Contributor
newtork
left a comment
newtork
left a comment
There was a problem hiding this comment.
Minor code inconsistency
CharlesDuboisSAP
requested changes
Mar 10, 2025
Contributor
CharlesDuboisSAP
left a comment
CharlesDuboisSAP
left a comment
There was a problem hiding this comment.
Add the new endpoint in the HTML homepage, otherwise LGTM
Member
Author
I left it out intentionally because most of the other endpoints are also not part of the HTML page. I did not see a reason to add this when the others like collection/create are not there. |
CharlesDuboisSAP
previously approved these changes
Mar 10, 2025
newtork
reviewed
Mar 10, 2025
| final var collections = this.getAllCollections("json"); | ||
| final var collectionsList = ((CollectionsListResponse) collections).getResources(); | ||
| var statusCode = 0; | ||
| final var deletions = new java.util.ArrayList<>(List.of()); |
Contributor
There was a problem hiding this comment.
(Minor)
Suggested change
| final var deletions = new java.util.ArrayList<>(List.of()); | |
| final var deletions = new java.util.ArrayList<>(); |
You could also use an import statement for that.
Member
Author
There was a problem hiding this comment.
What exactly do you mean with using an import statement?
Contributor
There was a problem hiding this comment.
I mean it's uncommon to use some basic collection class with fully-qualified name:
new java.util.ArrayList<>();Because using FQN here implies that there might be a naming conflict with some custom class that's named ArrayList which would very likely result in confusions and mixups: It has a code smell.
Normally, devs would just put an import statement upfront.
import java.util.ArrayList;
new ArrayList<>();
Member
Author
There was a problem hiding this comment.
Ah, sorry... I was thinking you ment something else (ich stand auf dem Schlauch) 😅
newtork
reviewed
Mar 10, 2025
Comment on lines
226
to
242
| var statusCode = 0; | ||
| final var deletions = new java.util.ArrayList<>(List.of()); | ||
|
|
||
| for (final var collection : collectionsList) { | ||
| if (COLLECTION_TITLE.equals(collection.getTitle())) { | ||
| final var deletion = this.deleteDocuments(collection.getId(), "json"); | ||
| if (deletion instanceof OpenApiResponse) { | ||
| final var status = ((OpenApiResponse) deletion).getStatusCode(); | ||
| deletions.add(deletion); | ||
| statusCode = Math.max(status, statusCode); | ||
| } | ||
| } | ||
| } | ||
| if ("json".equals(format)) { | ||
| return deletions; | ||
| } | ||
| return statusCode >= 400 ? "Failed to delete collections" : "Deletion successful"; |
Contributor
There was a problem hiding this comment.
I like your loop! For comparison, this is what it would've looked with Stream API
Suggested change
| var statusCode = 0; | |
| final var deletions = new java.util.ArrayList<>(List.of()); | |
| for (final var collection : collectionsList) { | |
| if (COLLECTION_TITLE.equals(collection.getTitle())) { | |
| final var deletion = this.deleteDocuments(collection.getId(), "json"); | |
| if (deletion instanceof OpenApiResponse) { | |
| final var status = ((OpenApiResponse) deletion).getStatusCode(); | |
| deletions.add(deletion); | |
| statusCode = Math.max(status, statusCode); | |
| } | |
| } | |
| } | |
| if ("json".equals(format)) { | |
| return deletions; | |
| } | |
| return statusCode >= 400 ? "Failed to delete collections" : "Deletion successful"; | |
| final var deletions = new java.util.ArrayList<>(); | |
| var statusCode = | |
| collectionsList.stream() | |
| .filter(c -> COLLECTION_TITLE.equals(c.getTitle())) | |
| .map(collection -> this.deleteDocuments(collection.getId(), "json")) | |
| .peek(deletions::add) | |
| .mapToInt(deletion -> ((OpenApiResponse) deletion).getStatusCode()) | |
| .max(); | |
| if ("json".equals(format)) { | |
| return deletions; | |
| } | |
| return statusCode.orElse(0) >= 400 ? "Failed to delete collections" : "Deletion successful"; |
However you could still reduce the code a little by dropping the instanceof check
Suggested change
| var statusCode = 0; | |
| final var deletions = new java.util.ArrayList<>(List.of()); | |
| for (final var collection : collectionsList) { | |
| if (COLLECTION_TITLE.equals(collection.getTitle())) { | |
| final var deletion = this.deleteDocuments(collection.getId(), "json"); | |
| if (deletion instanceof OpenApiResponse) { | |
| final var status = ((OpenApiResponse) deletion).getStatusCode(); | |
| deletions.add(deletion); | |
| statusCode = Math.max(status, statusCode); | |
| } | |
| } | |
| } | |
| if ("json".equals(format)) { | |
| return deletions; | |
| } | |
| return statusCode >= 400 ? "Failed to delete collections" : "Deletion successful"; | |
| var statusCode = 0; | |
| final var deletions = new java.util.ArrayList<>(List.of()); | |
| for (final var collection : collectionsList) { | |
| if (COLLECTION_TITLE.equals(collection.getTitle())) { | |
| final var deletion = (OpenApiResponse) this.deleteDocuments(collection.getId(), "json"); | |
| deletions.add(deletion); | |
| statusCode = Math.max(deletion.getStatusCode(), statusCode); | |
| } | |
| } | |
| if ("json".equals(format)) { | |
| return deletions; | |
| } | |
| return statusCode >= 400 ? "Failed to delete collections" : "Deletion successful"; |
(Minor)
- For consistency, please remove the throw statement in
deleteDocuments, the rest of the controller is not throwing (explicitly). - Would you mind renaming
deleteDocumentstodeleteCollection(singular)´? To me it looks like it would tie in better withdeleteCollections(plural).
newtork
approved these changes
Mar 10, 2025
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.
Context
Fix the failing grounding e2e test and also fix a small problem in the sample code.