Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
RyanSkraba merged 4 commits into from
Oct 22, 2025
Merged

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

RyanSkraba merged 4 commits into from
Oct 22, 2025

Conversation

@gszadovszky
Copy link
Contributor

@gszadovszky gszadovszky commented Oct 20, 2025

What is the purpose of the change

(For example: This pull request improves file read performance by buffering data, fixing AVRO-XXXX.)

Verifying this change

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Extended interop tests to verify consistent valid schema names between SDKs
  • Added test that validates that Java throws an AvroRuntimeException on invalid binary data
  • Manually verified the change by building the website and checking the new redirect

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added Java Pull Requests for Java binding build labels Oct 20, 2025
@martin-g martin-g requested a review from Copilot October 21, 2025 08:55
Copilot AI reviewed Oct 21, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the class security validation mechanism in Apache Avro by introducing a new ClassSecurityValidator utility that centralizes and simplifies how trusted classes and packages are configured. The change allows using package-level whitelisting instead of requiring explicit enumeration of every class, reducing configuration verbosity.

Key Changes:

  • Introduced ClassSecurityValidator class with a builder pattern and predicate-based validation
  • Added support for org.apache.avro.SERIALIZABLE_PACKAGES system property to whitelist entire packages
  • Migrated validation logic from SpecificDatumReader to the centralized ClassSecurityValidator

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lang/java/avro/src/main/java/org/apache/avro/util/ClassSecurityValidator.java New utility class implementing the centralized class security validation with builder pattern and system property support
lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java Updated to use the new ClassSecurityValidator and load classes without initialization for security validation
lang/java/avro/src/main/java/org/apache/avro/specific/SpecificDatumReader.java Refactored to delegate security validation to ClassSecurityValidator and deprecated old validation methods
lang/java/avro/src/test/java/org/apache/avro/util/TestClassSecurityValidator.java New test class covering various validation scenarios including builder, predicates, and composite validators
lang/java/avro/src/test/java/org/apache/avro/specific/TestSpecificRecordWithUnion.java Added test to verify SecurityException is thrown for untrusted classes
lang/java/avro/src/test/java/org/apache/avro/reflect/TestReflectDatumReader.java Added test to verify SecurityException is thrown for untrusted classes during reflection
lang/java/avro/pom.xml Updated system properties to use package whitelisting instead of individual class enumeration
lang/java/avro/src/it/pom.xml Updated system properties to use package whitelisting instead of individual class enumeration
lang/java/ipc/pom.xml Updated system properties to use package whitelisting instead of individual class enumeration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

RyanSkraba
RyanSkraba approved these changes Oct 22, 2025
View reviewed changes
Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

This LGTM -- I'm pretty confident in this implementation (and it has improved readability in what it's doing). We should definitely cherry-pick this to 1.11.x as well.

lang/java/avro/src/main/java/org/apache/avro/util/ClassUtils.java
}
if (c == null) {
throw new ClassNotFoundException("Failed to load class" + className);
throw new ClassNotFoundException("Failed to load class " + className);
Copy link
Contributor

@RyanSkraba RyanSkraba Oct 22, 2025

Choose a reason for hiding this comment

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

Nice catch!

@RyanSkraba RyanSkraba merged commit e0217ae into apache:main Oct 22, 2025
9 checks passed
RyanSkraba pushed a commit that referenced this pull request Oct 22, 2025
@gszadovszky @RyanSkraba
AVRO-4189: [java] Simplify the setting of the serializable classes (#…
3b764e1 
…3525)

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

* Fix missing license header

* Fix test failures + copilot findings

* Fix system property settings in pomx
@RyanSkraba
Copy link
Contributor

RyanSkraba commented Oct 22, 2025 •
edited
Loading

Cherry-picked to branch-1.12.

This was referenced Oct 23, 2025
Open
Open
gszadovszky added a commit that referenced this pull request Oct 27, 2025
@gszadovszky
AVRO-4189: [java] Simplify the setting of the serializable classes (#…
625395a 
…3525)

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

* Fix missing license header

* Fix test failures + copilot findings

* Fix system property settings in pomx

(cherry picked from commit e0217ae)
@gszadovszky
Copy link
Contributor Author

gszadovszky commented Oct 27, 2025

Cherry-picked to branch-1.11

@gszadovszky
Copy link
Contributor Author

gszadovszky commented Oct 27, 2025

@martin-g, thanks for pushing/cherry-picking my change.

I've cherry-picked to 1.11 as well. Then, I've realized, there is a branch called ci-for-branch-1.11. Not sure about its purpose. Shall I also cherry-pick it to there?

Also, may we have new releases with this change from 1.11 and 1.12?

@martin-g
Copy link
Member

martin-g commented Oct 27, 2025

Credits go to @RyanSkraba !

@opwvhk Which branch should be used for 1.11.x ?

@opwvhk
Copy link
Contributor

opwvhk commented Oct 27, 2025

I don't know about the ci- branch. The 1.11.x releases all came from branch-1.11.

@gszadovszky
Copy link
Contributor Author

gszadovszky commented Oct 27, 2025 •
edited
Loading

Sorry, @martin-g, not sure why I've pinged you about this...

Meanwhile, I've made a mistake during the backport. Could you check #3538, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@RyanSkraba RyanSkraba RyanSkraba approved these changes

@nandorKollar nandorKollar Awaiting requested review from nandorKollar

@martin-g martin-g Awaiting requested review from martin-g

Assignees

No one assigned

Labels

build Java Pull Requests for Java binding

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gszadovszky @RyanSkraba @martin-g @opwvhk