KPMP 6414 upgrade deps#34
Conversation
…iopeia-data into KPMP-6414_upgrade-deps
WalkthroughThe pull request upgrades the cassiopeia-data application from Spring Boot 2.x/Java 8 to Spring Boot 4.0.6/Java 21, modernizing the build system from legacy Gradle buildscript syntax to plugins DSL, updating Docker containerization, migrating servlet imports from javax to jakarta namespace, and converting all test code from JUnit 4 to JUnit 5. ChangesSpring Boot 4 and Jakarta Servlet Upgrade
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
1-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the container as a non-root user.
The image currently runs as root (no
USERset), which is an avoidable security risk.Suggested hardening patch
FROM alpine:3.22.4 @@ RUN apk add openjdk21 +RUN addgroup -S app && adduser -S app -G app @@ COPY ${DEPENDENCY}/BOOT-INF/lib /app/lib COPY ${DEPENDENCY}/META-INF /app/META-INF COPY ${DEPENDENCY}/BOOT-INF/classes /app +RUN chown -R app:app /app /tmp @@ ENTRYPOINT ["java","-cp","app:app/lib/*","org.kpmp.Application"] +USER app
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6966e23-8a65-418f-b84b-e858a1e2c2ef
📒 Files selected for processing (18)
.github/workflows/build-gradle-project.ymlDockerfilebuild.gradlegradle/wrapper/gradle-wrapper.propertiessrc/main/java/org/kpmp/error/ErrorController.javasrc/main/java/org/kpmp/logging/LoggingService.javasrc/main/java/org/kpmp/shibboleth/ShibbolethUserService.javasrc/main/java/org/kpmp/slides/SlideController.javasrc/test/java/org/kpmp/WebConfigTest.javasrc/test/java/org/kpmp/error/ErrorControllerTest.javasrc/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.javasrc/test/java/org/kpmp/shibboleth/ShibbolethUserTest.javasrc/test/java/org/kpmp/shibboleth/UTF8EncoderTest.javasrc/test/java/org/kpmp/slides/ParticipantTest.javasrc/test/java/org/kpmp/slides/SlideControllerTest.javasrc/test/java/org/kpmp/slides/SlideServiceTest.javasrc/test/java/org/kpmp/slides/SlideTest.javasrc/test/java/org/kpmp/slides/StainTest.java
| - name: Run build with Gradle Wrapper | ||
| run: | | ||
| ./gradlew build docker | ||
| ./gradlew bootBuildImage |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify build.gradle expects optional -Ptag and otherwise falls back to git branch detection.
rg -nC2 "hasProperty\\('tag'\\)|git rev-parse --abbrev-ref HEAD|bootBuildImage|imageName" build.gradle
# Verify workflow currently calls bootBuildImage without -Ptag while push uses separate tags.
rg -nC3 "IMAGE_TAG|bootBuildImage|docker push" .github/workflows/build-gradle-project.ymlRepository: KPMP/cassiopeia-data
Length of output: 1436
Pass -Ptag to bootBuildImage so the built image tag matches the Docker push tag in CI.
bootBuildImage uses getCurrentGitBranch() which falls back to git rev-parse --abbrev-ref HEAD when -Ptag isn’t provided. The workflow currently builds without -Ptag, but pushes kingstonduo/cassiopeia-data:$IMAGE_TAG for develop (IMAGE_TAG=1.7) and kingstonduo/cassiopeia-data:${current_branch} otherwise—leading to mismatched tags (e.g., detached HEAD yielding :HEAD).
Fix by computing TAG the same way as the push step and running ./gradlew bootBuildImage -Ptag="$TAG".
| RUN apk update && \ | ||
| apk upgrade | ||
| RUN apk add openjdk8 | ||
| RUN apk add openjdk21 |
There was a problem hiding this comment.
Use --no-cache for apk add on Line 5.
Without --no-cache, APK index/cache layers are kept and unnecessarily bloat the image.
Suggested patch
-RUN apk add openjdk21
+RUN apk add --no-cache openjdk21📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN apk add openjdk21 | |
| RUN apk add --no-cache openjdk21 |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 5-5: 'apk add' is missing '--no-cache'
'--no-cache' is missed: apk add openjdk21
Rule: DS-0025
(IaC/Dockerfile)
| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| MockitoAnnotations.initMocks(this); | ||
| MockitoAnnotations.openMocks(this); | ||
| controller = new ErrorController(logger); | ||
| } | ||
|
|
||
| @After | ||
| @AfterEach | ||
| public void tearDown() throws Exception { | ||
| controller = null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant section with line numbers
FILE="src/test/java/org/kpmp/error/ErrorControllerTest.java"
if [ -f "$FILE" ]; then
echo "=== $FILE (lines around 20-60) ==="
nl -ba "$FILE" | sed -n '1,120p'
else
echo "File not found: $FILE"
exit 1
fi
# Search for usage patterns of MockitoAnnotations.openMocks in the test suite
echo "=== Search for MockitoAnnotations.openMocks usage ==="
rg -n "MockitoAnnotations\.openMocks\(" src/test/java || true
# Check if any existing tearDown closes mocks elsewhere in this file
echo "=== Search for ".close()". patterns in this file ==="
rg -n "\.close\(\)" "$FILE" || trueRepository: KPMP/cassiopeia-data
Length of output: 208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the file by name (path may differ)
echo "=== Locating ErrorControllerTest.java ==="
FILE_PATH="$(fd -H "ErrorControllerTest\.java$" . || true | head -n 1)"
echo "Found: ${FILE_PATH:-<none>}"
if [ -z "${FILE_PATH:-}" ]; then
exit 0
fi
# Print a focused section with line numbers (use cat -n since nl may be unavailable)
echo "=== File excerpt (lines 1-160) ==="
cat -n "$FILE_PATH" | sed -n '1,160p'
# Search for MockitoAnnotations.openMocks usage
echo "=== Search for MockitoAnnotations.openMocks usage ==="
rg -n "MockitoAnnotations\.openMocks\(" "$(dirname "$FILE_PATH")" || true
# Search for closing calls in this file
echo "=== Search for .close() in this file ==="
rg -n "\.close\(\)" "$FILE_PATH" || true
# Search for AutoCloseable field/variable in this file
echo "=== Search for AutoCloseable in this file ==="
rg -n "AutoCloseable" "$FILE_PATH" || trueRepository: KPMP/cassiopeia-data
Length of output: 2344
Close Mockito openMocks resources in tearDown.
MockitoAnnotations.openMocks(this) in setUp() returns an AutoCloseable, but tearDown() only nulls controller and never closes the mocks, risking test resource leaks.
Proposed fix
public class ErrorControllerTest {
private ErrorController controller;
+ private AutoCloseable mocks;
`@Mock`
private LoggingService logger;
`@BeforeEach`
public void setUp() throws Exception {
- MockitoAnnotations.openMocks(this);
+ mocks = MockitoAnnotations.openMocks(this);
controller = new ErrorController(logger);
}
`@AfterEach`
public void tearDown() throws Exception {
+ if (mocks != null) {
+ mocks.close();
+ }
controller = null;
}| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| MockitoAnnotations.initMocks(this); | ||
| MockitoAnnotations.openMocks(this); | ||
| shibbolethUserService = new ShibbolethUserService(utf8Encoder); | ||
| } | ||
|
|
||
| @After | ||
| @AfterEach | ||
| public void tearDown() throws Exception { | ||
| shibbolethUserService = null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant test file
FILE="src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java"
if [ -f "$FILE" ]; then
echo "=== $FILE ==="
nl -ba "$FILE" | sed -n '1,200p'
else
echo "File not found: $FILE"
exit 1
fi
# Search for other uses of MockitoAnnotations.openMocks without closing in test sources
echo
echo "=== Search for MockitoAnnotations.openMocks(this) in tests ==="
rg -n --hidden --no-ignore -S "MockitoAnnotations\.openMocks\(\s*this\s*\)" src/test/java || true
# Search for close() on the result of openMocks (rough heuristic)
echo
echo "=== Search for patterns closing mocks returned from openMocks ==="
rg -n --hidden --no-ignore -S "openMocks\\(.*\\).*close\\(" src/test/java || trueRepository: KPMP/cassiopeia-data
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java"
if [ -f "$FILE" ]; then
echo "=== $FILE (first 200 lines) ==="
cat -n "$FILE" | sed -n '1,200p'
else
echo "File not found: $FILE"
exit 1
fi
echo
echo "=== Search for MockitoAnnotations.openMocks(this) in src/test/java ==="
rg -n -S "MockitoAnnotations\.openMocks\(\s*this\s*\)" src/test/java || true
echo
echo "=== Search for close() on the returned value of openMocks (heuristic) ==="
rg -n -S "openMocks\([^)]*\)\s*;|openMocks\([^)]*\)\s*\\)" src/test/java || true
rg -n -S "openMocks\([^)]*\).*close\(" src/test/java || true
rg -n -S "AutoCloseable\s+.*mocks|mocks\.close\(" src/test/java || trueRepository: KPMP/cassiopeia-data
Length of output: 3890
Close MockitoAnnotations.openMocks(...) AutoCloseable in @AfterEach. (src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java:26-35)
MockitoAnnotations.openMocks(this) returns an AutoCloseable that’s currently discarded; store it and close it in tearDown() to avoid mock initialization resource leakage across tests.
Proposed fix
public class ShibbolethUserServiceTest {
private ShibbolethUserService shibbolethUserService;
+ private AutoCloseable mocks;
`@Mock`
private UTF8Encoder utf8Encoder;
`@BeforeEach`
public void setUp() throws Exception {
- MockitoAnnotations.openMocks(this);
+ mocks = MockitoAnnotations.openMocks(this);
shibbolethUserService = new ShibbolethUserService(utf8Encoder);
}
`@AfterEach`
public void tearDown() throws Exception {
+ if (mocks != null) {
+ mocks.close();
+ }
shibbolethUserService = null;
}| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| MockitoAnnotations.initMocks(this); | ||
| MockitoAnnotations.openMocks(this); | ||
| controller = new SlideController(slideService, logger, shibbolethUserService); | ||
| } | ||
|
|
||
| @After | ||
| @AfterEach | ||
| public void tearDown() throws Exception { | ||
| controller = null; | ||
| } |
There was a problem hiding this comment.
Close the Mockito openMocks handle in @AfterEach
MockitoAnnotations.openMocks(this) returns an AutoCloseable, but tearDown() currently only nulls controller, leaving the Mockito open-mocks resources unclosed.
Proposed fix
public class SlideControllerTest {
`@Mock`
private SlideService slideService;
+ private AutoCloseable mocks;
private SlideController controller;
`@Mock`
private LoggingService logger;
`@Mock`
private ShibbolethUserService shibbolethUserService;
`@BeforeEach`
public void setUp() throws Exception {
- MockitoAnnotations.openMocks(this);
+ mocks = MockitoAnnotations.openMocks(this);
controller = new SlideController(slideService, logger, shibbolethUserService);
}
`@AfterEach`
public void tearDown() throws Exception {
+ if (mocks != null) {
+ mocks.close();
+ }
controller = null;
}| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| MockitoAnnotations.initMocks(this); | ||
| MockitoAnnotations.openMocks(this); | ||
| service = new SlideService(participantRepository); | ||
| } | ||
|
|
||
| @After | ||
| @AfterEach | ||
| public void tearDown() throws Exception { | ||
| service = null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file (in case path differs) and print relevant sections with line numbers
fd -a "SlideServiceTest.java" . || true
FILE="src/test/java/org/kpmp/slides/SlideServiceTest.java"
if [ -f "$FILE" ]; then
echo "=== $FILE (lines 1-200) ==="
nl -ba "$FILE" | sed -n '1,200p'
else
echo "File not found: $FILE"
fi
# Search for openMocks usage in repo to see any patterns/other tests
rg -n "MockitoAnnotations\.openMocks\\(this\\)|openMocks\\(" src/test/java/org/kpmp/slides -S || true
rg -n "MockitoAnnotations\.openMocks\\(" src/test/java -S || trueRepository: KPMP/cassiopeia-data
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/test/java/org/kpmp/slides/SlideServiceTest.java"
echo "=== $FILE (first 200 lines) ==="
if [ -f "$FILE" ]; then
cat -n "$FILE" | sed -n '1,200p'
else
echo "File not found: $FILE" >&2
exit 0
fi
echo "=== Search for MockitoAnnotations.openMocks usage ==="
rg -n "MockitoAnnotations\.openMocks\s*\(" src/test/java -S || true
echo "=== Search for closing returned handle (.close()) around openMocks ==="
rg -n "openMocks\s*\(.*\)|close\(\)\s*;|AutoCloseable\s+mocks|MockitoAnnotations\." src/test/java/org/kpmp/slides -S || trueRepository: KPMP/cassiopeia-data
Length of output: 4762
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILES=(
"src/test/java/org/kpmp/slides/SlideControllerTest.java"
"src/test/java/org/kpmp/error/ErrorControllerTest.java"
"src/test/java/org/kpmp/shibboleth/ShibbolethUserServiceTest.java"
)
for f in "${FILES[@]}"; do
echo "=== $f (lines 1-160) ==="
if [ -f "$f" ]; then
cat -n "$f" | sed -n '1,160p'
else
echo "Missing: $f" >&2
fi
done
echo "=== Search across src/test/java for openMocks handle assignment + close ==="
rg -n "MockitoAnnotations\.openMocks\s*\(.*\)|openMocks\s*\(.*\)" src/test/java -S
echo "=== Search across src/test/java for close() near openMocks ==="
rg -n "openMocks\s*\(.*\)[\s\S]{0,200}?close\s*\(" src/test/java -S || true
echo "=== Search across src/test/java for '.close();' on an AutoCloseable mocks handle ==="
rg -n "AutoCloseable\s+[^;]*mocks|mocks\s*=\s*MockitoAnnotations\.openMocks|mocks\.close\(" src/test/java -S || trueRepository: KPMP/cassiopeia-data
Length of output: 8907
Close the MockitoAnnotations.openMocks(...) resource in @AfterEach.
MockitoAnnotations.openMocks(this) (line 25) returns an AutoCloseable, but tearDown() only sets service = null and never closes it.
Proposed fix
public class SlideServiceTest {
`@Mock`
private ParticipantRepository participantRepository;
+ private AutoCloseable mocks;
private SlideService service;
`@BeforeEach`
public void setUp() throws Exception {
- MockitoAnnotations.openMocks(this);
+ mocks = MockitoAnnotations.openMocks(this);
service = new SlideService(participantRepository);
}
`@AfterEach`
public void tearDown() throws Exception {
+ if (mocks != null) {
+ mocks.close();
+ }
service = null;
}
2 participants
Summary by CodeRabbit
Release Notes
Dependencies
Bug Fixes
Chores