feat: Update to Jackson 3, update all deps, add RX3.#12
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughAdded CI/CD workflows and GitHub templates, migrated to Java 17 and Jackson "tools.jackson" packages, introduced an RxJava3 module, refactored non-blocking/reactive JSON readers (closeable, removed checked IOExceptions), migrated tests to JUnit 5, and updated multiple POMs and tests. Changes
sequenceDiagram
participant Publisher as Publisher<ByteBuffer>
participant Rx3Reader as Rx3ObjectReader
participant NBReader as NonBlockingObjectReader
participant ObjectReader as ObjectReader
participant Subscriber as Flowable Subscriber
Publisher->>Rx3Reader: stream chunks
Rx3Reader->>NBReader: create NonBlockingObjectReader (using(...), manages close)
Rx3Reader->>NBReader: feed chunk -> readObjects(chunk)
NBReader->>ObjectReader: deserialize TokenBuffer -> Object
ObjectReader-->>NBReader: object
NBReader-->>Rx3Reader: iterable of objects
Rx3Reader->>Subscriber: emit objects (Flowable)
Publisher->>Rx3Reader: complete stream
Rx3Reader->>NBReader: call endOfInput()
NBReader-->>Rx3Reader: tail objects
Rx3Reader->>Subscriber: emit tail objects, complete
Rx3Reader->>NBReader: close() (disposed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pom.xml (3)
7-7:⚠️ Potential issue | 🔴 CriticalCI pipeline failure — SNAPSHOT in effective dependency management.
The build job fails with
SNAPSHOT versions found in effective POM dependency management. The only source is the intra-reactorjson-nonblockingentry at${project.version}(line 91), which resolves to0.0.5-SNAPSHOTfrom line 7. Options:
- Relax the CI check to ignore intra-project (
com.playtika.reactivejson:*) coordinates — most common approach for multi-module projects still in development.- Or cut a non-SNAPSHOT release on
developbefore merging.This needs to be resolved for the pipeline to pass; the current code itself is correct Maven.
Also applies to: 85-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` at line 7, The pipeline fails because the project <version>0.0.5-SNAPSHOT</version> produces a SNAPSHOT dependency (the intra-reactor module json-nonblocking at ${project.version}); either change the module version to a non-SNAPSHOT release (bump <version> to a release like 0.0.5) or relax the CI check to ignore intra-project coordinates (e.g., exclude groupId com.playtika.reactivejson or artifactId json-nonblocking in the SNAPSHOT detection rule) so the effective POM no longer fails the SNAPSHOT dependency management check.
19-39:⚠️ Potential issue | 🟡 MinorUpdate GitHub organization URLs from
PlaytikatoPlaytikaOSS.The repository is hosted at
github.com/PlaytikaOSS/json-reactive, but<url>,<issueManagement>, and<scm>in pom.xml still reference the oldPlaytikaorganization. This will cause broken links in published POM metadata on Maven Central.📝 Proposed fix
- <url>https://github.com/Playtika/json-reactive</url> + <url>https://github.com/PlaytikaOSS/json-reactive</url> <issueManagement> <system>Github</system> - <url>https://github.com/Playtika/json-reactive/issues</url> + <url>https://github.com/PlaytikaOSS/json-reactive/issues</url> </issueManagement> @@ <scm> - <url>https://github.com/Playtika/json-reactive</url> - <connection>scm:git:git://github.com/Playtika/json-reactive.git</connection> - <developerConnection>scm:git:git@github.com:Playtika/json-reactive.git</developerConnection> + <url>https://github.com/PlaytikaOSS/json-reactive</url> + <connection>scm:git:git://github.com/PlaytikaOSS/json-reactive.git</connection> + <developerConnection>scm:git:git@github.com:PlaytikaOSS/json-reactive.git</developerConnection> <tag>HEAD</tag> </scm>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 19 - 39, Update the Maven POM metadata entries that still point to the old GitHub organization by replacing "Playtika" with "PlaytikaOSS" in the <url>, the <issueManagement><url>, and all <scm> entries (including <connection> and <developerConnection>) so published POM links point to https://github.com/PlaytikaOSS/json-reactive; ensure the <scm><url>, <scm><connection>, <scm><developerConnection> and the top-level <url> values all reflect the PlaytikaOSS organization.
106-118:⚠️ Potential issue | 🟡 MinorUpgrade
central-publishing-maven-pluginto version 0.10.0.BOM/dependency additions are correct:
tools.jackson:jackson-bom:3.1.2,org.junit:junit-bom:6.0.3, andio.reactivex.rxjava3:rxjava:3.1.10are all valid, published coordinates requiring Java 17 which matches thejava.versionbump.However,
central-publishing-maven-plugin:0.7.0at line 76 is outdated. The latest stable version is0.10.0(released January 7, 2026). Update to use the newer version for improved compatibility with Maven Central publishing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 106 - 118, Update the central publishing plugin version from 0.7.0 to 0.10.0 in the pom: locate the plugin declaration containing <artifactId>central-publishing-maven-plugin</artifactId> (and its current <version>0.7.0</version>) and change that <version> element to 0.10.0 to use the latest stable release.
🧹 Nitpick comments (7)
.github/workflows/required-labels.yml (1)
6-16: Optional: declare least-privilegepermissions.For defense-in-depth, restrict this job to read-only token scope since it only inspects PR labels.
jobs: label: if: github.event.pull_request.state == 'open' runs-on: ubuntu-latest + permissions: + pull-requests: read name: Verify Pull Request has labels🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/required-labels.yml around lines 6 - 16, Add least-privilege permissions to the workflow so the label verification job only has read access; update the job named "label" (where you call mheap/github-action-required-labels@v5) to include a permissions block granting pull-requests: read (or at workflow level if preferred) instead of using the full GITHUB_TOKEN scope..github/workflows/release.yml (3)
54-67:if: ${{ success() }}is redundant.
success()is the default for each step; the guard adds nothing but noise. Safe to drop on all three steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 54 - 67, Remove the redundant conditional from the three workflow steps named "Update version", "Publish to the Maven Central Repository", and "Commit & Push changes": delete the lines containing if: ${{ success() }} so each step runs under the default success() behavior; ensure no other logic depends on those explicit guards and leave the rest of each step (run and env blocks) unchanged.
25-28: Add a least‑privilegepermissions:block.No
permissions:is set, so the default (repo‑wide)GITHUB_TOKENscope applies. The push back to the release branch is already done with the app token, soGITHUB_TOKENcan be locked down, e.g.:permissions: contents: read🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 25 - 28, Add a least-privilege permissions block for the publish job so the default repo‑wide GITHUB_TOKEN scope is not used; update the jobs.publish definition to include a permissions section that limits GITHUB_TOKEN (e.g., set contents: read) so only the minimal scope is granted during the publish steps that use GITHUB_TOKEN.
54-56: Pinversions-maven-pluginwith a fully qualified coordinate for reproducible releases.The
versions:setgoal resolves via Maven's plugin prefix registry without an explicitpluginManagemententry, but this makes the plugin version nondeterministic across runners. Usemvn org.codehaus.mojo:versions-maven-plugin:2.17.1:set -DnewVersion=${{github.event.release.tag_name}}(or equivalent with your desired version) to ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 54 - 56, The "Update version" step currently calls the plugin by prefix (mvn ... versions:set versions:commit) which lets Maven pick a plugin version non-deterministically; change the run command to invoke the fully qualified plugin coordinates with a specific version for both goals, e.g. replace the existing mvn call with mvn org.codehaus.mojo:versions-maven-plugin:2.17.1:set -DnewVersion=${{github.event.release.tag_name}} org.codehaus.mojo:versions-maven-plugin:2.17.1:commit (using your chosen plugin version) so the versions-maven-plugin version is pinned and releases are reproducible.json-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.java (2)
52-58: Minor:assertResult(testEntities)on aFlowable<TestEntity>relies on vararg spreading.
TestEntity[]is passed intoassertResult(T...)and works today because the compiler elides the array wrap, but it's easy to misread and the exact behaviour depends on the RxJava 3 overload resolution (thetest()result implements bothassertResult(T...)andassertValues(...)). Consider.assertValueSequence(Arrays.asList(testEntities)).assertComplete().assertNoErrors()for intent-clarity — same coverage, no varargs gotcha.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@json-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.java` around lines 52 - 58, The test currently calls assertResult(testEntities) on the Flowable<TestEntity> produced by reader.readElements (variable testEntityRed), which relies on vararg array spreading and is ambiguous; update the assertion chain on testEntityRed.test() to explicitly assert the sequence and terminal state by replacing assertResult(...) with assertValueSequence(Arrays.asList(testEntities)).assertComplete().assertNoErrors() to make intent clear and avoid varargs/overload pitfalls when asserting the output of reader.readElements(...)/objectMapper.readerFor(TestEntity.class).
101-117: Recommended refactor: deduplicatestringBuffer/divideArrayacross test modules.Identical helpers now exist in
Rx3ObjectReaderTest,Rx2ObjectReaderTest, andReactorObjectReaderTest. Worth extracting a small test-support artifact (or atest-jarfromjson-nonblocking) so the chunking logic has one home.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@json-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.java` around lines 101 - 117, Duplicate test helpers stringBuffer(...) and divideArray(...) used in Rx3ObjectReaderTest, Rx2ObjectReaderTest, and ReactorObjectReaderTest should be extracted into a shared test utility: create a test-support class (e.g., TestBuffers.chunker or similar) in a test-jar or small test-support artifact (or add to json-nonblocking test-jar) and replace the local implementations in those tests with calls to that shared helper; update imports and remove the duplicate methods from Rx3ObjectReaderTest, Rx2ObjectReaderTest, and ReactorObjectReaderTest so all tests call the single shared utility.pom.xml (1)
50-53: Redundant compiler source/target properties.With
maven.compiler.releaseset (Java 9+),maven.compiler.sourceandmaven.compiler.targetare redundant and can make future JDK upgrades slightly error-prone if they drift. Not blocking.♻️ Optional cleanup
<java.version>17</java.version> - <maven.compiler.source>${java.version}</maven.compiler.source> - <maven.compiler.target>${java.version}</maven.compiler.target> <maven.compiler.release>${java.version}</maven.compiler.release>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 50 - 53, Remove the redundant maven.compiler.source and maven.compiler.target properties and keep only maven.compiler.release for Java configuration; specifically delete or omit the <maven.compiler.source> and <maven.compiler.target> entries so the build relies on <maven.compiler.release>${java.version}</maven.compiler.release>, ensuring future JDK upgrades aren't conflicted by drift between source/target and release settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/ISSUE_TEMPLATE/bug_report.md:
- Around line 16-17: Update the user-facing bug report template string
"**Enviroment (please complete the following information):**" to correct the
typo to "Environment (please complete the following information):" and change
the example OS value "MacOS" to the official casing "macOS" so the template
reads "**Environment (please complete the following information):** - OS name :
[e.g. macOS]".
In @.github/labeler.yml:
- Around line 1-3: The current .github/labeler.yml uses a non-standard leading
slash in the documentation rule ("documentation" -> changed-files ->
any-glob-to-any-file: '/**/*.md') which won't match GitHub's repo-relative
paths; change that glob to '**/*.md'. Also expand auto-labeling by adding rules
for the other required labels — at minimum add a "dependencies" rule matching
common dependency files (e.g., '**/pom.xml' or similar build files) and a
"housekeeping" rule matching repository config/infra changes (e.g.,
'.github/**'); update the top-level keys ("documentation", "dependencies",
"housekeeping") under changed-files so PRs are auto-labeled instead of requiring
manual labeling.
In @.github/PULL_REQUEST_TEMPLATE/pull_request_template.md:
- Around line 1-14: The PR template file is placed in
.github/PULL_REQUEST_TEMPLATE/pull_request_template.md which requires a
?template= query param and won't be auto-detected; move or rename that file to
.github/pull_request_template.md so GitHub auto-detects and shows the template
for new PRs, ensuring the content under "Motivation/Modification/Result" remains
identical after the move.
In @.github/workflows/changelog-release-drafter.yml:
- Around line 9-15: Add an explicit permissions block to the
update_release_draft job that grants the GITHUB_TOKEN the minimal rights
release-drafter needs: allow writing releases and reading PR metadata. Update
the job (update_release_draft) that uses release-drafter/release-drafter@v6 to
include a permissions section such as: permissions: pull-requests: read and
contents: write so the action can read PR info and create/update the draft
release.
In @.github/workflows/codeql-analysis.yml:
- Around line 1-28: Replace the compromised aquasecurity/trivy-action@master
usage with a pinned safe version (e.g., aquasecurity/trivy-action@v0.35.0 or a
specific commit SHA) in the Trivy workflow (the step using
aquasecurity/trivy-action), add the required repository permission
"security-events: write" in the workflow permissions block so
github/codeql-action/upload-sarif@v4 can successfully publish results, and
consider renaming the workflow file from codeql-analysis.yml to trivy.yml and
expanding severity to "CRITICAL,HIGH" for the trivy run (update the scan step's
severity field) to capture high-level findings.
In @.github/workflows/maven.yml:
- Around line 42-51: The current SNAPSHOT check that greps effective-pom.xml
into SNAPSHOTS is brittle; update the "Check for SNAPSHOT dependencies in
dependency management" step to parse the generated effective-pom.xml with an
XML-aware tool (e.g., xmllint --xpath or xmlstarlet) and target only
dependencyManagement version elements (e.g., XPath
/project/dependencyManagement//version[contains(text(),'SNAPSHOT')]) instead of
grepping the whole file or excluding the repo slug; keep the existing flow of
creating and removing effective-pom.xml, capture the XPath output into SNAPSHOTS
(or equivalent), and fail the job (exit 1) if any matches are found so only real
dependencyManagement SNAPSHOT versions are detected.
In @.github/workflows/release.yml:
- Around line 6-23: The workflow declares inputs dryRun and logLevel but never
uses them; update the job/step that runs mvn (e.g., the step running "mvn
deploy") to conditionally run only when dryRun is false (use an if: expression
referencing inputs.dryRun) and pass the selected log level into Maven by
appending a JVM/SLF4J property like -Dorg.slf4j.simpleLogger.defaultLogLevel=${{
inputs.logLevel }} (or another appropriate -D property) to the mvn command;
ensure you reference the inputs as inputs.dryRun and inputs.logLevel and keep
the deploy step gated so the UI controls take effect.
- Around line 68-72: Replace the floating actions-js/push@master ref with a
pinned immutable commit SHA (e.g. change uses: actions-js/push@master to uses:
actions-js/push@<commit-sha>) so the workflow no longer follows upstream master;
also audit and similarly pin actions/create-github-app-token (used earlier as
steps.app-token) to a stable tag or SHA to eliminate supply‑chain risk while
keeping the same inputs (github_token: ${{ steps.app-token.outputs.token }},
message, branch).
In `@json-nonblocking/src/main/java/reactivejson/NonBlockingObjectReader.java`:
- Around line 25-44: Both readObjects and endOfInput call tokenBuffer.asParser()
which creates a parser with ObjectReadContext.empty(), causing custom
deserializers to fail; fix by passing a real ObjectReadContext into asParser:
obtain the reader's ObjectReadContext (or otherwise create/propagate a non-empty
ObjectReadContext from the ObjectReader used by this class) and change the calls
in NonBlockingObjectReader.readObjects and NonBlockingObjectReader.endOfInput to
tokenBuffer.asParser(thatRealObjectReadContext); if needed, thread the
ObjectReadContext through the Tokenizer or construct TokenBuffer instances with
a real context instead of ObjectReadContext.empty() so custom `@JsonDeserialize`
deserializers receive correct context.
In `@json-rx3/src/test/java/reactivejson/domain/TestEntity.java`:
- Around line 25-31: The equals implementation in TestEntity can throw NPE when
name is null and you also need to implement hashCode to honor the
equals/hashCode contract; update TestEntity.equals to use a null-safe comparison
(e.g., Objects.equals(((TestEntity) obj).name, this.name) and compare id safely)
and add a corresponding `@Override` public int hashCode() in TestEntity that
returns Objects.hash(id, name) (or equivalent) so both id and name are used
consistently between equals and hashCode.
---
Outside diff comments:
In `@pom.xml`:
- Line 7: The pipeline fails because the project
<version>0.0.5-SNAPSHOT</version> produces a SNAPSHOT dependency (the
intra-reactor module json-nonblocking at ${project.version}); either change the
module version to a non-SNAPSHOT release (bump <version> to a release like
0.0.5) or relax the CI check to ignore intra-project coordinates (e.g., exclude
groupId com.playtika.reactivejson or artifactId json-nonblocking in the SNAPSHOT
detection rule) so the effective POM no longer fails the SNAPSHOT dependency
management check.
- Around line 19-39: Update the Maven POM metadata entries that still point to
the old GitHub organization by replacing "Playtika" with "PlaytikaOSS" in the
<url>, the <issueManagement><url>, and all <scm> entries (including <connection>
and <developerConnection>) so published POM links point to
https://github.com/PlaytikaOSS/json-reactive; ensure the <scm><url>,
<scm><connection>, <scm><developerConnection> and the top-level <url> values all
reflect the PlaytikaOSS organization.
- Around line 106-118: Update the central publishing plugin version from 0.7.0
to 0.10.0 in the pom: locate the plugin declaration containing
<artifactId>central-publishing-maven-plugin</artifactId> (and its current
<version>0.7.0</version>) and change that <version> element to 0.10.0 to use the
latest stable release.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 54-67: Remove the redundant conditional from the three workflow
steps named "Update version", "Publish to the Maven Central Repository", and
"Commit & Push changes": delete the lines containing if: ${{ success() }} so
each step runs under the default success() behavior; ensure no other logic
depends on those explicit guards and leave the rest of each step (run and env
blocks) unchanged.
- Around line 25-28: Add a least-privilege permissions block for the publish job
so the default repo‑wide GITHUB_TOKEN scope is not used; update the jobs.publish
definition to include a permissions section that limits GITHUB_TOKEN (e.g., set
contents: read) so only the minimal scope is granted during the publish steps
that use GITHUB_TOKEN.
- Around line 54-56: The "Update version" step currently calls the plugin by
prefix (mvn ... versions:set versions:commit) which lets Maven pick a plugin
version non-deterministically; change the run command to invoke the fully
qualified plugin coordinates with a specific version for both goals, e.g.
replace the existing mvn call with mvn
org.codehaus.mojo:versions-maven-plugin:2.17.1:set
-DnewVersion=${{github.event.release.tag_name}}
org.codehaus.mojo:versions-maven-plugin:2.17.1:commit (using your chosen plugin
version) so the versions-maven-plugin version is pinned and releases are
reproducible.
In @.github/workflows/required-labels.yml:
- Around line 6-16: Add least-privilege permissions to the workflow so the label
verification job only has read access; update the job named "label" (where you
call mheap/github-action-required-labels@v5) to include a permissions block
granting pull-requests: read (or at workflow level if preferred) instead of
using the full GITHUB_TOKEN scope.
In `@json-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.java`:
- Around line 52-58: The test currently calls assertResult(testEntities) on the
Flowable<TestEntity> produced by reader.readElements (variable testEntityRed),
which relies on vararg array spreading and is ambiguous; update the assertion
chain on testEntityRed.test() to explicitly assert the sequence and terminal
state by replacing assertResult(...) with
assertValueSequence(Arrays.asList(testEntities)).assertComplete().assertNoErrors()
to make intent clear and avoid varargs/overload pitfalls when asserting the
output of reader.readElements(...)/objectMapper.readerFor(TestEntity.class).
- Around line 101-117: Duplicate test helpers stringBuffer(...) and
divideArray(...) used in Rx3ObjectReaderTest, Rx2ObjectReaderTest, and
ReactorObjectReaderTest should be extracted into a shared test utility: create a
test-support class (e.g., TestBuffers.chunker or similar) in a test-jar or small
test-support artifact (or add to json-nonblocking test-jar) and replace the
local implementations in those tests with calls to that shared helper; update
imports and remove the duplicate methods from Rx3ObjectReaderTest,
Rx2ObjectReaderTest, and ReactorObjectReaderTest so all tests call the single
shared utility.
In `@pom.xml`:
- Around line 50-53: Remove the redundant maven.compiler.source and
maven.compiler.target properties and keep only maven.compiler.release for Java
configuration; specifically delete or omit the <maven.compiler.source> and
<maven.compiler.target> entries so the build relies on
<maven.compiler.release>${java.version}</maven.compiler.release>, ensuring
future JDK upgrades aren't conflicted by drift between source/target and release
settings.
🪄 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
Run ID: bcffc358-6546-4cf4-97a5-f462a5c49ed7
📒 Files selected for processing (30)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/PULL_REQUEST_TEMPLATE/pull_request_template.md.github/labeler.yml.github/release-drafter.yml.github/workflows/changelog-release-drafter.yml.github/workflows/codeql-analysis.yml.github/workflows/labeler.yml.github/workflows/maven.yml.github/workflows/release.yml.github/workflows/required-labels.ymljson-nonblocking/pom.xmljson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/JsonFactory.javajson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/NonBlockingJsonParser.javajson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/package-info.javajson-nonblocking/src/main/java/reactivejson/NonBlockingObjectReader.javajson-nonblocking/src/main/java/reactivejson/Tokenizer.javajson-nonblocking/src/test/java/reactivejson/TokenizerTest.javajson-reactor/pom.xmljson-reactor/src/main/java/reactivejson/ReactorObjectReader.javajson-reactor/src/test/java/reactivejson/ReactorObjectReaderTest.javajson-reactor/src/test/java/reactivejson/domain/TestEntity.javajson-rx2/pom.xmljson-rx2/src/main/java/reactivejson/Rx2ObjectReader.javajson-rx2/src/test/java/reactivejson/Rx2ObjectReaderTest.javajson-rx3/pom.xmljson-rx3/src/main/java/reactivejson/Rx3ObjectReader.javajson-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.javajson-rx3/src/test/java/reactivejson/domain/TestEntity.javapom.xml
💤 Files with no reviewable changes (2)
- json-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/package-info.java
- json-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/JsonFactory.java
Periecle
force-pushed
the
feature/jackson-3
branch
from
8c44f2b to
14a9244
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/codeql-analysis.yml (1)
19-19:⚠️ Potential issue | 🔴 CriticalPin Trivy Action instead of using mutable
@master.This security workflow still executes a moving branch. Pin it to a known-safe release or, preferably, a full commit SHA; the Aqua advisory lists
v0.35.0as the safe Trivy Action version and recommends SHA pinning: GHSA-69fq-xp46-6x23🔒 Proposed fix
- uses: aquasecurity/trivy-action@master + uses: aquasecurity/trivy-action@57a97c7e7821a5776cebc9bb87c984fa69cba8f1 # v0.35.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codeql-analysis.yml at line 19, Replace the mutable reference "uses: aquasecurity/trivy-action@master" with a pinned release or commit SHA to avoid running a moving branch; update the workflow step to use the known-safe tag (e.g., v0.35.0) or ideally a full commit SHA for aquasecurity/trivy-action so the action is immutably pinned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codeql-analysis.yml:
- Around line 23-25: The workflow currently sets Trivy inputs format: 'sarif',
output: 'trivy-results.sarif', and severity: 'CRITICAL' but omits
limit-severities-for-sarif; add the input limit-severities-for-sarif: true
alongside those keys so SARIF output will be filtered to the specified severity
(when format is 'sarif' and severity is 'CRITICAL'); update the Trivy step to
include limit-severities-for-sarif: true to ensure only CRITICAL alerts are
uploaded.
In `@json-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.java`:
- Around line 69-70: The test serializes multiple TestEntity objects into a
Publisher<ByteBuffer> using Flowable.fromArray(testEntities).flatMap(...), which
can interleave byte chunks and break the reader (Rx3ObjectReader.readImpl) that
expects ordered byte streams; change the stream composition to use concatMap
instead of flatMap so each object's bytes are fully emitted before the next
begins (i.e., replace flatMap with concatMap when producing byteBuffers from
testEntities) to preserve ordering required by Rx3ObjectReader.readImpl.
---
Duplicate comments:
In @.github/workflows/codeql-analysis.yml:
- Line 19: Replace the mutable reference "uses:
aquasecurity/trivy-action@master" with a pinned release or commit SHA to avoid
running a moving branch; update the workflow step to use the known-safe tag
(e.g., v0.35.0) or ideally a full commit SHA for aquasecurity/trivy-action so
the action is immutably pinned.
🪄 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
Run ID: 35ce8aa7-f08b-49db-8ce0-5c96999876cf
📒 Files selected for processing (32)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/labeler.yml.github/pull_request_template.md.github/release-drafter.yml.github/workflows/changelog-release-drafter.yml.github/workflows/codeql-analysis.yml.github/workflows/labeler.yml.github/workflows/maven.yml.github/workflows/release.yml.github/workflows/required-labels.ymljson-nonblocking/pom.xmljson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/JsonFactory.javajson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/NonBlockingJsonParser.javajson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/package-info.javajson-nonblocking/src/main/java/reactivejson/NonBlockingObjectReader.javajson-nonblocking/src/main/java/reactivejson/Tokenizer.javajson-nonblocking/src/test/java/reactivejson/NonBlockingObjectReaderTest.javajson-nonblocking/src/test/java/reactivejson/TokenizerTest.javajson-reactor/pom.xmljson-reactor/src/main/java/reactivejson/ReactorObjectReader.javajson-reactor/src/test/java/reactivejson/ReactorObjectReaderTest.javajson-reactor/src/test/java/reactivejson/domain/TestEntity.javajson-rx2/pom.xmljson-rx2/src/main/java/reactivejson/Rx2ObjectReader.javajson-rx2/src/test/java/reactivejson/Rx2ObjectReaderTest.javajson-rx2/src/test/java/reactivejson/domain/TestEntity.javajson-rx3/pom.xmljson-rx3/src/main/java/reactivejson/Rx3ObjectReader.javajson-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.javajson-rx3/src/test/java/reactivejson/domain/TestEntity.javapom.xml
💤 Files with no reviewable changes (2)
- json-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/package-info.java
- json-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/JsonFactory.java
✅ Files skipped from review due to trivial changes (14)
- .github/labeler.yml
- .github/pull_request_template.md
- .github/ISSUE_TEMPLATE/feature_request.md
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/workflows/labeler.yml
- json-reactor/pom.xml
- json-rx2/pom.xml
- .github/workflows/required-labels.yml
- .github/workflows/changelog-release-drafter.yml
- .github/release-drafter.yml
- json-rx3/src/test/java/reactivejson/domain/TestEntity.java
- .github/workflows/maven.yml
- json-rx3/pom.xml
- json-nonblocking/src/main/java/reactivejson/Tokenizer.java
🚧 Files skipped from review as they are similar to previous changes (10)
- json-rx2/src/main/java/reactivejson/Rx2ObjectReader.java
- json-reactor/src/main/java/reactivejson/ReactorObjectReader.java
- .github/workflows/release.yml
- json-reactor/src/test/java/reactivejson/domain/TestEntity.java
- json-nonblocking/src/main/java/reactivejson/NonBlockingObjectReader.java
- json-reactor/src/test/java/reactivejson/ReactorObjectReaderTest.java
- json-rx3/src/main/java/reactivejson/Rx3ObjectReader.java
- json-rx2/src/test/java/reactivejson/Rx2ObjectReaderTest.java
- json-nonblocking/src/test/java/reactivejson/TokenizerTest.java
- pom.xml
ijusti
approved these changes
Apr 20, 2026
Periecle
force-pushed
the
feature/jackson-3
branch
from
14a9244 to
d824490
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
json-nonblocking/src/main/java/reactivejson/Tokenizer.java (1)
128-138:⚠️ Potential issue | 🟠 MajorFlush top-level array elements that are arrays.
With
tokenizeArrayElements=true, input like[[1],[2]]drops the inner arrays: after an innerEND_ARRAY,arrayDepth == 1, but Line 135 only flushesEND_OBJECTor scalar values. Include inner-array completion while still excluding the outer top-levelEND_ARRAY.🐛 Proposed fix
if (this.objectDepth == 0 && (this.arrayDepth == 0 || this.arrayDepth == 1) && - (token == JsonToken.END_OBJECT || token.isScalarValue())) { + (token == JsonToken.END_OBJECT || token.isScalarValue() || + (token == JsonToken.END_ARRAY && this.arrayDepth == 1))) { result.add(this.tokenBuffer); this.tokenBuffer = TokenBuffer.forBuffering(this.parser, ObjectReadContext.empty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@json-nonblocking/src/main/java/reactivejson/Tokenizer.java` around lines 128 - 138, In processTokenArray, when tokenizeArrayElements is enabled you need to also flush completed inner arrays so inner END_ARRAY tokens cause the current tokenBuffer to be added, but still avoid flushing the outer/top-level END_ARRAY; update the flush condition in processTokenArray (method name: processTokenArray, vars: tokenBuffer, arrayDepth, objectDepth) to treat JsonToken.END_ARRAY as a flush trigger only when arrayDepth == 1 (i.e., add token == JsonToken.END_ARRAY to the OR list but guard it with this.arrayDepth == 1), then add the tokenBuffer to result and recreate tokenBuffer as before.
🧹 Nitpick comments (2)
pom.xml (2)
228-246: JaCoCoreportgoal is bound to thetestphase — fragile ordering.Both
maven-surefire-plugin:testand thisjacoco:reportexecution bind to thetestphase. They currently run in the correct order only because the plugins are declared in that order in<build><plugins>. If a future reorder (or a child module overriding plugin order) putsjacoco:reportbefore surefire, the report will be generated with no (or stale)jacoco.execdata and coverage will appear empty without an obvious error.The conventional binding for
reportisverify(or at minimumprepare-package), which deterministically runs after all tests have executed:♻️ Proposed fix
<execution> <id>report</id> - <phase>test</phase> + <phase>verify</phase> <goals> <goal>report</goal> </goals> </execution>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 228 - 246, The jacoco-maven-plugin execution with id "report" is currently bound to the "test" phase which risks running before tests (and surefire) if plugin ordering changes; update the execution for artifactId "jacoco-maven-plugin" and execution id "report" to bind it to a later lifecycle phase (e.g., "verify" — or at minimum "prepare-package") so jacoco:report always runs after tests and picks up the generated coverage data.
50-53: Dropmaven.compiler.source/targetin favor ofmaven.compiler.release.With
maven-compiler-plugin3.13.0 and JDK 17,maven.compiler.release=17already sets both source and target correctly while also constraining against the module's JDK API. Keeping all three is redundant and can silently diverge if one is ever changed in isolation. Since no other Maven tooling here depends on the legacy properties, consider removing them.♻️ Proposed cleanup
<java.version>17</java.version> - <maven.compiler.source>${java.version}</maven.compiler.source> - <maven.compiler.target>${java.version}</maven.compiler.target> <maven.compiler.release>${java.version}</maven.compiler.release>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 50 - 53, Remove the redundant maven.compiler.source and maven.compiler.target properties and keep only maven.compiler.release (set to ${java.version}) so the compiler plugin uses the release flag to control source/target and API surface; locate the properties block containing <java.version>, <maven.compiler.source>, <maven.compiler.target>, and <maven.compiler.release> and delete the two legacy properties while leaving <java.version> and <maven.compiler.release> intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@json-nonblocking/src/test/java/reactivejson/NonBlockingObjectReaderTest.java`:
- Around line 41-53: Test opens a NonBlockingObjectReader but doesn't close it;
wrap the NonBlockingObjectReader creation in a try-with-resources so the reader
(and its owned parser) is closed after use. Change the test to construct
NonBlockingObjectReader via try (NonBlockingObjectReader reader = new
NonBlockingObjectReader(jsonFactory, false, mapper.readerFor(Wrapper.class))) {
... } and keep existing calls to reader.readObjects(...) and reader.endOfInput()
inside the try block to ensure proper resource cleanup.
---
Outside diff comments:
In `@json-nonblocking/src/main/java/reactivejson/Tokenizer.java`:
- Around line 128-138: In processTokenArray, when tokenizeArrayElements is
enabled you need to also flush completed inner arrays so inner END_ARRAY tokens
cause the current tokenBuffer to be added, but still avoid flushing the
outer/top-level END_ARRAY; update the flush condition in processTokenArray
(method name: processTokenArray, vars: tokenBuffer, arrayDepth, objectDepth) to
treat JsonToken.END_ARRAY as a flush trigger only when arrayDepth == 1 (i.e.,
add token == JsonToken.END_ARRAY to the OR list but guard it with
this.arrayDepth == 1), then add the tokenBuffer to result and recreate
tokenBuffer as before.
---
Nitpick comments:
In `@pom.xml`:
- Around line 228-246: The jacoco-maven-plugin execution with id "report" is
currently bound to the "test" phase which risks running before tests (and
surefire) if plugin ordering changes; update the execution for artifactId
"jacoco-maven-plugin" and execution id "report" to bind it to a later lifecycle
phase (e.g., "verify" — or at minimum "prepare-package") so jacoco:report always
runs after tests and picks up the generated coverage data.
- Around line 50-53: Remove the redundant maven.compiler.source and
maven.compiler.target properties and keep only maven.compiler.release (set to
${java.version}) so the compiler plugin uses the release flag to control
source/target and API surface; locate the properties block containing
<java.version>, <maven.compiler.source>, <maven.compiler.target>, and
<maven.compiler.release> and delete the two legacy properties while leaving
<java.version> and <maven.compiler.release> intact.
🪄 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
Run ID: ae143da2-9c5d-4778-bc31-fee4da95e8f0
📒 Files selected for processing (32)
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/labeler.yml.github/pull_request_template.md.github/release-drafter.yml.github/workflows/changelog-release-drafter.yml.github/workflows/codeql-analysis.yml.github/workflows/labeler.yml.github/workflows/maven.yml.github/workflows/release.yml.github/workflows/required-labels.ymljson-nonblocking/pom.xmljson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/JsonFactory.javajson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/NonBlockingJsonParser.javajson-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/package-info.javajson-nonblocking/src/main/java/reactivejson/NonBlockingObjectReader.javajson-nonblocking/src/main/java/reactivejson/Tokenizer.javajson-nonblocking/src/test/java/reactivejson/NonBlockingObjectReaderTest.javajson-nonblocking/src/test/java/reactivejson/TokenizerTest.javajson-reactor/pom.xmljson-reactor/src/main/java/reactivejson/ReactorObjectReader.javajson-reactor/src/test/java/reactivejson/ReactorObjectReaderTest.javajson-reactor/src/test/java/reactivejson/domain/TestEntity.javajson-rx2/pom.xmljson-rx2/src/main/java/reactivejson/Rx2ObjectReader.javajson-rx2/src/test/java/reactivejson/Rx2ObjectReaderTest.javajson-rx2/src/test/java/reactivejson/domain/TestEntity.javajson-rx3/pom.xmljson-rx3/src/main/java/reactivejson/Rx3ObjectReader.javajson-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.javajson-rx3/src/test/java/reactivejson/domain/TestEntity.javapom.xml
💤 Files with no reviewable changes (2)
- json-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/package-info.java
- json-nonblocking/src/main/java/com/fasterxml/jackson/core/async_/JsonFactory.java
✅ Files skipped from review due to trivial changes (12)
- .github/ISSUE_TEMPLATE/bug_report.md
- .github/workflows/labeler.yml
- .github/ISSUE_TEMPLATE/feature_request.md
- .github/labeler.yml
- .github/workflows/required-labels.yml
- json-reactor/pom.xml
- .github/release-drafter.yml
- .github/workflows/changelog-release-drafter.yml
- .github/workflows/codeql-analysis.yml
- json-rx3/src/test/java/reactivejson/domain/TestEntity.java
- json-rx3/pom.xml
- .github/workflows/maven.yml
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/pull_request_template.md
- json-rx2/pom.xml
- .github/workflows/release.yml
- json-reactor/src/test/java/reactivejson/domain/TestEntity.java
- json-rx2/src/test/java/reactivejson/domain/TestEntity.java
- json-nonblocking/pom.xml
- json-rx3/src/main/java/reactivejson/Rx3ObjectReader.java
- json-rx2/src/main/java/reactivejson/Rx2ObjectReader.java
- json-rx3/src/test/java/reactivejson/Rx3ObjectReaderTest.java
- json-reactor/src/main/java/reactivejson/ReactorObjectReader.java
- json-nonblocking/src/test/java/reactivejson/TokenizerTest.java
- json-rx2/src/test/java/reactivejson/Rx2ObjectReaderTest.java
- json-reactor/src/test/java/reactivejson/ReactorObjectReaderTest.java
Periecle
force-pushed
the
feature/jackson-3
branch
from
d824490 to
90f5704
Compare
Periecle
force-pushed
the
feature/jackson-3
branch
from
90f5704 to
bf7a7f8
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
2 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
Breaking Changes
Chores