Runtime: implement withoutUnknownFields() without builders - #3684
Conversation
When Kotlin generation hides builders, newBuilder() throws AssertionError. Strip unknown fields via an adapter round-trip of the known-field prefix.
oldergod
left a comment
oldergod
left a comment
There was a problem hiding this comment.
The fix is correct. All Wire encoders write unknown fields as the last bytes of the encoding, so the substring cut lands on a field boundary and the prefix decodes cleanly. Approving, with a few non-blocking comments.
On test coverage: no test in wire-tests/jvm-kotlin-interop calls withoutUnknownFields(). The PR description says that module covers javaInterop = true, but its UnknownFieldsTest only uses builders. Please add one assertion there that mirrors the Java variant in wire-tests/jvm-java-kotlin/.../UnknownFieldsTest.java. Also, no test covers a message that consists only of unknown fields, so the substring(0, 0) path never runs. A case such as OneField.ADAPTER.decode("109506".decodeHex()) with an assertion that withoutUnknownFields() equals OneField() would close that gap.
| * Implemented as an adapter round-trip of the known-field prefix rather than via [newBuilder], | ||
| * which throws when Kotlin generation hides builders (`javaInterop = false`). | ||
| */ | ||
| fun withoutUnknownFields(): M { |
There was a problem hiding this comment.
Consider an early return before the round trip:
if (unknownFields.size == 0) return this as MMessages with no unknown fields are the common case, and each call now pays a full encode and decode of the whole message, where the old builder path did a cheap field copy. The round trip also adds a new failure mode: the decoder enforces a recursion limit of 100, so a message graph nested deeper than that now throws IOException where the old path succeeded. The early return makes the no-op case O(1) and keeps deep graphs working when there is nothing to strip.
| @Suppress("UNCHECKED_CAST") | ||
| val message = this as M | ||
| val encoded = adapter.encodeByteString(message) | ||
| return adapter.decode(encoded.substring(0, encoded.size - unknownFields.size)) |
There was a problem hiding this comment.
This arithmetic depends on an invariant that is not stated anywhere: Wire adapters always write unknown fields as the last bytes of the encoding. It holds for every encoder in this repo today, but a hand-written adapter or a future encoder change could break it and silently drop known fields. Please state the invariant in the KDoc so the dependency is explicit.
|
Thanks for the contribution! One thing before we can merge: Square requires all outside contributors to sign our Contributor License Agreement, and you don't appear to be on the signers list yet. You can sign it here: https://docs.google.com/forms/d/e/1FAIpQLSeRVQ35-gq2vdSxD1kdh7CJwRdjmUA0EZ9gRXaWYoUeKPZEQQ/viewform?formkey=dDViT2xzUHAwRkI3X3k5Z0lQM091OGc6MQ&ndplr=1 It only takes a minute. Please drop a note here once you've signed and we can proceed with the merge.
|
Remaining tasksWhy am I seeing this comment?This comment is a summary of the pending tasks on this PR. It is intended to make it easier to find pending tasks so you can ship your PR faster. Got feedback? Let us know in #dx-help!
Want to disable me? Add |
2 participants
Fixes #1200.
Message.withoutUnknownFields()on the JVM callednewBuilder().clearUnknownFields().build(). Kotlin generation with the defaultjavaInterop = falseimplementsnewBuilder()asthrow AssertionError("Builders are deprecated..."), so callers on JVM Kotlin hit that assertion. Closed #1201 only movedUnknownFieldsTesttocommonTest, which usescopy(unknownFields = ByteString.EMPTY)and never exercises this JVM method.Wire always encodes this message's unknown fields as a suffix (
writer.writeBytes(value.unknownFields)last, including the reverse encoder). Decode that known-field prefix so builders are not required. Nested unknown fields stay until stripped on those messages, same as the old builder path.Test plan
:wire-tests:multiplatform:jvmTest --tests com.squareup.wire.WithoutUnknownFieldsTest(javaInterop=false) fails withAssertionError: Builders are deprecated...fromMessage.withoutUnknownFields:wire-tests:jvm-java-kotlin:test --tests com.squareup.wire.UnknownFieldsTest --tests com.squareup.wire.ParseTest:wire-tests:jvm-kotlin-interop:test --tests com.squareup.wire.UnknownFieldsTest(javaInterop=true):wire-runtime:apiCheckand spotless on the touched modules./gradlew -Dkjs=false -Dknative=false -Dkwasm=false -Pswift=falseI will complete Square's Individual CLA if the bot asks.