You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/07/12 12:34:58 UTC
[GitHub] [avro] clesaec opened a new pull request, #1768: Avro 530 def recursion
clesaec opened a new pull request, #1768:
URL: https://github.com/apache/avro/pull/1768
[AVRO-530](https://issues.apache.org/jira/browse/AVRO-530) : Types in protocol can be defined each other
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec merged pull request #1768: AVRO-530: Allow recursive types in protocol
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec merged PR #1768:
URL: https://github.com/apache/avro/pull/1768
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] github-code-scanning[bot] commented on a diff in pull request #1768: Avro 530 def recursion
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #1768:
URL: https://github.com/apache/avro/pull/1768#discussion_r918924967
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -694,6 +697,81 @@
public String toString() {
return name + " type:" + schema.type + " pos:" + position;
}
+
+ static Field parse(JsonNode field, Names names, String namespace) {
+ String fieldName = getRequiredText(field, "name", "No field name");
+ String fieldDoc = getOptionalText(field, "doc");
+ JsonNode fieldTypeNode = field.get("type");
+ if (fieldTypeNode == null) {
+ throw new SchemaParseException("No field type: " + field);
+ }
+
+ Schema fieldSchema = null;
+ if (fieldTypeNode.isTextual()) {
+ Schema schemaField = names.get(fieldTypeNode.textValue());
+ if (schemaField == null) {
+ schemaField = names.get(namespace + "." + fieldTypeNode.textValue());
+ }
+ if (schemaField == null) {
+ throw new SchemaParseException(fieldTypeNode + " is not a defined name." + " The type of the \"" + fieldName
+ + "\" field must be a defined name or a {\"type\": ...} expression.");
+ }
+ fieldSchema = schemaField;
+ } else if (fieldTypeNode.isObject()) {
+ fieldSchema = resolveSchema(fieldTypeNode, names, namespace);
+ if (fieldSchema == null) {
+ fieldSchema = Schema.parseFieldsOnly(fieldTypeNode, names, namespace);
+ }
+ } else if (fieldTypeNode.isArray()) {
+ List<Schema> unionTypes = new ArrayList<>();
+
+ fieldTypeNode.forEach((JsonNode node) -> {
+ Schema subSchema = null;
+ if (node.isTextual()) {
+ subSchema = names.get(node.asText());
+ if (subSchema == null) {
+ subSchema = names.get(namespace + "." + node.asText());
+ }
+ } else if (node.isObject()) {
+ subSchema = Schema.parseFieldsOnly(node, names, namespace);
+ } else {
+ throw new SchemaParseException("Illegal type in union : " + node);
+ }
+ if (subSchema == null) {
+ throw new SchemaParseException("Null element in union : " + node);
+ }
+ unionTypes.add(subSchema);
+ });
+
+ fieldSchema = Schema.createUnion(unionTypes);
+ }
+
+ if (fieldSchema == null) {
+ throw new SchemaParseException("Can't find type for field " + fieldName);
+ }
+ Field.Order order = Field.Order.ASCENDING;
+ JsonNode orderNode = field.get("order");
+ if (orderNode != null)
+ order = Field.Order.valueOf(orderNode.textValue().toUpperCase(Locale.ENGLISH));
+ JsonNode defaultValue = field.get("default");
+
+ if (defaultValue != null && fieldSchema != null
+ && (Type.FLOAT.equals(fieldSchema.getType()) || Type.DOUBLE.equals(fieldSchema.getType()))
+ && defaultValue.isTextual())
+ defaultValue = new DoubleNode(Double.valueOf(defaultValue.textValue()));
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/avro/security/code-scanning/2776)
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -694,6 +697,81 @@
public String toString() {
return name + " type:" + schema.type + " pos:" + position;
}
+
+ static Field parse(JsonNode field, Names names, String namespace) {
+ String fieldName = getRequiredText(field, "name", "No field name");
+ String fieldDoc = getOptionalText(field, "doc");
+ JsonNode fieldTypeNode = field.get("type");
+ if (fieldTypeNode == null) {
+ throw new SchemaParseException("No field type: " + field);
+ }
+
+ Schema fieldSchema = null;
+ if (fieldTypeNode.isTextual()) {
+ Schema schemaField = names.get(fieldTypeNode.textValue());
+ if (schemaField == null) {
+ schemaField = names.get(namespace + "." + fieldTypeNode.textValue());
+ }
+ if (schemaField == null) {
+ throw new SchemaParseException(fieldTypeNode + " is not a defined name." + " The type of the \"" + fieldName
+ + "\" field must be a defined name or a {\"type\": ...} expression.");
+ }
+ fieldSchema = schemaField;
+ } else if (fieldTypeNode.isObject()) {
+ fieldSchema = resolveSchema(fieldTypeNode, names, namespace);
+ if (fieldSchema == null) {
+ fieldSchema = Schema.parseFieldsOnly(fieldTypeNode, names, namespace);
+ }
+ } else if (fieldTypeNode.isArray()) {
+ List<Schema> unionTypes = new ArrayList<>();
+
+ fieldTypeNode.forEach((JsonNode node) -> {
+ Schema subSchema = null;
+ if (node.isTextual()) {
+ subSchema = names.get(node.asText());
+ if (subSchema == null) {
+ subSchema = names.get(namespace + "." + node.asText());
+ }
+ } else if (node.isObject()) {
+ subSchema = Schema.parseFieldsOnly(node, names, namespace);
+ } else {
+ throw new SchemaParseException("Illegal type in union : " + node);
+ }
+ if (subSchema == null) {
+ throw new SchemaParseException("Null element in union : " + node);
+ }
+ unionTypes.add(subSchema);
+ });
+
+ fieldSchema = Schema.createUnion(unionTypes);
+ }
+
+ if (fieldSchema == null) {
+ throw new SchemaParseException("Can't find type for field " + fieldName);
+ }
+ Field.Order order = Field.Order.ASCENDING;
+ JsonNode orderNode = field.get("order");
+ if (orderNode != null)
+ order = Field.Order.valueOf(orderNode.textValue().toUpperCase(Locale.ENGLISH));
+ JsonNode defaultValue = field.get("default");
+
+ if (defaultValue != null && fieldSchema != null
Review Comment:
## Useless null check
This check is useless, [fieldSchema](1) cannot be null here, since it is guarded by [... == ...](2).
[Show more details](https://github.com/apache/avro/security/code-scanning/2773)
##########
lang/java/avro/src/main/java/org/apache/avro/Schema.java:
##########
@@ -1668,78 +1746,38 @@
}
}
- /** @see #parse(String) */
- static Schema parse(JsonNode schema, Names names) {
+ static Schema parseNamesDeclared(JsonNode schema, Names names, String nameSpace) {
if (schema == null) {
- throw new SchemaParseException("Cannot parse <null> schema");
+ return null;
}
- if (schema.isTextual()) { // name
- Schema result = names.get(schema.textValue());
- if (result == null)
- throw new SchemaParseException("Undefined name: " + schema);
- return result;
- } else if (schema.isObject()) {
- Schema result;
- String type = getRequiredText(schema, "type", "No type");
+ if (schema.isObject()) {
+
+ String type = Schema.getOptionalText(schema, "type");
Name name = null;
- String savedSpace = names.space();
+
String doc = null;
+ Schema result = null;
final boolean isTypeError = "error".equals(type);
final boolean isTypeRecord = "record".equals(type);
final boolean isTypeEnum = "enum".equals(type);
final boolean isTypeFixed = "fixed".equals(type);
+
if (isTypeRecord || isTypeError || isTypeEnum || isTypeFixed) {
String space = getOptionalText(schema, "namespace");
doc = getOptionalText(schema, "doc");
if (space == null)
- space = savedSpace;
+ space = nameSpace;
name = new Name(getRequiredText(schema, "name", "No name in schema"), space);
- names.space(name.space); // set default namespace
}
- if (PRIMITIVES.containsKey(type)) { // primitive
- result = create(PRIMITIVES.get(type));
- } else if (isTypeRecord || isTypeError) { // record
- List<Field> fields = new ArrayList<>();
+ if (isTypeRecord || isTypeError) { // record
result = new RecordSchema(name, doc, isTypeError);
- if (name != null)
- names.add(result);
+ names.add(result);
JsonNode fieldsNode = schema.get("fields");
+
if (fieldsNode == null || !fieldsNode.isArray())
throw new SchemaParseException("Record has no fields: " + schema);
- for (JsonNode field : fieldsNode) {
- String fieldName = getRequiredText(field, "name", "No field name");
- String fieldDoc = getOptionalText(field, "doc");
- JsonNode fieldTypeNode = field.get("type");
- if (fieldTypeNode == null)
- throw new SchemaParseException("No field type: " + field);
- if (fieldTypeNode.isTextual() && names.get(fieldTypeNode.textValue()) == null)
- throw new SchemaParseException(fieldTypeNode + " is not a defined name." + " The type of the \"" + fieldName
- + "\" field must be a defined name or a {\"type\": ...} expression.");
- Schema fieldSchema = parse(fieldTypeNode, names);
- Field.Order order = Field.Order.ASCENDING;
- JsonNode orderNode = field.get("order");
- if (orderNode != null)
- order = Field.Order.valueOf(orderNode.textValue().toUpperCase(Locale.ENGLISH));
- JsonNode defaultValue = field.get("default");
- if (defaultValue != null
- && (Type.FLOAT.equals(fieldSchema.getType()) || Type.DOUBLE.equals(fieldSchema.getType()))
- && defaultValue.isTextual())
- defaultValue = new DoubleNode(Double.valueOf(defaultValue.textValue()));
- Field f = new Field(fieldName, fieldSchema, fieldDoc, defaultValue, true, order);
- Iterator<String> i = field.fieldNames();
- while (i.hasNext()) { // add field props
- String prop = i.next();
- if (!FIELD_RESERVED.contains(prop))
- f.addProp(prop, field.get(prop));
- }
- f.aliases = parseAliases(field);
- fields.add(f);
- if (fieldSchema.getLogicalType() == null && getOptionalText(field, LOGICAL_TYPE_PROP) != null)
- LOG.warn(
- "Ignored the {}.{}.logicalType property (\"{}\"). It should probably be nested inside the \"type\" for the field.",
- name, fieldName, getOptionalText(field, "logicalType"));
- }
- result.setFields(fields);
+ exploreFields(fieldsNode, names, name.space);
Review Comment:
## Dereferenced variable may be null
Variable [name](1) may be null here because of [this](2) assignment.
[Show more details](https://github.com/apache/avro/security/code-scanning/2774)
##########
lang/java/avro/src/test/java/org/apache/avro/TestProtocol.java:
##########
@@ -23,10 +23,33 @@
import static java.util.Collections.singletonMap;
import static org.junit.Assert.*;
+import java.io.File;
+import java.io.IOException;
+
+import org.junit.Assert;
import org.junit.Test;
public class TestProtocol {
+ @Test
+ public void parse() throws IOException {
+ File fic = new File("../../../share/test/schemas/namespace.avpr");
+ Protocol protocol = Protocol.parse(fic);
+ Assert.assertNotNull(protocol);
+ Assert.assertEquals("TestNamespace", protocol.getName());
+ }
+
+ @Test
+ public void testProUnknown() {
+ String userStatus = "{ \"protocol\" : \"p1\", " + "\"types\": ["
+ + "{\"name\": \"User\", \"type\": \"record\", \"fields\": [{\"name\": \"current_status\", \"type\": \"Status\"}]},\n"
+ + "\n"
+ + "{\"name\": \"Status\", \"type\": \"record\", \"fields\": [{\"name\": \"author\", \"type\": \"User\"}]}"
+ + "]}";
+
+ Protocol protocol = Protocol.parse(userStatus);
Review Comment:
## Unread local variable
Variable 'Protocol protocol' is never read.
[Show more details](https://github.com/apache/avro/security/code-scanning/2775)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] clesaec commented on pull request #1768: AVRO-530: Allow recursive types in protocol
Posted by "clesaec (via GitHub)" <gi...@apache.org>.
clesaec commented on PR #1768:
URL: https://github.com/apache/avro/pull/1768#issuecomment-1661699944
The fix in this PR is similar to the one in [this other](https://github.com/apache/avro/pull/2375) for parsing multiple schema [AVRO-3805](https://issues.apache.org/jira/browse/AVRO-3805).
So this one may wait the other to be merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org