implement a converter to dynamic message #212

andrewparmet · 2023-12-30T22:07:28Z

No description provided.

…-dynamic-message

…ment-converter-to-dynamic-message

…mic-message

andrewparmet · 2024-02-07T16:59:28Z

protokt-core/src/jvmMain/kotlin/protokt/v1/google/protobuf/AnyUtil.kt

 inline fun <reified T : KtMessage> Any.isA() =
    typeUrl.substringAfterLast('/') ==
        (
            T::class.java.getAnnotation(KtGeneratedMessage::class.java)?.fullTypeName
-                ?: T::class.java.getAnnotation(com.toasttab.protokt.rt.KtGeneratedMessage::class.java)?.fullTypeName


this was removed a few PRs ago so it's dead code

andrewparmet · 2024-02-07T17:00:36Z

protokt-reflect/src/jvmMain/kotlin/protokt/v1/google/protobuf/KtMessages.kt

+fun KtMessage.toDynamicMessage(context: RuntimeContext): DynamicMessage =
+    context.convertValue(this) as DynamicMessage
+
+fun KtMessage.hasField(field: FieldDescriptor): Boolean {
+    val value = getField(field)
+
+    return if (field.hasPresence()) {
+        value != null
+    } else {
+        value != defaultValue(field)
+    }
+}
+
+private fun defaultValue(field: FieldDescriptor) =
+    when (field.type) {
+        Type.UINT64, Type.FIXED64 -> 0uL
+        Type.UINT32, Type.FIXED32 -> 0u
+        Type.BYTES -> Bytes.empty()
+        else -> field.defaultValue
+    }
+
+fun KtMessage.getField(field: FieldDescriptor) =
+    ProtoktReflect.getField(this, field)
+
+class RuntimeContext internal constructor(


New public APIs. Open to revision before 1.0.

andrewparmet · 2024-02-07T17:01:24Z

shared-src/reflect/protokt/v1/reflect/FieldType.kt

-import com.squareup.kotlinpoet.CodeBlock
-import com.squareup.kotlinpoet.MemberName
-import com.squareup.kotlinpoet.MemberName.Companion.member
-import com.squareup.kotlinpoet.asTypeName


removing kotlinpoet deps from code now used in reflection at runtime

andrewparmet · 2024-04-08T00:26:39Z

protokt-codegen/src/main/kotlin/protokt/v1/codegen/generate/DeserializerGenerator.kt

 import protokt.v1.codegen.util.KotlinPlugin
 import protokt.v1.codegen.util.Message
 import protokt.v1.codegen.util.Oneof
 import protokt.v1.codegen.util.StandardField
 import protokt.v1.codegen.util.Tag
+import protokt.v1.reflect.FieldType


All of these types that are used for codegen but are also going to be used in dynamic conversion are moved to shared source and stripped of any bad runtime deps, e.g. KotlinPoet. They're staying internal so the public API does not expose them.

andrewparmet · 2024-04-08T00:27:51Z

protokt-codegen/src/test/kotlin/protokt/v1/codegen/ProtoktCodegenTest.kt

@@ -30,3 +36,30 @@ class ProtoktCodegenTest : AbstractProtoktCodegenTest() {
            }
    }
 }
+
+object UuidBytesConverter : AbstractConverter<Bytes, UUID>(), OptimizedSizeOfConverter<Bytes, UUID> {


Allow testing of wrapper types with a debugger.

andrewparmet · 2024-04-08T00:29:12Z

protokt-reflect/build.gradle.kts

+            srcDir(rootProject.file("shared-src/reflect"))
+        }
+        proto {
+            srcDir("../extensions/protokt-extensions-lite/src/extensions-proto")


Generate the protokt extensions with Java for this module to be able to load its descriptor to get our custom options in the reflect runtime.

andrewparmet · 2024-04-08T00:29:58Z

protokt-reflect/src/jvmMain/kotlin/protokt/v1/google/protobuf/KtMessages.kt

+fun KtMessage.hasField(field: FieldDescriptor): Boolean {
+    val value = getField(field)
+
+    return if (field.hasPresence()) {
+        value != null
+    } else {
+        value != defaultValue(field)
+    }
+}
+
+private fun defaultValue(field: FieldDescriptor) =
+    when (field.type) {
+        Type.UINT64, Type.FIXED64 -> 0uL
+        Type.UINT32, Type.FIXED32 -> 0u
+        Type.BYTES -> Bytes.empty()
+        else -> field.defaultValue
+    }
+
+fun KtMessage.getField(field: FieldDescriptor) =


hasField and getField are the two APIs needed to run the Buf validator.

andrewparmet · 2024-04-08T00:31:00Z

shared-src/reflect/protokt/v1/reflect/ClassLookup.kt

-            }.run {
-                val table = HashBasedTable.create<String, String, MutableList<Converter<*, *>>>()
-                forEach { table.getOrPut(it.wrapped.qualifiedName!!, it.wrapper.qualifiedName!!) { mutableListOf() }.add(it) }
-                ImmutableTable.builder<String, String, List<Converter<*, *>>>().putAll(table).build()


Removing Guava from the reflect runtime's dependencies.

andrewparmet · 2024-04-08T00:31:33Z

shared-src/reflect/protokt/v1/reflect/ClassNames.kt

+
+package protokt.v1.reflect
+
+internal fun inferClassName(className: String, pkg: String): Pair<String, List<String>> {


These are more or less copied from KotlinPoet since we need them for reflection.

andrewparmet · 2024-04-08T00:31:53Z

shared-src/reflect/protokt/v1/reflect/FieldType.kt

-    data object SInt64 : Scalar(Long::class)
-    data object UInt32 : Scalar(UInt::class)
-    data object UInt64 : Scalar(ULong::class)
+    object Bool : Scalar(Boolean::class)


need compatibility with Kotlin 1.8

andrewparmet · 2024-04-08T00:32:38Z

third-party/proto-google-common-protos-lite/build.gradle.kts

+sourceSets {
+    test {
+        java {
+            srcDir(rootProject.file("shared-src/lite-util"))


forgotten edge case - good to have the lite tests here.

ogolberg

See comments inline.

My other question is - can we just codegenerate a converter from KtMesage to DynamicMessage for each KtMessage without using any reflection?

ogolberg · 2024-04-10T15:18:52Z

protokt-reflect/src/jvmMain/kotlin/protokt/v1/google/protobuf/KtMessages.kt

+}
+
+private fun getDescriptors() =
+    ClassGraph()


can we generate an annotation on the message class that points to the descriptor?

I feel really uncomfortable scanning the whole classpath for this

Perhaps. We can't point to it directly, since it won't exist in lite code, but maybe there's a way to make reflection direct instead of scanned.

Okay - the reason this is implemented reflectively is for convenience. If we had an annotation on a message in your hypothetical case, you still have to get those annotations, either by scanning or by explicit reference. If you have them explicitly then you may as well reference the descriptors themselves instead to build your own RuntimeContext.

If we don't want to encourage this kind of scan, which should only be done once, e.g. at application initialization to build validators, then we can remove this code and require users to write this themselves.

ogolberg · 2024-04-10T15:21:16Z

protokt-reflect/src/jvmMain/kotlin/protokt/v1/google/protobuf/KtMessages.kt

+
+    internal val descriptorsByTypeName = descriptors.associateBy { it.fullName }
+
+    fun convertValue(value: Any?) =


not obvious what this public API method does

I can make this not take a nullable argument for starters.

Would naming it better be a good idea? I think it has to be able to take any type since it's called at runtime with whatever field value the descriptor extracts from a protokt message.

A more descriptive name could be convertProtoktToProtobufJava.

The reason this has to be public is to enable sub-message conversion in protovalidate. If it's acceptable to convert the whole message every time, even if some portion of the message may never be validated, then this can be internal.

andrewparmet · 2024-04-11T01:49:11Z

can we just codegenerate a converter from KtMesage to DynamicMessage for each KtMessage without using any reflection?

We could, but that feels a bit messy. I haven't benchmarked this yet but I think it's a big enough step forward in terms of permitting message validation that it's worth having.

This doesn't prevent us from doing that in the future, of course.

andrewparmet · 2024-04-11T01:53:25Z

Code generation of a converter gets tricky - is there a way to do it without exposing a dependency on protobuf-java?

andrewparmet added 30 commits December 30, 2023 17:06

implement a converter to dynamic message

2dd4541

api dump

dd96e8e

fix

72fbb9f

internal

da3ff2e

reorganize shared but unpublished code

58b1d4c

lint

e28dd17

move over what is needed to infer protobuf type names

3659cb2

map value well known wrapper types do not work correctly

cd757ca

maps work

21d9534

compilation

283d20d

more tests

eca1983

api dump

8e96790

fix field presence lookup

f980afa

api dump

1036d0e

more api cleanup

b2b0e1a

fix map wrapper types

786ee1d

some cleanup

9d426aa

mostly clean up map wrapper types

5f6a280

isolate more and get tests passing

3689bc8

fix

c783115

fix tags in codegen for large field numbers

437909a

api dump

d1a4417

Merge branch 'fix-tags-for-large-numbers' into implement-converter-to…

deddc2d

…-dynamic-message

change strategy for sharing potentially unpublished code

d357e38

lint

e2e0379

Merge branch 'change-strategy-for-common-unpublished-code' into imple…

c679e4f

…ment-converter-to-dynamic-message

fix map wrapper types for well known wrappers

33206b3

Merge branch 'fix-map-wrapper-types' into implement-converter-to-dyna…

dbcd52c

…mic-message

remove guava

ed9c87b

rm guava

294fb53

andrewparmet and others added 6 commits January 6, 2024 12:14

rm line

3af2204

reflect need not be compatible with android

d150db2

turns out this line was needed

0e82a08

Update build.gradle.kts

301a9e2

Merge branch 'main' into implement-converter-to-dynamic-message

d50dd74

Update FieldTypeExt.kt

af1af55

andrewparmet commented Feb 7, 2024

View reviewed changes

andrewparmet marked this pull request as ready for review February 7, 2024 17:02

andrewparmet requested a review from ogolberg February 7, 2024 17:02

andrewparmet closed this Feb 7, 2024

andrewparmet deleted the implement-converter-to-dynamic-message branch February 7, 2024 19:02

andrewparmet restored the implement-converter-to-dynamic-message branch February 13, 2024 02:17

andrewparmet reopened this Feb 13, 2024

andrewparmet and others added 2 commits March 17, 2024 11:09

Merge branch 'main' into implement-converter-to-dynamic-message

a171c1e

no data objects

ad92a9f

andrewparmet commented Apr 8, 2024

View reviewed changes

ogolberg reviewed Apr 10, 2024

View reviewed changes

andrewparmet added 6 commits April 21, 2024 23:32

don't allow conversion of null

ae1c609

Merge branch 'main' into implement-converter-to-dynamic-message

dc621f5

classpath scan only in tests

27ff3cc

apidump

5eaae73

update readme

c7e643e

space

bc8cea8

ogolberg approved these changes May 8, 2024

View reviewed changes

andrewparmet merged commit aca2830 into open-toast:main May 8, 2024
13 checks passed

andrewparmet deleted the implement-converter-to-dynamic-message branch May 8, 2024 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement a converter to dynamic message #212

implement a converter to dynamic message #212

andrewparmet commented Dec 30, 2023

andrewparmet Feb 7, 2024

andrewparmet Feb 7, 2024

andrewparmet Feb 7, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

andrewparmet Apr 8, 2024

ogolberg left a comment

ogolberg Apr 10, 2024

andrewparmet Apr 10, 2024

andrewparmet Apr 11, 2024

ogolberg Apr 10, 2024

andrewparmet Apr 11, 2024

andrewparmet Apr 11, 2024

andrewparmet commented Apr 11, 2024

andrewparmet commented Apr 11, 2024


		package protokt.v1.reflect

		internal fun inferClassName(className: String, pkg: String): Pair<String, List<String>> {


		internal val descriptorsByTypeName = descriptors.associateBy { it.fullName }

		fun convertValue(value: Any?) =

implement a converter to dynamic message #212

implement a converter to dynamic message #212

Conversation

andrewparmet commented Dec 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogolberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewparmet commented Apr 11, 2024

andrewparmet commented Apr 11, 2024