Distinguish between converters that are safe to call on default values and those that would fail during deserialization on default-valued fields #205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewparmet
commented
Oct 21, 2023
|
|
||
| @AutoService(Converter::class) | ||
| @Deprecated("use LocalDateStringConverter or upgrade protokt") | ||
| object LocalDateConverter : Converter<LocalDate, String> { |
There was a problem hiding this comment.
Not needed in V1; only needed for ABI backwards compatibility.
andrewparmet
commented
Oct 21, 2023
Comment on lines
-72
to
+80
| } ?: interceptValueAccess(f, ctx, accessSize) | ||
| } ?: accessSize |
There was a problem hiding this comment.
this wasn't doing anything
andrewparmet
commented
Oct 21, 2023
Comment on lines
85
to
94
| fieldAccess: CodeBlock, | ||
| ctx: Context | ||
| ) = | ||
| f.withWrapper(ctx) { wrapper, wrapped -> | ||
| val converter = converter(wrapper, wrapped, ctx) | ||
| if (converter.optimizedSizeof) { | ||
| CodeBlock.of("%T.sizeOf(%L)", converter.className, accessSize) | ||
| f.withWrapper(ctx) { | ||
| if (it.optimizedSizeof) { | ||
| callConverterMethod(OptimizedSizeOfConverter<Any, Any>::sizeOf, it, fieldAccess) | ||
| } else { | ||
| f.sizeOf(accessSize) | ||
| accessSize | ||
| } | ||
| } ?: f.sizeOf(accessSize) | ||
| } ?: accessSize |
There was a problem hiding this comment.
tidy up this call to encapsulate sizeOf in MessageSizeGenerator
244cf88 to
91c5da7
Compare
91c5da7 to
e387d2d
Compare
This reverts commit de960cc.
ogolberg
approved these changes
Nov 25, 2023
ogolberg
pushed a commit
to ogolberg/protokt
that referenced
this pull request
Jan 2, 2024
…s and those that would fail during deserialization on default-valued fields (open-toast#205) Prevents addition of fields that are marked as wrapped from breaking compiled code forwards compatibility when the wrapper type is not safe to use on an absent field. The three out-of-the-box converters that have this property are those for UUID, LocalDate (from String), and InetAddress. Protokt now has a property that Converters use to indicate their ability to deserialize protobuf default values. Protokt attempts to instantiate each wrapper type with the default value if the wrapped field is not nullable and not marked with the nonnull annotation; i.e., non-optional scalar fields like bytes, string, int32. If that call fails, protokt will insist that the Converter declares itself unable to deserialize the default type and then marks the field as nullable and so it does not attempt to wrap the "zero" instance if the field doesn't appear on the wire. The non_null option is now able to be applied to non-optional scalar fields in the case where they are wrapped by a converter that does not work on default values. It's a dangerous thing to do, but it's sufficiently hard to do this by accident and I could imagine a real use case for it. This PR also inverts the type arguments for Converter since I think it's confusing for the Kotlin type to be first with the Protobuf type second when I think of it more as "wrapping from Protobuf type to Kotlin type." Open to undoing this though - it's easy to flip back and forth. Maybe renaming the type parameters is sufficient.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prevents addition of fields that are marked as wrapped from breaking compiled code forwards compatibility when the wrapper type is not safe to use on an absent field. The three out-of-the-box converters that have this property are those for UUID, LocalDate (from String), and InetAddress.
Protokt now has a property that Converters use to indicate their ability to deserialize protobuf default values. Protokt attempts to instantiate each wrapper type with the default value if the wrapped field is not nullable and not marked with the nonnull annotation; i.e., non-optional scalar fields like
bytes,string,int32. If that call fails, protokt will insist that the Converter declares itself unable to deserialize the default type and then marks the field as nullable and so it does not attempt to wrap the "zero" instance if the field doesn't appear on the wire.The non_null option is now able to be applied to non-optional scalar fields in the case where they are wrapped by a converter that does not work on default values. It's a dangerous thing to do, but it's sufficiently hard to do this by accident and I could imagine a real use case for it.
This PR also inverts the type arguments for Converter since I think it's confusing for the Kotlin type to be first with the Protobuf type second when I think of it more as "wrapping from Protobuf type to Kotlin type." Open to undoing this though - it's easy to flip back and forth. Maybe renaming the type parameters is sufficient.