Conversation
afranken
force-pushed
the
junie-ctd-2
branch
from
be2d84c to
ea50d00
Compare
afranken
force-pushed
the
junie-ctd-2
branch
2 times, most recently
from
7e47d78 to
0c617f5
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
integration-tests/src/test/kotlin/com/adobe/testing/s3mock/its/S3TestBase.kt
Dismissed
Show dismissed
Hide dismissed
afranken
force-pushed
the
junie-ctd-2
branch
3 times, most recently
from
6ccd67a to
53b9529
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive maintenance improvements including adding more tests, refactoring Kotlin code to idiomatic syntax, and updating build tooling with checkstyle/ktlint formatting fixes.
- Added new tests and expanded test coverage across various components
- Refactored Kotlin code to use more idiomatic syntax, improving readability and maintainability
- Updated Maven wrapper, added ktlint for Kotlin formatting, and fixed checkstyle violations
Reviewed Changes
Copilot reviewed 103 out of 126 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| testsupport/testcontainers/src/main/java/com/adobe/testing/s3mock/testcontainers/S3MockContainer.java | Added Javadoc comment for S3MockContainer class |
| testsupport/testcontainers/pom.xml | Added test scope to dependency |
| testsupport/pom.xml | Added Maven Javadoc skip property and ktlint plugin configuration |
| testsupport/junit4/src/main/java/com/adobe/testing/s3mock/junit4/S3MockRule.java | Reorganized method order for better code structure |
| testsupport/common/src/main/java/com/adobe/testing/s3mock/testsupport/common/S3MockStarter.java | Improved Javadoc formatting with proper spacing |
| server/src/test/kotlin/ | Extensive Kotlin refactoring to idiomatic syntax across test files |
| server/src/main/resources/application.properties | Enabled lazy initialization for Spring |
| server/src/main/java/ | Various improvements including documentation, null safety, and bug fixes |
| mvnw, mvnw.cmd | Updated Maven wrapper to version 3.3.3 |
| integration-tests/src/test/kotlin/ | Converted integration tests to idiomatic Kotlin syntax |
| pom.xml | Added ktlint plugin, updated checkstyle configuration, and improved Maven settings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return File(Objects.requireNonNull(classLoader.getResource(fileName)).file) | ||
| val testClass = requireNotNull(testInfo.testClass.orElse(null)) { "Test class is not present in TestInfo" } | ||
| val resource = requireNotNull(testClass.classLoader.getResource(fileName)) { "Resource not found on classpath: $fileName" } | ||
| return File(resource.toURI()) |
There was a problem hiding this comment.
The toURI() method can throw URISyntaxException which is not handled. Consider wrapping this in a try-catch block or documenting that this method can throw an exception.
Comment on lines
+284
to
+288
| var rootPath = rootFolder.toPath().toAbsolutePath().normalize(); | ||
| var candidate = rootPath.resolve(bucketName).normalize(); | ||
| if (!candidate.startsWith(rootPath)) { | ||
| throw new IllegalArgumentException("Invalid bucket name (path traversal detected)."); | ||
| } |
There was a problem hiding this comment.
Good security improvement to prevent path traversal attacks. However, consider also validating that the bucket name doesn't contain null bytes or other potentially dangerous characters before path resolution.
server/src/main/java/com/adobe/testing/s3mock/service/BucketService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/adobe/testing/s3mock/TaggingHeaderConverter.java
Outdated
Show resolved
Hide resolved
server/src/test/kotlin/com/adobe/testing/s3mock/store/StoreTestBase.kt
Outdated
Show resolved
Hide resolved
| fun `PUT part creates the correct set of folders and files`() { | ||
| val fileName = "PartFile" | ||
| val partNumber = "1" | ||
| val partNumber = 1 |
There was a problem hiding this comment.
The change from String to Int for partNumber is good, but ensure all test assertions and mock verifications are updated consistently throughout the file to expect Int instead of String.
The JavaDoc plugin threw errors during the build. POM type modules do not produce code that can be analyzed for JavaDoc, skip.
Let functions run in random order and in parallel, if possible. This leads to non-deterministic test execution.
S3Mock starts up pretty fast already, but this can't hurt.
The build-config module was never necessary.
The wasn't updated to the latest Google Checkstyle in years. Fix all violations.
Ran "mvn wrapper:wrapper".
No reason to do this in the ObjectController.
Also add tests.
This doesn't change the logic but makes static code analysis happy.
afranken
force-pushed
the
junie-ctd-2
branch
from
bd37fce to
47aaef8
Compare
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
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.
Description
More Junie evaluation and manual edits.
Related Issue
N/A
Tasks