Skip to content

Commit

Permalink
AVRO-4053: doc consistency in velocity templates (#3150)
Browse files Browse the repository at this point in the history 
* AVRO-4053: Improve consistency in javadoc comments

* AVRO-4053: Add test case

* AVRO-4053: rename test

* AVRO-4053: Fix omission

* AVRO-4053: spotless

* AVRO-4053: Restore old name in public API

* AVRO-4053: Static imports on top
  • Loading branch information
@opwvhk
opwvhk authored Oct 4, 2024
1 parent 8040078 commit 84bc732
Show file tree
Hide file tree
Showing 8 changed files with 148 additions and 73 deletions.
85 changes: 56 additions & 29 deletions lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,7 @@
*/
package org.apache.avro.compiler.specific;

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.StringWriter;
import java.io.Writer;
import java.lang.reflect.InvocationTargetException;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import static java.nio.charset.StandardCharsets.UTF_8;

import org.apache.avro.Conversion;
import org.apache.avro.Conversions;
Expand All @@ -60,7 +40,28 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import static java.nio.charset.StandardCharsets.UTF_8;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStreamWriter;
import java.io.StringWriter;
import java.io.Writer;
import java.lang.reflect.InvocationTargetException;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

/**
* Generate specific Java interfaces and classes for protocols and schemas.
Expand Down Expand Up @@ -1004,27 +1005,43 @@ public String conversionInstance(Schema schema) {
*/
public String[] javaAnnotations(JsonProperties props) {
final Object value = props.getObjectProp("javaAnnotation");
if (value == null)
return new String[0];
if (value instanceof String)
if (value instanceof String && isValidAsAnnotation((String) value))
return new String[] { value.toString() };
if (value instanceof List) {
final List<?> list = (List<?>) value;
final List<String> annots = new ArrayList<>(list.size());
for (Object o : list) {
annots.add(o.toString());
if (isValidAsAnnotation(o.toString()))
annots.add(o.toString());
}
return annots.toArray(new String[0]);
}
return new String[0];
}

private static final String PATTERN_IDENTIFIER_PART = "\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*";
private static final String PATTERN_IDENTIFIER = String.format("(?:%s(?:\\.%s)*)", PATTERN_IDENTIFIER_PART,
PATTERN_IDENTIFIER_PART);
private static final String PATTERN_STRING = "\"(?:\\\\[\\\\\"ntfb]|(?<!\\\\).)*\"";
private static final String PATTERN_NUMBER = "(?:\\((?:byte|char|short|int|long|float|double)\\))?[x0-9_.]*[fl]?";
private static final String PATTERN_LITERAL_VALUE = String.format("(?:%s|%s|true|false)", PATTERN_STRING,
PATTERN_NUMBER);
private static final String PATTERN_PARAMETER_LIST = String.format(
"\\(\\s*(?:%s|%s\\s*=\\s*%s(?:\\s*,\\s*%s\\s*=\\s*%s)*)?\\s*\\)", PATTERN_LITERAL_VALUE, PATTERN_IDENTIFIER,
PATTERN_LITERAL_VALUE, PATTERN_IDENTIFIER, PATTERN_LITERAL_VALUE);
private static final Pattern VALID_AS_ANNOTATION = Pattern
.compile(String.format("%s(?:%s)?", PATTERN_IDENTIFIER, PATTERN_PARAMETER_LIST));

private boolean isValidAsAnnotation(String value) {
return VALID_AS_ANNOTATION.matcher(value.strip()).matches();
}

// maximum size for string constants, to avoid javac limits
int maxStringChars = 8192;

/**
* Utility for template use. Takes a (potentially overly long) string and splits
* it into a quoted, comma-separted sequence of escaped strings.
* it into a quoted, comma-separated sequence of escaped strings.
*
* @param s The string to split
* @return A sequence of quoted, comma-separated, escaped strings
Expand All @@ -1036,7 +1053,7 @@ public String javaSplit(String s) throws IOException {
if (i != 0)
b.append("\",\""); // insert quote-comma-quote
String chunk = s.substring(i, Math.min(s.length(), i + maxStringChars));
b.append(javaEscape(chunk)); // escape chunks
b.append(escapeForJavaString(chunk)); // escape chunks
}
b.append("\""); // final quote
return b.toString();
Expand All @@ -1045,10 +1062,20 @@ public String javaSplit(String s) throws IOException {
/**
* Utility for template use. Escapes quotes and backslashes.
*/
public static String javaEscape(String o) {
public static String escapeForJavaString(String o) {
return o.replace("\\", "\\\\").replace("\"", "\\\"");
}

/**
* Utility for template use (previous name). Escapes quotes and backslashes.
*
* @deprecated Use {@link #escapeForJavaString(String)} instead
*/
@Deprecated(since = "1.12.1", forRemoval = true)
public static String javaEscape(String o) {
return escapeForJavaString(o);
}

/**
* Utility for template use. Escapes comment end with HTML entities.
*/
Expand Down
4 changes: 2 additions & 2 deletions ...mpiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/enum.vm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
package $this.mangle($schema.getNamespace());
#end
#if ($schema.getDoc())
/** $schema.getDoc() */
/** $this.escapeForJavadoc($schema.getDoc()) */
#end
#foreach ($annotation in $this.javaAnnotations($schema))
@$annotation
Expand All @@ -28,7 +28,7 @@ package $this.mangle($schema.getNamespace());
public enum ${this.mangleTypeIdentifier($schema.getName())} implements org.apache.avro.generic.GenericEnumSymbol<${this.mangleTypeIdentifier($schema.getName())}> {
#foreach ($symbol in ${schema.getEnumSymbols()})${this.mangle($symbol)}#if ($foreach.hasNext), #end#end
;
public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.javaEscape($schema.toString())}");
public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.escapeForJavaString($schema.toString())}");
public static org.apache.avro.Schema getClassSchema() { return SCHEMA$; }

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...piler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/fixed.vm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
package $this.mangle($schema.getNamespace());
#end
#if ($schema.getDoc())
/** $schema.getDoc() */
/** $this.escapeForJavadoc($schema.getDoc()) */
#end
#foreach ($annotation in $this.javaAnnotations($schema))
@$annotation
Expand All @@ -28,7 +28,7 @@ package $this.mangle($schema.getNamespace());
@org.apache.avro.specific.AvroGenerated
public class ${this.mangleTypeIdentifier($schema.getName())} extends org.apache.avro.specific.SpecificFixed {
private static final long serialVersionUID = ${this.fingerprint64($schema)}L;
public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.javaEscape($schema.toString())}");
public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.escapeForJavaString($schema.toString())}");
public static org.apache.avro.Schema getClassSchema() { return SCHEMA$; }
public org.apache.avro.Schema getSchema() { return SCHEMA$; }

Expand Down
8 changes: 4 additions & 4 deletions ...er/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/protocol.vm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ package $this.mangle($protocol.getNamespace());
#end

#if ($protocol.getDoc())
/** $protocol.getDoc() */
/** $this.escapeForJavadoc($protocol.getDoc()) */
#end
#foreach ($annotation in $this.javaAnnotations($protocol))
@$annotation
Expand All @@ -37,7 +37,7 @@ public interface $this.mangleTypeIdentifier($protocol.getName()) {
* $this.escapeForJavadoc($message.getDoc())
#end
#foreach ($p in $message.getRequest().getFields())##
#if ($p.doc()) * @param ${this.mangle($p.name())} $p.doc()
#if ($p.doc()) * @param ${this.mangle($p.name())} $this.escapeForJavadoc($p.doc())
#end
#end
*/
Expand All @@ -62,7 +62,7 @@ ${this.mangle($error.getFullName())}##

## Generate nested callback API
#if ($protocol.getDoc())
/** $protocol.getDoc() */
/** $this.escapeForJavadoc($protocol.getDoc()) */
#end
@org.apache.avro.specific.AvroGenerated
public interface Callback extends $this.mangleTypeIdentifier($protocol.getName()) {
Expand All @@ -78,7 +78,7 @@ ${this.mangle($error.getFullName())}##
* $this.escapeForJavadoc($message.getDoc())
#end
#foreach ($p in $message.getRequest().getFields())##
#if ($p.doc()) * @param ${this.mangle($p.name())} $p.doc()
#if ($p.doc()) * @param ${this.mangle($p.name())} $this.escapeForJavadoc($p.doc())
#end
#end
* @throws java.io.IOException The async call could not be completed.
Expand Down
32 changes: 16 additions & 16 deletions ...iler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import org.apache.avro.message.SchemaStore;
#if (${this.gettersReturnOptional} || ${this.createOptionalGetters})import java.util.Optional;#end

#if ($schema.getDoc())
/** $schema.getDoc() */
/** $this.escapeForJavadoc($schema.getDoc()) */
#end
#foreach ($annotation in $this.javaAnnotations($schema))
@$annotation
Expand Down Expand Up @@ -116,7 +116,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

#foreach ($field in $schema.getFields())
#if ($field.doc())
/** $field.doc() */
/** $this.escapeForJavadoc($field.doc()) */
#end
#foreach ($annotation in $this.javaAnnotations($field))
@$annotation
Expand Down Expand Up @@ -155,7 +155,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
/**
* All-args constructor.
#foreach ($field in $schema.getFields())
#if ($field.doc()) * @param ${this.mangle($field.name())} $field.doc()
#if ($field.doc()) * @param ${this.mangle($field.name())} $this.escapeForJavadoc($field.doc())
#else * @param ${this.mangle($field.name())} The new value for ${field.name()}
#end
#end
Expand Down Expand Up @@ -228,7 +228,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#if (${this.gettersReturnOptional} && (!${this.optionalGettersForNullableFieldsOnly} || ${field.schema().isNullable()}))
/**
* Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional&lt;${this.escapeForJavadoc(${this.javaType($field.schema())})}&gt;.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return The value wrapped in an Optional&lt;${this.escapeForJavadoc(${this.javaType($field.schema())})}&gt;.
*/
Expand All @@ -238,7 +238,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#else
/**
* Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
#if ($field.doc()) * @return $field.doc()
#if ($field.doc()) * @return $this.escapeForJavadoc($field.doc())
#else * @return The value of the '${this.mangle($field.name(), $schema.isError())}' field.
#end
*/
Expand All @@ -257,7 +257,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#if (${this.createOptionalGetters})
/**
* Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional&lt;${this.escapeForJavadoc(${this.javaType($field.schema())})}&gt;.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return The value wrapped in an Optional&lt;${this.escapeForJavadoc(${this.javaType($field.schema())})}&gt;.
*/
Expand All @@ -269,7 +269,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#if ($this.createSetters)
/**
* Sets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @param value the value to set.
*/
Expand Down Expand Up @@ -323,7 +323,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

#foreach ($field in $schema.getFields())
#if ($field.doc())
/** $field.doc() */
/** $this.escapeForJavadoc($field.doc()) */
#end
private ${this.javaUnbox($field.schema(), false)} ${this.mangle($field.name(), $schema.isError())};
#if (${this.hasBuilder($field.schema())})
Expand Down Expand Up @@ -402,7 +402,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#foreach ($field in $schema.getFields())
/**
* Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return The value.
*/
Expand All @@ -413,7 +413,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#if (${this.createOptionalGetters})
/**
* Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional&lt;${this.escapeForJavadoc(${this.javaType($field.schema())})}&gt;.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return The value wrapped in an Optional&lt;${this.escapeForJavadoc(${this.javaType($field.schema())})}&gt;.
*/
Expand All @@ -424,7 +424,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

/**
* Sets the value of the '${this.mangle($field.name(), $schema.isError())}' field.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @param value The value of '${this.mangle($field.name(), $schema.isError())}'.
* @return This builder.
Expand All @@ -441,7 +441,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

/**
* Checks whether the '${this.mangle($field.name(), $schema.isError())}' field has been set.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return True if the '${this.mangle($field.name(), $schema.isError())}' field has been set, false otherwise.
*/
Expand All @@ -452,7 +452,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS
#if (${this.hasBuilder($field.schema())})
/**
* Gets the Builder instance for the '${this.mangle($field.name(), $schema.isError())}' field and creates one if it doesn't exist yet.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return This builder.
*/
Expand All @@ -469,7 +469,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

/**
* Sets the Builder instance for the '${this.mangle($field.name(), $schema.isError())}' field
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @param value The builder instance that must be set.
* @return This builder.
Expand All @@ -483,7 +483,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

/**
* Checks whether the '${this.mangle($field.name(), $schema.isError())}' field has an active Builder instance
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return True if the '${this.mangle($field.name(), $schema.isError())}' field has an active Builder instance
*/
Expand All @@ -494,7 +494,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS

/**
* Clears the value of the '${this.mangle($field.name(), $schema.isError())}' field.
#if ($field.doc()) * $field.doc()
#if ($field.doc()) * $this.escapeForJavadoc($field.doc())
#end
* @return This builder.
*/
Expand Down
Loading

0 comments on commit 84bc732

Please sign in to comment.