Skip to content

AVRO-3285: Upgrade JavaCC plugin and library #1447

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

Merged
martin-g merged 1 commit into from
Jan 13, 2022
Merged

AVRO-3285: Upgrade JavaCC plugin and library #1447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions lang/java/compiler/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
the License. You may obtain a copy of the License at:
https://www.apache.org/licenses/LICENSE-2.0
Expand All @@ -24,7 +24,7 @@
<artifactId>avro-parent</artifactId>
<groupId>org.apache.avro</groupId>
<version>1.12.0-SNAPSHOT</version>
<relativePath>../</relativePath>
<relativePath>../pom.xml</relativePath>
Copy link
Contributor Author

@opwvhk opwvhk Jan 9, 2022

Choose a reason for hiding this comment

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

Minor really, but the relative path should point to a pom.

</parent>

<artifactId>avro-compiler</artifactId>
Expand Down Expand Up @@ -93,7 +93,7 @@
and outputs to target/generated-sources/javacc See http://mojo.codehaus.org/javacc-maven-plugin/javacc-mojo.html
for more info on using this plugin. -->
<plugin>
<groupId>org.codehaus.mojo</groupId>
<groupId>org.javacc.plugin</groupId>
<artifactId>javacc-maven-plugin</artifactId>
<executions>
<execution>
Expand Down Expand Up @@ -133,11 +133,11 @@
<classpathScope>test</classpathScope>
<arguments>
<argument>-classpath</argument>
<classpath></classpath>
<classpath/>
<argument>org.apache.avro.compiler.specific.SchemaTask</argument>
<argument>${project.basedir}/src/test/resources/full_record_v1.avsc</argument>
<argument>${project.basedir}/src/test/resources/full_record_v2.avsc</argument>
<argument>${project.basedir}/target/generated-test-sources</argument>
<argument>${project.basedir}/target/generated-test-sources/javacc</argument>
Copy link
Contributor Author

@opwvhk opwvhk Jan 9, 2022

Choose a reason for hiding this comment

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

Generated code should be placed in a subdirectory per tool. I copied the directory name from line 178.

</arguments>
</configuration>
</execution>
Expand All @@ -149,10 +149,8 @@
<executions>
<execution>
<!--
Usually code is generated using a special-purpose maven plugin and the plugin
automatically adds the generated sources into project.
Here since general-purpose exec plugin is used for generating code, we need to manually
add the sources.
Special-purpose maven plugins that generate code usually add the generated sources into the project.
But because the general-purpose exec plugin above generated code, we need to manually add the generated sources.
-->
<id>add-test-source</id>
<phase>generate-test-sources</phase>
Expand All @@ -161,16 +159,14 @@
</goals>
<configuration>
<sources>
<source>${project.basedir}/target/generated-test-sources</source>
<source>${project.basedir}/target/generated-test-sources/javacc</source>
Copy link
Contributor Author

@opwvhk opwvhk Jan 9, 2022

Choose a reason for hiding this comment

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

Mirror the change in line 140 to maintain a working build.

</sources>
</configuration>
</execution>
<execution>
<!--
Usually code is generated using a special-purpose maven plugin and the plugin
automatically adds the generated sources into project.
Here since general-purpose exec plugin is used for generating code, we need to manually
add the sources.
Special-purpose maven plugins that generate code usually add the generated sources into the project.
But because the general-purpose exec plugin above generated code, we need to manually add the generated sources.
-->
<id>add-source</id>
<phase>generate-sources</phase>
Expand Down Expand Up @@ -225,8 +221,8 @@
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>${commons-lang.version}</version>
<artifactId>commons-text</artifactId>
<version>${commons-text.version}</version>
</dependency>
<dependency>
<groupId>org.apache.velocity</groupId>
Expand Down
83 changes: 31 additions & 52 deletions lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ import org.apache.avro.util.internal.Accessor;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.*;

import org.apache.commons.lang3.StringEscapeUtils;
import org.apache.commons.text.StringEscapeUtils;
Copy link
Contributor Author

@opwvhk opwvhk Jan 9, 2022

Choose a reason for hiding this comment

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

org.apache.commons.lang3.StringEscapeUtils is deprecated in favor of org.apache.commons.text.StringEscapeUtils.


/**
* Grammar to parse a higher-level language into an Avro Schema.
Expand All @@ -96,7 +96,7 @@ public class Idl implements Closeable {
URI inputDir;
ClassLoader resourceLoader = null;
String namespace;
Map<String,Schema> names = new LinkedHashMap<String,Schema>();
Map<String,Schema> names = new LinkedHashMap<>();

private List<String> parserWarnings = Collections.emptyList();
/**
Expand Down Expand Up @@ -137,6 +137,7 @@ public class Idl implements Closeable {
this.resourceLoader = parent.resourceLoader;
}

@SuppressWarnings("RedundantThrows")
Copy link
Contributor Author

@opwvhk opwvhk Jan 9, 2022

Choose a reason for hiding this comment

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

A bit silly, but this is the one thing I cannot fix.

public void close() throws IOException {
jj_input_stream.inputStream.close();
}
Expand All @@ -159,7 +160,7 @@ public class Idl implements Closeable {
JsonNode value = props.get(key);
if (!value.isArray())
throw error(key+" property must be array: "+value, token);
List<String> values = new ArrayList<String>();
List<String> values = new ArrayList<>();
for (JsonNode n : value)
if (n.isTextual())
values.add(n.textValue());
Expand Down Expand Up @@ -209,7 +210,6 @@ public class Idl implements Closeable {
}
return schema;
}

}

PARSER_END(Idl)
Expand Down Expand Up @@ -248,13 +248,13 @@ MORE :
<DOC_COMMENT>
SPECIAL_TOKEN :
{
<"*/" > {DocCommentHelper.setDoc(matchedToken);} : DEFAULT
"*/" {DocCommentHelper.setDoc(matchedToken);} : DEFAULT
}

<MULTI_LINE_COMMENT>
SKIP :
{
<"*/" > : DEFAULT
"*/" : DEFAULT
}

/* RESERVED WORDS AND LITERALS */
Expand Down Expand Up @@ -1053,7 +1053,7 @@ Protocol CompilationUnit():
}
{
p = ProtocolDeclaration()
( < "\u001a" > )?
( "\u001a" )?
( <STUFF_TO_IGNORE: ~[]> )?
<EOF>
{
Expand Down Expand Up @@ -1102,7 +1102,7 @@ private Schema NamedSchemaDeclaration(Map<String, JsonNode> props):
Schema UnionDefinition():
{
Schema s;
List<Schema> schemata = new ArrayList<Schema>();
List<Schema> schemata = new ArrayList<>();
}
{
// Don't disallow unions here: its constructor disallows nested unions and throws a descriptive exception.
Expand All @@ -1127,7 +1127,7 @@ Protocol ProtocolDeclaration():
{
String name;
Protocol p;
Map<String, JsonNode> props = new LinkedHashMap<String, JsonNode>();
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
( SchemaProperty(props) )*
Expand Down Expand Up @@ -1164,16 +1164,15 @@ Schema EnumDeclaration():
symbols = EnumBody()
[ <EQUALS> defaultSymbol=Identifier() <SEMICOLON>]
{
Schema s = Schema.createEnum(name, doc, this.namespace, symbols,
defaultSymbol);
Schema s = Schema.createEnum(name, doc, namespace, symbols, defaultSymbol);
names.put(s.getFullName(), s);
return s;
}
}

List<String> EnumBody():
{
List<String> symbols = new ArrayList<String>();
List<String> symbols = new ArrayList<>();
}
{
"{"
Expand All @@ -1197,7 +1196,7 @@ void ProtocolBody(Protocol p):
Schema schema;
Message message;
Protocol importProtocol;
Map<String, JsonNode> props = new LinkedHashMap<String, JsonNode>();
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
"{"
Expand Down Expand Up @@ -1234,13 +1233,8 @@ Protocol ImportIdl() : {
{
<IDL> importFile = JsonString() ";"
{
try {
Idl idl = new Idl(findFile(importFile), this);
try {
return idl.CompilationUnit();
} finally {
idl.close();
}
try (Idl idl=new Idl(findFile(importFile), this)){
return idl.CompilationUnit();
} catch (IOException e) {
throw error("Error importing "+importFile+": "+e, token);
}
Expand All @@ -1253,14 +1247,8 @@ Protocol ImportProtocol() : {
{
<PROTOCOL> importFile = JsonString() ";"
{

try {
InputStream stream = findFile(importFile).openStream();
try {
return Protocol.parse(stream);
} finally {
stream.close();
}
try (InputStream stream=findFile(importFile).openStream()) {
return Protocol.parse(stream);
} catch (IOException e) {
throw error("Error importing "+importFile+": "+e, token);
}
Expand All @@ -1273,17 +1261,12 @@ Schema ImportSchema() : {
{
<SCHEMA> importFile = JsonString() ";"
{
try {
try (InputStream stream=findFile(importFile).openStream()){
Parser parser = new Schema.Parser();
parser.addTypes(names); // inherit names
InputStream stream = findFile(importFile).openStream();
try {
Schema value = parser.parse(stream);
names = parser.getTypes(); // update names
return value;
} finally {
stream.close();
}
Schema value = parser.parse(stream);
names = parser.getTypes(); // update names
return value;
} catch (IOException e) {
throw error("Error importing "+importFile+": "+e, token);
}
Expand All @@ -1309,7 +1292,7 @@ Schema FixedDeclaration():
Schema RecordDeclaration():
{
String name;
List<Field> fields = new ArrayList<Field>();
List<Field> fields = new ArrayList<>();
boolean isError;
}
{
Expand Down Expand Up @@ -1361,7 +1344,7 @@ void VariableDeclarator(Schema type, List<Field> fields):
{
String name;
JsonNode defaultValue = null;
Map<String, JsonNode> props = new LinkedHashMap<String, JsonNode>();
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
( SchemaProperty(props) )*
Expand Down Expand Up @@ -1408,7 +1391,7 @@ private Message MessageDeclaration(Protocol p, Map<String, JsonNode> props):
Schema request;
Schema response;
boolean oneWay = false;
List<Schema> errorSchemata = new ArrayList<Schema>();
List<Schema> errorSchemata = new ArrayList<>();
errorSchemata.add(Protocol.SYSTEM_ERROR);
}
{
Expand Down Expand Up @@ -1440,14 +1423,12 @@ void ErrorList(List<Schema> errors):

Schema FormalParameters():
{
List<Field> fields = new ArrayList<Field>();
List<Field> fields = new ArrayList<>();
}
{
(
"(" [ FormalParameter(fields) ( "," FormalParameter(fields) )* ] ")"
)
"(" [ FormalParameter(fields) ( "," FormalParameter(fields) )* ] ")"
{
return Schema.createRecord(fields);
return Schema.createRecord(null, null, null, false, fields);
}
}

Expand All @@ -1463,7 +1444,7 @@ void FormalParameter(List<Field> fields):
Schema Type():
{
Schema s;
Map<String, JsonNode> props = new LinkedHashMap<String, JsonNode>();
Map<String, JsonNode> props = new LinkedHashMap<>();
}
{
( SchemaProperty(props) )*
Expand Down Expand Up @@ -1557,10 +1538,8 @@ Schema ReferenceType():
StringBuilder sb = new StringBuilder();
}
{
(
part = Identifier() { sb.append(part); }
("." tok = AnyIdentifier() { sb.append(".").append(tok.image); })*
)
part = Identifier() { sb.append(part); }
("." tok = AnyIdentifier() { sb.append(".").append(tok.image); })*
{
String name = sb.toString();
if ((name.indexOf('.') == -1) && namespace != null)
Expand All @@ -1575,7 +1554,7 @@ Schema ReferenceType():
}

Schema PrimitiveType():
{}
{ Schema s; }
{
"boolean" { return Schema.create(Type.BOOLEAN); }
| "bytes" { return Schema.create(Type.BYTES); }
Expand All @@ -1589,7 +1568,7 @@ Schema PrimitiveType():
| "time_ms" { return LogicalTypes.timeMillis().addToSchema(Schema.create(Type.INT)); }
| "timestamp_ms" { return LogicalTypes.timestampMillis().addToSchema(Schema.create(Type.LONG)); }
| "local_timestamp_ms" { return LogicalTypes.localTimestampMillis().addToSchema(Schema.create(Type.LONG)); }
| "decimal" { return DecimalTypeProperties(); }
| "decimal" s = DecimalTypeProperties() { return s; }
Copy link
Member

@martin-g martin-g Jan 10, 2022

Choose a reason for hiding this comment

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

what is the benefit of this change ?

Copy link
Member

@martin-g martin-g Jan 13, 2022

Choose a reason for hiding this comment

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

@opwvhk Ping!

Copy link
Contributor Author

@opwvhk opwvhk Jan 13, 2022

Choose a reason for hiding this comment

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

Nothing directly, but the rule DecimalTypeProperties no longer shows up as 'unused in IntelliJ.

| "uuid" {return LogicalTypes.uuid().addToSchema(Schema.create(Type.STRING));}
}

Expand Down
16 changes: 12 additions & 4 deletions lang/java/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<groupId>org.apache.avro</groupId>
<artifactId>avro-toplevel</artifactId>
<version>1.12.0-SNAPSHOT</version>
<relativePath>../../</relativePath>
<relativePath>../../pom.xml</relativePath>
</parent>

<artifactId>avro-parent</artifactId>
Expand Down Expand Up @@ -53,7 +53,7 @@
<ant.version>1.10.11</ant.version>
<commons-cli.version>1.4</commons-cli.version>
<commons-compress.version>1.21</commons-compress.version>
<commons-lang.version>3.12.0</commons-lang.version>
<commons-text.version>1.9</commons-text.version>
<tukaani.version>1.9</tukaani.version>
<easymock.version>4.3</easymock.version>
<hamcrest.version>2.2</hamcrest.version>
Expand All @@ -64,7 +64,8 @@
<bundle-plugin-version>5.1.2</bundle-plugin-version>
<exec-plugin.version>3.0.0</exec-plugin.version>
<file-management.version>3.0.0</file-management.version>
<javacc-plugin.version>2.6</javacc-plugin.version>
<javacc-plugin.version>3.0.3</javacc-plugin.version>
<javacc.version>7.0.10</javacc.version>
</properties>

<modules>
Expand Down Expand Up @@ -210,9 +211,16 @@
</configuration>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<groupId>org.javacc.plugin</groupId>
<artifactId>javacc-maven-plugin</artifactId>
<version>${javacc-plugin.version}</version>
<dependencies>
Copy link
Contributor Author

@opwvhk opwvhk Jan 9, 2022

Choose a reason for hiding this comment

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

The new plugin treats JavaCC as a provided dependency, so we need to add it explicitly.

<dependency>
<groupId>net.java.dev.javacc</groupId>
<artifactId>javacc</artifactId>
<version>${javacc.version}</version>
</dependency>
</dependencies>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
Expand Down