Skip to content

fix(java): preserve externalizable containers in compatible mode#3628

Merged
chaokunyang merged 2 commits into
apache:mainfrom
mandrean:mandrean/fix-3621-compatible-map
Apr 28, 2026
Merged

fix(java): preserve externalizable containers in compatible mode#3628
chaokunyang merged 2 commits into
apache:mainfrom
mandrean:mandrean/fix-3621-compatible-map

Conversation

@mandrean
Copy link
Copy Markdown
Contributor

@mandrean mandrean commented Apr 28, 2026

Why?

Fory can lose entries for compatible-mode map and collection classes that implement Externalizable if they are handled as ordinary containers instead of using their externalization logic.

This fixes the data loss reported in #3621.

What does this PR do?

  • Adds a regression test for an Externalizable AbstractMap stored through a Map<String, String> field.
  • Routes Externalizable map and collection implementations through the JDK-compatible serializer path.
  • Delegates those containers to ExternalizableSerializer so writeExternal / readExternal preserve their state.

Related issues

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: yes
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes.

AI Usage Disclosure

  • substantial_ai_assistance: yes
  • scope: issue investigation, regression test drafting, code change drafting, validation command selection, PR description drafting
  • affected_files_or_subsystems: Java serializer resolution, compatible collection/map serializers, Java map serializer tests
  • ai_review: line-by-line self-review completed locally; required two-fresh-reviewer loop is pending, so this PR remains draft until final clean review evidence can be attached
  • ai_review_artifacts: pending; screenshot evidence or persisted links for both fresh final clean AI reviews will be added before marking ready for maintainer review
  • human_verification:
    • Reproduced the regression by applying the new test on apache/main, where it fails with Maps do not have the same size:0 != 1.
    • Verified the fixed branch with mvn -pl fory-core -Dtest=org.apache.fory.serializer.collection.MapSerializersTest#testExternalizableMapCompatibleMode test.
    • Ran Java/style/install checks: mvn -T10 -B --no-transfer-progress spotless:check checkstyle:check, python ./ci/run_ci.py java --version 11, python ./ci/run_ci.py java --version 17, python ./ci/run_ci.py java --version 21, python ./ci/run_ci.py java --version 25, python ./ci/run_ci.py java --install-fory, and mvn -T10 --no-transfer-progress clean install -DskipTests -Dmaven.javadoc.skip=true -Dmaven.source.skip=true.
  • performance_verification: N/A; correctness fix for Externalizable container serializer selection in compatible mode, no benchmark run
  • provenance_license_confirmation: Apache-2.0-compatible provenance confirmed; no incompatible third-party code introduced

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

Not run. This is a correctness fix for existing compatible-mode Externalizable map/collection handling and does not intentionally change public APIs or binary protocol format.

@mandrean mandrean force-pushed the mandrean/fix-3621-compatible-map branch from 373ec21 to 54754d6 Compare April 28, 2026 12:10
@mandrean mandrean force-pushed the mandrean/fix-3621-compatible-map branch from 54754d6 to 39ef80e Compare April 28, 2026 12:40
@mandrean mandrean marked this pull request as ready for review April 28, 2026 12:56
@mandrean mandrean requested a review from chaokunyang as a code owner April 28, 2026 12:56
Copy link
Copy Markdown
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chaokunyang chaokunyang merged commit fdd1143 into apache:main Apr 28, 2026
123 of 124 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COMPATIBLE mode silently drops contents of unregistered Map/Collection implementations

2 participants