From 9e5d82d54bda9b19aafa80811e2ff969db62d25f Mon Sep 17 00:00:00 2001 From: Erem Boto Date: Tue, 16 Jan 2018 14:32:25 -0800 Subject: [PATCH 1/2] Fix bug preventing DataTemplates.readRecord to process generated templates (#72) Also: - Enable scala-runtime test cases --- .../generator/RecordGeneratorTest.scala | 11 ++ .../generator/UnionGeneratorTest.scala | 12 +++ .../courier/templates/DataTemplates.scala | 52 ++++++--- .../codecs/InlineStringCodecTest.scala | 4 +- .../courier/templates/DataTemplatesTest.scala | 101 ++++++++++++++++-- 5 files changed, 153 insertions(+), 27 deletions(-) diff --git a/scala/generator-test/src/test/scala/org/coursera/courier/generator/RecordGeneratorTest.scala b/scala/generator-test/src/test/scala/org/coursera/courier/generator/RecordGeneratorTest.scala index 1901b43a..0c4f267d 100644 --- a/scala/generator-test/src/test/scala/org/coursera/courier/generator/RecordGeneratorTest.scala +++ b/scala/generator-test/src/test/scala/org/coursera/courier/generator/RecordGeneratorTest.scala @@ -470,4 +470,15 @@ class RecordGeneratorTest extends GeneratorTest with SchemaFixtures { // Fortunately, nothing should depend solely on hashCode() for determining whether two // records match. } + + @Test + def testDataTemplateHelperCompatibility(): Unit = { + // DataTemplates provides a bunch of convenient helper methods for + // serializing and deserializing records. Let's make sure the generated + // records are compatible with those methods. + val origRecord = Simple(Some("Hi")) + val jsString = DataTemplates.writeRecord(origRecord) + val roundTrippedRecord = DataTemplates.readRecord[Simple](jsString) + assert(origRecord === roundTrippedRecord) + } } diff --git a/scala/generator-test/src/test/scala/org/coursera/courier/generator/UnionGeneratorTest.scala b/scala/generator-test/src/test/scala/org/coursera/courier/generator/UnionGeneratorTest.scala index 9b8f1650..f2752a27 100644 --- a/scala/generator-test/src/test/scala/org/coursera/courier/generator/UnionGeneratorTest.scala +++ b/scala/generator-test/src/test/scala/org/coursera/courier/generator/UnionGeneratorTest.scala @@ -155,4 +155,16 @@ class UnionGeneratorTest extends GeneratorTest with SchemaFixtures { def testUnionTyperefSchema(): Unit = { assert(TypedDefinition.TYPEREF_SCHEMA.getDereferencedDataSchema === TypedDefinition.SCHEMA) } + + @Test + def testDataTemplateHelperSupport(): Unit = { + // DataTemplates provides a bunch of convenient helper methods for + // serializing and deserializing unions. Let's make sure the generated + // unions are compatible with those methods. + val origMember = WithPrimitiveCustomTypesUnion.Union.CustomIntMember(CustomInt(1)) + val original = WithPrimitiveCustomTypesUnion(origMember) + val json = DataTemplates.writeUnion(origMember) + val roundTripped = DataTemplates.readUnion[WithPrimitiveCustomTypesUnion.Union](json) + assert(origMember === roundTripped) + } } diff --git a/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala b/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala index 16632d92..642d4552 100644 --- a/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala +++ b/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala @@ -16,6 +16,8 @@ package org.coursera.courier.templates +import java.lang.reflect.Method + import com.linkedin.data.DataComplex import com.linkedin.data.DataList import com.linkedin.data.DataMap @@ -211,32 +213,52 @@ object DataTemplates { private[this] def newDataMapTemplate[T <: DataTemplate[DataMap]]( data: DataMap, clazz: Class[T]): T = { val companionInstance = companion(clazz) - val applyMethod = - companionInstance.getClass.getDeclaredMethod( - "apply", - classOf[DataMap], - classOf[DataConversion]) + val applyMethod = getTemplateDataConstructor(companionInstance, classOf[DataMap]) + applyMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] } private[this] def newDataListTemplate[T <: DataTemplate[DataList]]( data: DataList, clazz: Class[T]): T = { val companionInstance = companion(clazz) - val applyMethod = - companionInstance.getClass.getDeclaredMethod( - "apply", - classOf[DataList], - classOf[DataConversion]) + val applyMethod = getTemplateDataConstructor(companionInstance, classOf[DataList]) applyMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] } private[this] def newUnionTemplate[T <: UnionTemplate](data: DataMap, clazz: Class[T]): T = { val companionInstance = companion(clazz) - val applyMethod = - companionInstance.getClass.getDeclaredMethod( - "apply", - classOf[DataMap], - classOf[DataConversion]) + val applyMethod = getTemplateDataConstructor(companionInstance, classOf[DataMap]) + applyMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] } + + /** + * Reflect for the correct method to instantiate the template from a DataMap + * and DataConversion. The correct method is usually "build", since + * pre-2.0.3. Before then it was "apply". In case this method is reading + * templates generated in an old version, we start with "build" and fall back + * on "apply". + * + * @param tmplCompanionInstance the result of calling the private method companion(clazz) + * @param dataClass either classOf[DataMap] or classOf[DataList], depending on whether + * operating on a record/union or an array. + * @return a method that can be construct a Union or Record template when + * invoked a DataMap and DataConversion as args + */ + private[this] def getTemplateDataConstructor(tmplCompanionInstance: AnyRef, dataClass: Class[_]): Method = { + val companionClass = tmplCompanionInstance.getClass + def getWithName(methodName: String): Method = { + companionClass.getDeclaredMethod( + methodName, + dataClass, + classOf[DataConversion] + ) + } + + try { + getWithName("build") + } catch { + case e: NoSuchMethodException => getWithName("apply") + } + } } diff --git a/scala/runtime/src/test/scala/org/coursera/courier/codecs/InlineStringCodecTest.scala b/scala/runtime/src/test/scala/org/coursera/courier/codecs/InlineStringCodecTest.scala index ddcf2438..c205aff6 100644 --- a/scala/runtime/src/test/scala/org/coursera/courier/codecs/InlineStringCodecTest.scala +++ b/scala/runtime/src/test/scala/org/coursera/courier/codecs/InlineStringCodecTest.scala @@ -33,9 +33,9 @@ import com.linkedin.data.template.DataTemplateUtil import org.coursera.courier.templates.DataTemplates import org.coursera.courier.templates.DataValidationException import org.junit.Test -import org.scalatest.junit.AssertionsForJUnit +import org.scalatest.junit.{AssertionsForJUnit, JUnitSuite} -class InlineStringCodecTest extends AssertionsForJUnit { +class InlineStringCodecTest extends JUnitSuite with AssertionsForJUnit { @Test def testPrimitives(): Unit = { diff --git a/scala/runtime/src/test/scala/org/coursera/courier/templates/DataTemplatesTest.scala b/scala/runtime/src/test/scala/org/coursera/courier/templates/DataTemplatesTest.scala index 940880f6..a6b0fb65 100644 --- a/scala/runtime/src/test/scala/org/coursera/courier/templates/DataTemplatesTest.scala +++ b/scala/runtime/src/test/scala/org/coursera/courier/templates/DataTemplatesTest.scala @@ -23,40 +23,91 @@ import com.linkedin.data.schema.UnionDataSchema import com.linkedin.data.template.DataTemplateUtil import com.linkedin.data.template.RecordTemplate import com.linkedin.data.template.UnionTemplate +import org.coursera.courier.data.IntArray import org.coursera.courier.templates.DataTemplates.DataConversion -import org.scalatest.junit.AssertionsForJUnit +import org.scalatest.junit.{AssertionsForJUnit, JUnitSuite} import org.junit.Test -class DataTemplatesTest extends AssertionsForJUnit { +class DataTemplatesTest extends JUnitSuite with AssertionsForJUnit { import DataTemplatesTest._ + @Test + def readWriteLegacyRecord(): Unit = { + val json = """{"string":"a","int":1}""" + val record = DataTemplates.readRecord[MockLegacyRecord](json) + val roundTripped = DataTemplates.writeRecord(record) + assert(DataTemplates.readDataMap(roundTripped) === DataTemplates.readDataMap(json)) + } + @Test def readWriteRecord(): Unit = { val json = """{"string":"a","int":1}""" - val union = DataTemplates.readRecord[MockRecord](json) - val roundTripped = DataTemplates.writeRecord(union) + val record = DataTemplates.readRecord[MockRecord](json) + val roundTripped = DataTemplates.writeRecord(record) assert(DataTemplates.readDataMap(roundTripped) === DataTemplates.readDataMap(json)) } @Test def readWriteUnion(): Unit = { val json = """{"int":1}""" - val union = DataTemplates.readUnion[MockTyperefUnion](json) + val union = DataTemplates.readUnion[MockLegacyTyperefUnion](json) + val roundTripped = DataTemplates.writeUnion(union) + assert(DataTemplates.readDataMap(roundTripped) === DataTemplates.readDataMap(json)) + } + + @Test + def readWriteLegacyUnion(): Unit = { + val json = """{"int":1}""" + val union = DataTemplates.readUnion[MockLegacyTyperefUnion](json) val roundTripped = DataTemplates.writeUnion(union) assert(DataTemplates.readDataMap(roundTripped) === DataTemplates.readDataMap(json)) } @Test def testGetSchema(): Unit = { - val schemaFromClass = DataTemplates.getSchema(classOf[MockRecord]) - assert(schemaFromClass === MockRecord.SCHEMA) + val schemaFromClass = DataTemplates.getSchema(classOf[MockLegacyRecord]) + assert(schemaFromClass === MockLegacyRecord.SCHEMA) + + val schemaFromClassTag = DataTemplates.getSchema[MockLegacyRecord] + assert(schemaFromClassTag === MockLegacyRecord.SCHEMA) + } - val schemaFromClassTag = DataTemplates.getSchema[MockRecord] - assert(schemaFromClassTag === MockRecord.SCHEMA) + @Test + def testReadWriteArray(): Unit = { + val array = "[1,2,3,4,5]" + val read = DataTemplates.readArray[IntArray](array) + val written = DataTemplates.writeArray(read) + assert(array === written) } } object DataTemplatesTest { + class MockLegacyRecord(private val dataMap: DataMap) + extends RecordTemplate(dataMap, MockLegacyRecord.SCHEMA) { + dataMap.makeReadOnly() + } + + object MockLegacyRecord { + val SCHEMA_JSON = + """ + |{ + | "name": "MockLegacyRecord", + | "type": "record", + | "fields": [ + | { "name": "string", "type": "string" }, + | { "name": "int", "type": "int" } + | ] + |} + |""".stripMargin + + val SCHEMA = DataTemplateUtil.parseSchema(SCHEMA_JSON).asInstanceOf[RecordDataSchema] + + def apply(dataMap: DataMap, dataConversion: DataConversion) = { + new MockLegacyRecord(dataMap) + } + } + + /** Like MockRecord, but has `build` instead of `apply`, as per */ class MockRecord(private val dataMap: DataMap) extends RecordTemplate(dataMap, MockRecord.SCHEMA) { dataMap.makeReadOnly() @@ -77,11 +128,41 @@ object DataTemplatesTest { val SCHEMA = DataTemplateUtil.parseSchema(SCHEMA_JSON).asInstanceOf[RecordDataSchema] - def apply(dataMap: DataMap, dataConversion: DataConversion) = { + def build(dataMap: DataMap, dataConversion: DataConversion) = { new MockRecord(dataMap) } } + class MockLegacyTyperefUnion(private val dataMap: DataMap) + extends UnionTemplate(dataMap, MockLegacyTyperefUnion.SCHEMA) { + dataMap.makeReadOnly() + } + + object MockLegacyTyperefUnion { + val SCHEMA_JSON = + """ + |[ "int", "string" ] + |""".stripMargin + + val SCHEMA = DataTemplateUtil.parseSchema(SCHEMA_JSON).asInstanceOf[UnionDataSchema] + + val TYPEREF_SCHEMA_JSON = + """ + |{ + | "name": "MockTyperefUnion", + | "type": "typeref", + | "ref": [ "int", "string" ] + |} + |""".stripMargin + + val TYPEREF_SCHEMA = + DataTemplateUtil.parseSchema(TYPEREF_SCHEMA_JSON).asInstanceOf[TyperefDataSchema] + + def apply(dataMap: DataMap, dataConversion: DataConversion) = { + new MockLegacyTyperefUnion(dataMap) + } + } + class MockTyperefUnion(private val dataMap: DataMap) extends UnionTemplate(dataMap, MockTyperefUnion.SCHEMA) { dataMap.makeReadOnly() From bf6eb9f8031f2d43189a42fc12c8ed87bac43a0b Mon Sep 17 00:00:00 2001 From: Erem Boto Date: Tue, 23 Jan 2018 15:50:50 -0800 Subject: [PATCH 2/2] applyMethod -> buildMethod --- .../coursera/courier/templates/DataTemplates.scala | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala b/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala index 642d4552..c0410e67 100644 --- a/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala +++ b/scala/runtime/src/main/scala/org/coursera/courier/templates/DataTemplates.scala @@ -213,23 +213,23 @@ object DataTemplates { private[this] def newDataMapTemplate[T <: DataTemplate[DataMap]]( data: DataMap, clazz: Class[T]): T = { val companionInstance = companion(clazz) - val applyMethod = getTemplateDataConstructor(companionInstance, classOf[DataMap]) + val buildMethod = getTemplateDataConstructor(companionInstance, classOf[DataMap]) - applyMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] + buildMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] } private[this] def newDataListTemplate[T <: DataTemplate[DataList]]( data: DataList, clazz: Class[T]): T = { val companionInstance = companion(clazz) - val applyMethod = getTemplateDataConstructor(companionInstance, classOf[DataList]) - applyMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] + val buildMethod = getTemplateDataConstructor(companionInstance, classOf[DataList]) + buildMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] } private[this] def newUnionTemplate[T <: UnionTemplate](data: DataMap, clazz: Class[T]): T = { val companionInstance = companion(clazz) - val applyMethod = getTemplateDataConstructor(companionInstance, classOf[DataMap]) + val buildMethod = getTemplateDataConstructor(companionInstance, classOf[DataMap]) - applyMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] + buildMethod.invoke(companionInstance, data, DataConversion.SetReadOnly).asInstanceOf[T] } /**