[ALS-10196] [ALS-10195] Add configuration endpoint and table#245
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a configuration management feature: DB migration, JPA entity and DTO, repository and BaseRepository updates, ConfigurationService CRUD, REST endpoints with SUPER_ADMIN-protected writes, JWTFilter allowlist for public reads, persistence registration, and unit tests. ChangesConfiguration Management Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigurationRS
participant ConfigurationService
participant ConfigurationRepository
participant Database
Client->>ConfigurationRS: HTTP request (GET/POST/PATCH/DELETE)
ConfigurationRS->>ConfigurationService: delegate operation
ConfigurationService->>ConfigurationRepository: query/persist/merge/remove
ConfigurationRepository->>Database: SQL SELECT/INSERT/UPDATE/DELETE
ConfigurationService-->>ConfigurationRS: Optional result
ConfigurationRS-->>Client: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
srpiatt
force-pushed
the
feature/configuration2
branch
from
b2f663e to
9ae4483
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@pic-sure-api-data/src/main/java/edu/harvard/dbmi/avillach/data/entity/Configuration.java`:
- Around line 83-87: The Configuration.toString() currently includes the raw
'value' field which may leak secrets; update the Configuration.toString() method
to stop serializing the raw value (field 'value')—either omit it, replace it
with a fixed placeholder like "[REDACTED]" or a boolean flag (e.g., "hasValue":
value != null), and keep other fields the same; modify the
Json.createObjectBuilder() call in the Configuration.toString() implementation
to use the masked/omitted representation for the 'value' field instead of
value.toString().
In
`@pic-sure-api-data/src/main/java/edu/harvard/dbmi/avillach/data/request/ConfigurationRequest.java`:
- Around line 20-23: The name and kind fields in ConfigurationRequest
(ConfigurationRequest.name and ConfigurationRequest.kind) — and the other
Pattern-annotated string field in the same class — lack a max-length constraint
and can exceed the DB column; add a `@Size`(max = 255) annotation to each of these
String fields (and import javax.validation.constraints.Size) so API validation
enforces the 255-character DB limit before persistence.
In
`@pic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/security/JWTFilter.java`:
- Around line 135-143: Normalize the request path from
requestContext.getUriInfo().getPath() before using EXCLUDED_PATHS or comparing
to "/system/status": compute a normalizedPath variable (e.g., ensure it starts
with a leading '/' by prefixing one when missing) and use normalizedPath in the
EXCLUDED_PATHS.stream().anyMatch(...) check and in the path/contentEquals check
(replace path.contentEquals("/system/status") with
normalizedPath.contentEquals("/system/status")), keeping the existing
HttpMethod.GET comparison; update JWTFilter to use normalizedPath everywhere
downstream.
In
`@pic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/service/ConfigurationService.java`:
- Around line 43-52: getConfigurationByIdentifier currently parses the
identifier as a UUID and returns early if getById(uuid) yields no result, which
prevents falling back to name when the identifier is UUID-shaped; modify
getConfigurationByIdentifier to (1) still attempt UUID.fromString(identifier)
and call configurationRepository.getById(uuid) via the existing method, (2) if
getById returns null/empty, then perform the name lookup using
configurationRepository.getByColumn("name", identifier) and return the first
match if present, and (3) retain the existing catch for IllegalArgumentException
to handle non-UUID identifiers and perform the same name lookup there; reference
the method getConfigurationByIdentifier and the repository calls
configurationRepository.getById and configurationRepository.getByColumn.
- Around line 81-90: The code currently calls config.patch(request) on the
managed Configuration before checking for (name, kind) collisions; instead,
create a candidate representation (e.g., compute the would-be name and kind or
clone the config into a transient DTO/object) using request values and run
nameKindPairExists against that candidate, and only call config.patch(request)
on the managed entity if the uniqueness check passes; refer to
ConfigurationService methods using configurationRepository.getById(...),
nameKindPairExists(...), and config.patch(...) to locate where to build/validate
the candidate and defer patching the managed entity until after the check.
In
`@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/ConfigurationRSTest.java`:
- Around line 38-50: The test currently only ensures methods are not restricted
to SUPER_ADMIN; change it to assert public-read explicitly by requiring
`@PermitAll` on the endpoint method (or at the class level). Update
assertNotRestrictedToSuperAdmin to check method.getAnnotation(PermitAll.class)
!= null or if absent, check ConfigurationRS.class.getAnnotation(PermitAll.class)
!= null, and fail if neither is present or if any RolesAllowed exists (e.g.,
ensure rolesAllowed == null). Apply this check for
ConfigurationRS.getConfigurations and ConfigurationRS.getConfigurationById.
In
`@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.java`:
- Around line 133-140: The testExcludedFilterPaths_openapi in JWTFilterTest
currently only verifies setProperty("username") was not called which can miss
cases where the filter aborted the request; update the test to also assert the
request was not aborted and no auth-header was read by verifying
filter.filter(ctx) did not call ctx.abortWith(any(Response.class)) and did not
call ctx.getHeaderString("Authorization") (use verify(ctx, never()) with
appropriate matchers) after invoking filter.filter(ctx) to prevent false
positives.
In
`@pic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/ConfigurationServiceTest.java`:
- Around line 277-297: The test updateConfiguration_changeName_nameKindCollision
is not actually exercising a (name, kind) collision because
request.setKind("some other kind") differs from the duplicate config2.kind;
change the request to use the same kind as the duplicate (request.setKind("some
kind")) so the (name, kind) pair collides, and tighten the stub for
configurationRepository.getByColumns to match the requested name and kind (e.g.
use argument matchers for the request name/kind instead of broad any()) to
ensure the duplicateNames return is triggered when
configurationService.updateConfiguration(request) runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27aaf838-c2bf-4504-8e25-44366a3349d6
📒 Files selected for processing (13)
README.mdpic-sure-api-data/src/main/java/edu/harvard/dbmi/avillach/data/entity/Configuration.javapic-sure-api-data/src/main/java/edu/harvard/dbmi/avillach/data/repository/BaseRepository.javapic-sure-api-data/src/main/java/edu/harvard/dbmi/avillach/data/repository/ConfigurationRepository.javapic-sure-api-data/src/main/java/edu/harvard/dbmi/avillach/data/request/ConfigurationRequest.javapic-sure-api-data/src/main/resources/db/sql/V8__ADD_CONFIG_TABLE.sqlpic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/ConfigurationRS.javapic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/security/JWTFilter.javapic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/service/ConfigurationService.javapic-sure-api-war/src/main/resources/META-INF/persistence.xmlpic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/ConfigurationRSTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/security/JWTFilterTest.javapic-sure-api-war/src/test/java/edu/harvard/dbmi/avillach/service/ConfigurationServiceTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@pic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/security/AuthSecurityContext.java`:
- Line 38: The log in AuthSecurityContext.isUserInRole currently uses
logger.info and should be reduced to diagnostic level and/or gated: change the
call from logger.info(...) to logger.debug(...) or logger.trace(...), and wrap
the logging with a logger.isDebugEnabled() (or isTraceEnabled()) check; also
consider removing sensitive details (user.getRoles() and explicit result) or
only include them behind a development-only flag so production logs do not emit
authorization decisions.
- Around line 36-37: Add a TODO marker and ticket reference to the temporary
diagnostic comment in AuthSecurityContext to ensure it’s tracked for removal;
locate the diagnostic comment in the AuthSecurityContext class (the block
beginning "// TEMP DIAGNOSTIC...") and update it to include a TODO with a ticket
id (e.g., TODO ALS-10196 or create a new issue number) and brief intent/expiry
so reviewers can find and remove the diagnostic after root-causing `@RolesAllowed`
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ba392b4-1611-406f-b6a0-f9b53452ef6d
📒 Files selected for processing (2)
pic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/security/AuthSecurityContext.javapic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/security/JWTFilter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pic-sure-api-war/src/main/java/edu/harvard/dbmi/avillach/security/JWTFilter.java
JamesPeck
reviewed
Jun 3, 2026
Role checks now match against the user's privileges (AuthSecurityContext), so ConfigurationRS @RolesAllowed annotations name the SUPER_ADMIN privilege rather than the 'PIC-SURE Top Admin' role. Update the guard test to assert SUPER_ADMIN, matching the current annotations.
Gcolon021
approved these changes
Jun 5, 2026
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores