Skip to content

Commit

Permalink
IPROTO-62 Implement annotation based schema generation support for 'o…
Browse files Browse the repository at this point in the history 
…neof'
  • Loading branch information
@anistor
anistor committed Jul 4, 2018
1 parent efe2e99 commit 7a9191c
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 15 deletions.
2 changes: 2 additions & 0 deletions core/src/main/java/org/infinispan/protostream/annotations/ProtoField.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,6 @@
Class<?> javaType() default void.class;

Class<? extends Collection> collectionImplementation() default Collection.class;

String oneof() default "";
}
21 changes: 15 additions & 6 deletions core/src/main/java/org/infinispan/protostream/annotations/impl/ProtoFieldMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ final class ProtoFieldMetadata implements HasProtoSchema {

private final int number;
private final String name;
private final String oneof;
private final Class<?> javaType;
private final Class<?> collectionImplementation;
private final Type protobufType;
Expand All @@ -32,12 +33,13 @@ final class ProtoFieldMetadata implements HasProtoSchema {
private final Method getter;
private final Method setter;

ProtoFieldMetadata(int number, String name, Class<?> javaType,
ProtoFieldMetadata(int number, String name, String oneof, Class<?> javaType,
Class<?> collectionImplementation, Type protobufType, ProtoTypeMetadata protoTypeMetadata,
boolean isRequired, boolean isRepeated, boolean isArray, Object defaultValue,
Field field) {
this.number = number;
this.name = name;
this.oneof = oneof;
this.javaType = javaType;
this.collectionImplementation = collectionImplementation;
this.protoTypeMetadata = protoTypeMetadata;
Expand All @@ -53,12 +55,13 @@ final class ProtoFieldMetadata implements HasProtoSchema {
this.documentation = DocumentationExtractor.getDocumentation(field);
}

ProtoFieldMetadata(int number, String name, Class<?> javaType,
ProtoFieldMetadata(int number, String name, String oneof, Class<?> javaType,
Class<?> collectionImplementation, Type protobufType, ProtoTypeMetadata protoTypeMetadata,
boolean isRequired, boolean isRepeated, boolean isArray, Object defaultValue,
Method definingMethod, Method getter, Method setter) {
this.number = number;
this.name = name;
this.oneof = oneof;
this.javaType = javaType;
this.collectionImplementation = collectionImplementation;
this.protoTypeMetadata = protoTypeMetadata;
Expand All @@ -82,6 +85,10 @@ public String getName() {
return name;
}

public String getOneof() {
return oneof;
}

public Class<?> getJavaType() {
return javaType;
}
Expand Down Expand Up @@ -138,10 +145,12 @@ public String getLocation() {
public void generateProto(IndentWriter iw) {
iw.append('\n');
ProtoTypeMetadata.appendDocumentation(iw, documentation);
if (isRepeated) {
iw.append("repeated ");
} else {
iw.append(isRequired ? "required " : "optional ");
if (oneof == null) {
if (isRepeated) {
iw.append("repeated ");
} else {
iw.append(isRequired ? "required " : "optional ");
}
}
String typeName;
if (protobufType == Type.ENUM || protobufType == Type.MESSAGE || protobufType == Type.GROUP) {
Expand Down
93 changes: 85 additions & 8 deletions core/src/main/java/org/infinispan/protostream/annotations/impl/ProtoMessageTypeMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;

import org.infinispan.protostream.annotations.ProtoDoc;
import org.infinispan.protostream.annotations.ProtoField;
import org.infinispan.protostream.annotations.ProtoMessage;
import org.infinispan.protostream.annotations.ProtoName;
Expand Down Expand Up @@ -112,8 +115,26 @@ public void generateProto(IndentWriter iw) {
}

iw.inc();
for (ProtoFieldMetadata f : fields.values()) {
f.generateProto(iw);
LinkedList<ProtoFieldMetadata> unprocessedFields = new LinkedList<>(fields.values());
while (!unprocessedFields.isEmpty()) {
ProtoFieldMetadata f = unprocessedFields.remove();
if (f.getOneof() == null) {
f.generateProto(iw);
} else {
iw.append("\noneof ").append(f.getOneof()).append(" {\n");
iw.inc();
f.generateProto(iw);
Iterator<ProtoFieldMetadata> it = unprocessedFields.iterator();
while (it.hasNext()) {
ProtoFieldMetadata f2 = it.next();
if (f.getOneof().equals(f2.getOneof())) {
f2.generateProto(iw);
it.remove();
}
}
iw.dec();
iw.append("}\n");
}
}
iw.dec();

Expand All @@ -135,8 +156,11 @@ public void scanMemberAnnotations() {
// all the fields discovered in this class hierarchy, by name
Map<String, ProtoFieldMetadata> fieldsByName = new HashMap<>();

// all the oneofs discovered in this class hierarchy
Set<String> oneofs = new HashSet<>();

Set<Class<?>> examinedClasses = new HashSet<>();
discoverFields(javaClass, examinedClasses, fields, fieldsByName);
discoverFields(javaClass, examinedClasses, fields, fieldsByName, oneofs);
if (fields.isEmpty()) {
throw new ProtoSchemaBuilderException("Class " + javaClass.getCanonicalName() + " does not have any @ProtoField annotated fields. The class should be either annotated or it should have a custom marshaller.");
}
Expand All @@ -155,24 +179,27 @@ private void checkConstructor() {
}
}

private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<Integer, ProtoFieldMetadata> fieldsByNumber, Map<String, ProtoFieldMetadata> fieldsByName) {
private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<Integer, ProtoFieldMetadata> fieldsByNumber, Map<String, ProtoFieldMetadata> fieldsByName, Set<String> oneofs) {
if (!examinedClasses.add(clazz)) {
// avoid re-examining classes due to multiple interface inheritance
return;
}

if (clazz.getSuperclass() != null) {
discoverFields(clazz.getSuperclass(), examinedClasses, fieldsByNumber, fieldsByName);
discoverFields(clazz.getSuperclass(), examinedClasses, fieldsByNumber, fieldsByName, oneofs);
}
for (Class<?> i : clazz.getInterfaces()) {
discoverFields(i, examinedClasses, fieldsByNumber, fieldsByName);
discoverFields(i, examinedClasses, fieldsByNumber, fieldsByName, oneofs);
}

for (Field field : clazz.getDeclaredFields()) {
if (field.getAnnotation(ProtoUnknownFieldSet.class) != null) {
if (unknownFieldSetField != null || unknownFieldSetGetter != null || unknownFieldSetSetter != null) {
throw new ProtoSchemaBuilderException("The @ProtoUnknownFieldSet annotation should not be used multiple times in one class hierarchy : " + field);
}
if (field.getAnnotation(ProtoField.class) != null) {
throw new ProtoSchemaBuilderException("The @ProtoUnknownFieldSet and @ProtoField annotations cannot be used together: " + field);
}
unknownFieldSetField = field;
} else {
ProtoField annotation = field.getAnnotation(ProtoField.class);
Expand Down Expand Up @@ -222,7 +249,20 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
if (protobufType == Type.ENUM || protobufType == Type.MESSAGE || protobufType == Type.GROUP) {
protoTypeMetadata = protoSchemaGenerator.scanAnnotations(javaType);
}
ProtoFieldMetadata fieldMetadata = new ProtoFieldMetadata(annotation.number(), fieldName, javaType, collectionImplementation,

String oneof = annotation.oneof();
if (oneof.isEmpty()) {
oneof = null;
} else {
if (oneof.equals(fieldName) || fieldsByName.containsKey(oneof)) {
throw new ProtoSchemaBuilderException("The field named '" + fieldName + "' of " + clazz.getName() + " is member of the '" + oneof + "' oneof which collides with an existing field or oneof.");
}
if (isRepeated || isRequired) {
throw new ProtoSchemaBuilderException("The field named '" + fieldName + "' of " + clazz.getName() + " cannot be marked repeated or required since it is member of the '" + oneof + " oneof.");
}
oneofs.add(oneof);
}
ProtoFieldMetadata fieldMetadata = new ProtoFieldMetadata(annotation.number(), fieldName, oneof, javaType, collectionImplementation,
protobufType, protoTypeMetadata, isRequired, isRepeated, isArray, defaultValue, field);

ProtoFieldMetadata existing = fieldsByNumber.get(annotation.number());
Expand All @@ -242,11 +282,20 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
}
}

Set<Method> skipMethods = new HashSet<>();

for (Method method : clazz.getDeclaredMethods()) {
if (skipMethods.contains(method)) {
continue;
}

if (method.getAnnotation(ProtoUnknownFieldSet.class) != null) {
if (unknownFieldSetField != null || unknownFieldSetGetter != null || unknownFieldSetSetter != null) {
throw new ProtoSchemaBuilderException("The @ProtoUnknownFieldSet annotation should not be used multiple times in one class hierarchy : " + method);
}
if (method.getAnnotation(ProtoField.class) != null) {
throw new ProtoSchemaBuilderException("The @ProtoUnknownFieldSet and @ProtoField annotations cannot be used together: " + method);
}
String propertyName;
if (method.getReturnType().equals(Void.TYPE)) {
// this method is expected to be a setter
Expand All @@ -260,6 +309,8 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
}
unknownFieldSetSetter = method;
unknownFieldSetGetter = findGetter(propertyName, method.getParameterTypes()[0]);
checkForbiddenAnnotations(unknownFieldSetGetter, unknownFieldSetSetter);
skipMethods.add(unknownFieldSetGetter);
} else {
// this method is expected to be a getter
if (method.getName().startsWith("get") && method.getName().length() >= 4) {
Expand All @@ -271,6 +322,8 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
}
unknownFieldSetGetter = method;
unknownFieldSetSetter = findSetter(propertyName, unknownFieldSetGetter.getReturnType());
checkForbiddenAnnotations(unknownFieldSetSetter, unknownFieldSetGetter);
skipMethods.add(unknownFieldSetSetter);
}
} else {
ProtoField annotation = method.getAnnotation(ProtoField.class);
Expand All @@ -297,6 +350,8 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
}
setter = method;
getter = findGetter(propertyName, method.getParameterTypes()[0]);
checkForbiddenAnnotations(getter, setter);
skipMethods.add(getter);
} else {
// this method is expected to be a getter
if (method.getName().startsWith("get") && method.getName().length() >= 4) {
Expand All @@ -308,6 +363,8 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
}
getter = method;
setter = findSetter(propertyName, getter.getReturnType());
checkForbiddenAnnotations(setter, getter);
skipMethods.add(setter);
}
String fieldName = annotation.name();
if (fieldName.isEmpty()) {
Expand Down Expand Up @@ -346,7 +403,19 @@ private void discoverFields(Class<?> clazz, Set<Class<?>> examinedClasses, Map<I
protoTypeMetadata = protoSchemaGenerator.scanAnnotations(javaType);
}

ProtoFieldMetadata fieldMetadata = new ProtoFieldMetadata(annotation.number(), fieldName, javaType, collectionImplementation,
String oneof = annotation.oneof();
if (oneof.isEmpty()) {
oneof = null;
} else {
if (oneof.equals(fieldName) || fieldsByName.containsKey(oneof)) {
throw new ProtoSchemaBuilderException("The field named '" + fieldName + "' of " + clazz.getName() + " is member of the '" + oneof + "' oneof which collides with an existing field or oneof.");
}
if (isRepeated || isRequired) {
throw new ProtoSchemaBuilderException("The field named '" + fieldName + "' of " + clazz.getName() + " cannot be marked repeated or required since it is member of the '" + oneof + " oneof.");
}
oneofs.add(oneof);
}
ProtoFieldMetadata fieldMetadata = new ProtoFieldMetadata(annotation.number(), fieldName, oneof, javaType, collectionImplementation,
protobufType, protoTypeMetadata, isRequired, isRepeated, isArray, defaultValue,
method, getter, setter);

Expand Down Expand Up @@ -585,6 +654,14 @@ private Method findSetter(String propertyName, Class<?> propertyType) {
return setter;
}

private void checkForbiddenAnnotations(Method m1, Method m2) {
if (m1.getAnnotation(ProtoDoc.class) != null
|| m1.getAnnotation(ProtoField.class) != null
|| m1.getAnnotation(ProtoUnknownFieldSet.class) != null) {
throw new ProtoSchemaBuilderException("No @ProtoDoc, @ProtoField or @ProtoUnknownFieldSet annotations allowed on method " + m1 + ". They should have been added to " + m2);
}
}

private static Class<?> determineElementType(Class<?> type, java.lang.reflect.Type genericType) {
if (type.isArray()) {
return type.getComponentType();
Expand Down
74 changes: 74 additions & 0 deletions core/src/test/java/org/infinispan/protostream/annotations/impl/ProtoSchemaBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.infinispan.protostream.FileDescriptorSource;
import org.infinispan.protostream.ProtobufUtil;
import org.infinispan.protostream.SerializationContext;
import org.infinispan.protostream.annotations.ProtoDoc;
import org.infinispan.protostream.annotations.ProtoEnumValue;
import org.infinispan.protostream.annotations.ProtoField;
import org.infinispan.protostream.annotations.ProtoSchemaBuilder;
Expand Down Expand Up @@ -793,4 +794,77 @@ public void testAllFieldTypes() throws Exception {
byte[] bytes = ProtobufUtil.toWrappedByteArray(ctx, new MessageWithAllFieldTypes());
ProtobufUtil.fromWrappedByteArray(ctx, bytes);
}

@ProtoDoc("A test for 'oneof'")
public static class TestOneof {

@ProtoField(number = 1, oneof = "oneof1")
public String fieldA;

@ProtoField(number = 2, oneof = "oneof1")
public String fieldB;

@ProtoField(number = 3)
public String fieldC;

@ProtoField(number = 4, oneof = "oneof2")
public String fieldD;

@ProtoField(number = 5, oneof = "oneof2")
public String fieldE;
}

@Test
public void testOneOf() throws Exception {
SerializationContext ctx = createContext();
ProtoSchemaBuilder protoSchemaBuilder = new ProtoSchemaBuilder();
protoSchemaBuilder
.fileName("test.proto")
.addClass(TestOneof.class)
.build(ctx);

assertTrue(ctx.canMarshall(TestOneof.class));
assertTrue(ctx.canMarshall("TestOneof"));
}

public static class TestInvalidOneof1 {

@ProtoField(number = 1, oneof = "fieldA")
public String fieldA;
}

@Test
public void testInvalidOneOf1() throws Exception {
exception.expect(ProtoSchemaBuilderException.class);
exception.expectMessage("The field named 'fieldA' of org.infinispan.protostream.annotations.impl.ProtoSchemaBuilderTest$TestInvalidOneof1 is member of the 'fieldA' oneof which collides with an existing field or oneof.");

SerializationContext ctx = createContext();
ProtoSchemaBuilder protoSchemaBuilder = new ProtoSchemaBuilder();
protoSchemaBuilder
.fileName("test.proto")
.addClass(TestInvalidOneof1.class)
.build(ctx);
}

public static class TestInvalidOneof2 {

@ProtoField(number = 1, name = "fieldX")
public String fieldA;

@ProtoField(number = 2, name = "fieldX")
public String fieldB;
}

@Test
public void testInvalidOneOf2() throws Exception {
exception.expect(ProtoSchemaBuilderException.class);
exception.expectMessage("The field named 'fieldA' of org.infinispan.protostream.annotations.impl.ProtoSchemaBuilderTest$TestInvalidOneof1 is member of the 'fieldA' oneof which collides with an existing field or oneof.");

SerializationContext ctx = createContext();
ProtoSchemaBuilder protoSchemaBuilder = new ProtoSchemaBuilder();
protoSchemaBuilder
.fileName("test.proto")
.addClass(TestInvalidOneof2.class)
.build(ctx);
}
}
3 changes: 2 additions & 1 deletion core/src/test/java/org/infinispan/protostream/annotations/impl/testdomain/Simple.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public class Simple {
@ProtoField(number = 1111)
public Simple simple;

@ProtoField(number = 1, required = true) //todo [anistor] should be possible not to require required in this case?
@ProtoField(number = 1, required = true) //todo [anistor] should be possible not to require 'required' in this case because it is implied already?
public float afloat;

@ProtoField(number = 2)
Expand All @@ -25,5 +25,6 @@ public class Simple {
public TestEnum myEnumField;

@ProtoUnknownFieldSet
@ProtoField(number = 314, name = "my_enum_field", defaultValue = "AX")
public UnknownFieldSet unknownFieldSet;
}

0 comments on commit 7a9191c

Please sign in to comment.