Skip to content

AVRO-4313: [java] Clarify javaAnnotation validation comments - #3907

Merged
RyanSkraba merged 1 commit into
apache:mainfrom
iemejia:AVRO-4313-clarify-comments
Aug 2, 2026
Merged

AVRO-4313: [java] Clarify javaAnnotation validation comments#3907
RyanSkraba merged 1 commit into
apache:mainfrom
iemejia:AVRO-4313-clarify-comments

Conversation

@iemejia

@iemejia iemejia commented Jul 31, 2026

Copy link
Copy Markdown
Member

What is the purpose of the change

Follow-up to AVRO-4313 / #3892. That fix tightened the regex that validates a
schema's javaAnnotation before it is copied verbatim into generated Java
source. The surrounding comments (and the regression test comments) were hard
to follow, so this rewrites them in plainer language.

The comments now explain:

  • why the check exists — the javaAnnotation property is emitted verbatim
    into generated source, so it must be validated first;
  • what an unescaped quote in a string literal would allow — a single literal
    running past its closing quote and swallowing injected code;
  • how the regression test distinguishes the safely-escaped copy of the
    payload inside the SCHEMA$ constant from a real verbatim emission.

This is a comment-only change; no behavior change.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.
The existing annotationCannotBreakOutViaStringLiteral and the rest of the
compiler module suite continue to pass.

Documentation

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

@github-actions github-actions Bot added the Java Pull Requests for Java binding label Jul 31, 2026
@iemejia
iemejia requested review from RyanSkraba and Copilot July 31, 2026 19:49
Rewrite the comments around the annotation-validation grammar and its
regression test in plainer language. Explain why the check exists (the
javaAnnotation property is emitted verbatim into generated source) and
what an unescaped quote in a string literal would allow, so the intent is
clear to readers who are not familiar with the regex.

Comment-only change; no behavior change.
@iemejia
iemejia force-pushed the AVRO-4313-clarify-comments branch from 0cb972b to 2e5bf47 Compare July 31, 2026 19:50

Copilot AI left a comment

Copy link
Copy Markdown

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 clarifies the security rationale and mechanics behind javaAnnotation validation in the Java SpecificCompiler, and improves the readability of the associated regression test commentary (AVRO-4313 follow-up). The changes are comment-only and do not alter behavior.

Changes:

  • Rewrites the annotationCannotBreakOutViaStringLiteral test comments to more plainly explain the injection scenario and why the assertion checks for unescaped quotes in generated code.
  • Adds a clearer, higher-level comment block in SpecificCompiler describing why javaAnnotation is validated and what the regex sub-patterns are intended to prevent.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java Clarifies the regression test’s intent and how it distinguishes safe schema-string embedding from unsafe verbatim code emission.
lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java Adds clearer documentation for the javaAnnotation validation grammar and the string-literal constraints that prevent code injection.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@RyanSkraba

Copy link
Copy Markdown
Contributor

Thanks -- it's important for these complicated PRs!

@RyanSkraba
RyanSkraba merged commit beb7662 into apache:main Aug 2, 2026
9 checks passed
@RyanSkraba

Copy link
Copy Markdown
Contributor

Cherry-picked to branch-1.12.

RyanSkraba pushed a commit that referenced this pull request Aug 2, 2026
Rewrite the comments around the annotation-validation grammar and its
regression test in plainer language. Explain why the check exists (the
javaAnnotation property is emitted verbatim into generated source) and
what an unescaped quote in a string literal would allow, so the intent is
clear to readers who are not familiar with the regex.

Comment-only change; no behavior change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java Pull Requests for Java binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants