3525: AVRO-4189: [java] Simplify the setting of the serializable classes

[martin-augment]

3525: To review by AI

[claude]

PR Review: AVRO-4189

Summary

This PR introduces ClassSecurityValidator to centralize class security validation. Good refactoring with backward compatibility maintained.

High Priority Issues

1. Thread Safety (ClassSecurityValidator.java:70)

setGlobal() modifies static variable without synchronization - potential race condition.
Recommendation: Use AtomicReference instead.

2. Package Matching Bug (ClassSecurityValidator.java:250-251)

False positives possible: "com.example." would match "com.example2.BadClass".
Need to verify match is at package boundary.

Medium Priority Issues

• Add warning log when wildcard package trust is enabled
• Clean up mixed JUnit 4/5 imports in TestSpecificRecordWithUnion
• Consider String.strip() instead of regex for trimming (line 231)

Strengths

• Well-structured Strategy pattern with ClassSecurityPredicate
• Excellent test coverage with edge cases
• Classes validated before initialization (security+performance)
• NavigableSet.lower() is efficient O(log n)
• Good JavaDoc documentation

Security Notes

• Classes validated before initialization prevents malicious static blocks
• Array and primitive handling prevents bypass attempts
• Wildcard mode needs runtime warning
• Consider security implications of trusting java.util collection classes in IT tests

Test Coverage

Missing: Concurrent access test for setGlobal()/getGlobal()

Recommendation

Solid refactoring but needs fixes for thread safety and package matching before merge.

---

Generated with Claude Code

[augmentcode]

Review completed. 2 suggestions posted.

[augmentcode]

Consider using JUnit 5’s Assertions.assertThrows (org.junit.jupiter.api.Assertions.assertThrows) instead of org.junit.Assert.assertThrows to align with the JUnit Jupiter @Test and avoid requiring JUnit 4.13 on the classpath.

🤖 React with 👍 or 👎 to let us know if the comment was useful.

[martin-augment]

value:valid-but-wont-fix; category:typo; feedback:using JUnit 5.x only APIs would be preferred but the old tests already use the JUnit 4.x assertEquals method in several places. The migration to JUnit 5.x could be done in a separate commit.

[augmentcode]

The comment "add the hard-coded trusted classes as well" appears misleading here: the defaults aren’t added to SERIALIZABLE_CLASSES, they’re combined via DEFAULT = composite(DEFAULT_TRUSTED_CLASSES, SYSTEM_PROPERTIES). Consider updating the comment to reflect that.

🤖 React with 👍 or 👎 to let us know if the comment was useful.

[martin-augment]

value:good-to-have; category:documentation; feedback:The comment is indeed wrong. The hardcoded classes are defined with DEFAULT_TRUSTED_CLASSES at line 117. The comment is about the extra classes provided with system properties.

[github-actions]

• .github/workflows/claude.yml:1 – Deleting this (and the other workflow files touched in the PR) would drop several required CI/review checks; it looks like the branch was cut from an older snapshot and needs rebasing to pick up the latest automation.
• doc/content/en/blog/releases/avro-1.12.1-released.md:1 – The release notes added on main vanish under this change; please rebase so recent docs stay intact.
• lang/java/pom.xml:44 – All of the dependency properties here get rolled back (e.g., commons-lang3 3.19.0 → 3.18.0, grpc 1.76.0 → 1.75.0, netty 4.2.7 → 4.2.4, protobuf 4.33.0 → 4.32.0, zstd-jni 1.5.7-5 → 1.5.7-4, exec-plugin 3.6.2 → 3.5.1); these regressions will erase the recent upgrade work.
• pom.xml:51 – Same issue for the build plugins (checkstyle 12.1.0 → 11.0.1, enforcer 3.6.2 → 3.6.1, extra-enforcer-rules 1.11.0 → 1.10.0, maven-compiler 3.14.1 → 3.14.0, maven-javadoc 3.12.0 → 3.11.2, maven-shade 3.6.1 → 3.6.0); we can’t merge while these downgrades are in place.
• lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java:138 – getTrustedPackages()/getTrustedClasses() now return Arrays.asList(...), so existing callers that still mutate those deprecated lists (e.g., getTrustedPackages().add(...)) will start throwing UnsupportedOperationException; that’s a behavioral regression.

These issues block merging; once the branch is rebased and the unintended regressions are cleared up we can take another look at the functional changes.