SchemaDefinitionBuilder.withJSR310ConversionEnabled(true) has no effect (it should use timestamp-millis for java.time.Instant) #15858
Description
Describe the bug
When JSR310 is enabled in SchemaDefintionBuilder the TimestampMillisConversion is registered for java.time.Instant (using logical type timestamp-millis), however that conversion is never applied as TimestampMicrosConversion is also registered first (i.e. with higher priority) inside AvroSchema class of Pulsar client.
E.g. take this POJO definition:
public class TestObject
{
private Instant startTimestamp;
public Instant getStartTimestamp() { return startTimestamp; }
public void setStartTimestamp(Instant startTimestamp) { this.startTimestamp = startTimestamp; }
}
Consider these two ways of obtaining AVRO schema from POJO:
Schema<TestObject> schema = Schema.AVRO(TestObject.class);
Schema<TestObject> schema = AvroSchema.of(SchemaDefinition.<T>builder().withPojo(TestObject.class).withJSR310ConversionEnabled(true).build());
These two methods produce same AVRO type for field startTimestamp (union with null has been omited for clarity):
{"type":"long","logicalType":"timestamp-micros"}
Second method should however produce following type (timestamp-millis instead of timestamp-micros):
{"type":"long","logicalType":"timestamp-millis"}
If we look at AvroSchema.addLogicalTypeConversions() we can spot the BUG:
public static void addLogicalTypeConversions(ReflectData reflectData, boolean jsr310ConversionEnabled, boolean decimalConversionEnabled) {
if (decimalConversionEnabled) {
reflectData.addLogicalTypeConversion(new DecimalConversion());
}
reflectData.addLogicalTypeConversion(new DateConversion());
reflectData.addLogicalTypeConversion(new TimeMillisConversion());
reflectData.addLogicalTypeConversion(new TimeMicrosConversion());
reflectData.addLogicalTypeConversion(new TimestampMicrosConversion());
if (jsr310ConversionEnabled) {
// This registration has no effect as TimestampMicrosConversion is
// registered first and always takes precedence
reflectData.addLogicalTypeConversion(new TimestampMillisConversion());
} else {
try {
Class.forName("org.joda.time.DateTime");
reflectData.addLogicalTypeConversion(new AvroSchema.TimestampConversion());
} catch (ClassNotFoundException var4) {
}
}
reflectData.addLogicalTypeConversion(new UUIDConversion());
}
One fix could be the following:
public static void addLogicalTypeConversions(ReflectData reflectData, boolean jsr310ConversionEnabled, boolean decimalConversionEnabled) {
if (decimalConversionEnabled) {
reflectData.addLogicalTypeConversion(new DecimalConversion());
}
reflectData.addLogicalTypeConversion(new DateConversion());
reflectData.addLogicalTypeConversion(new TimeMillisConversion());
reflectData.addLogicalTypeConversion(new TimeMicrosConversion());
if (jsr310ConversionEnabled) {
reflectData.addLogicalTypeConversion(new TimestampMillisConversion());
} else {
// Register alternative conversion to micros if JSR310 is not enabled
reflectData.addLogicalTypeConversion(new TimestampMicrosConversion());
try {
Class.forName("org.joda.time.DateTime");
reflectData.addLogicalTypeConversion(new AvroSchema.TimestampConversion());
} catch (ClassNotFoundException var4) {
}
}
reflectData.addLogicalTypeConversion(new UUIDConversion());
}
To Reproduce
Already indicated in the description.
Expected behavior
When JSR310 flag is enabled interpret use logical type timestamp-millis instead of timestamp-micros in AVRO Schema for java.time.Instant types.
Screenshots
None
Desktop (please complete the following information):
NA
Additional context
Affects all versions since 2.5.1