Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ internal class MessageTypeAdapter<M : Message<M, B>, B : Message.Builder<M, B>>(
// interpreted as the appropriate default value when parsed into a protocol buffer."
if (value == null) continue

jsonField.fieldBinding.set(builder, value)
jsonField.fieldBinding.set(builder, jsonField.fieldBinding.withoutStrayNullElements(value))
}
input.endObject()
return builder.build()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ internal class MessageJsonAdapter<M : Message<M, B>, B : Message.Builder<M, B>>(
if (value == null) continue

val fieldBinding = messageAdapter.fieldBindingsArray[index]
fieldBinding.set(builder, value)
fieldBinding.set(builder, fieldBinding.withoutStrayNullElements(value))
}
input.endObject()
return builder.build()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,23 @@ abstract class FieldOrOneOfBinding<M, B> {
if (isMap && syntax == Syntax.PROTO_3) return true
return false
}

/**
* Returns [value] with any null elements removed when it's the decoded JSON value for a repeated
* field. A literal `null` in a JSON array (e.g. `"items": [{...}, null]`) decodes to a null list
* element that isn't representable in a proto repeated field; left in place it would trip the
* `null !in result` check in [immutableCopyOf]. Struct fields (such as
* `repeated google.protobuf.NullValue`) are decoded by the struct adapter and may legitimately
* carry null entries, so they're returned unchanged.
*/
Comment on lines +93 to +100

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment sounds like an explanation that would live in this PR, but not in the code.
Simply explaining what it does is sufficient.

Suggested change
/**
* Returns [value] with any null elements removed when it's the decoded JSON value for a repeated
* field. A literal `null` in a JSON array (e.g. `"items": [{...}, null]`) decodes to a null list
* element that isn't representable in a proto repeated field; left in place it would trip the
* `null !in result` check in [immutableCopyOf]. Struct fields (such as
* `repeated google.protobuf.NullValue`) are decoded by the struct adapter and may legitimately
* carry null entries, so they're returned unchanged.
*/
/**
* Normalizes a decoded JSON field value before assigning it to a builder.
*
* For repeated, non-struct fields, null list elements are discarded because proto repeated fields
* cannot store null elements. Struct fields are left unchanged because `google.protobuf` struct
* values use null as a valid JSON value.
*/

Also let's rename the method to filterNullElements

fun withoutStrayNullElements(value: Any?): Any? {
if (!label.isRepeated || singleAdapter.isStructAdapter() || value !is List<*>) return value
if (null !in value) return value
return value.filterNotNull()
}

private fun ProtoAdapter<*>.isStructAdapter(): Boolean = this == ProtoAdapter.STRUCT_MAP ||
this == ProtoAdapter.STRUCT_LIST ||
this == ProtoAdapter.STRUCT_VALUE ||
this == ProtoAdapter.STRUCT_NULL
}
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,44 @@ class WireJsonTest {
assertThat(parsed).isEqualTo(expected)
}

/**
* A literal `null` element inside a repeated message field is not representable in proto, so it's
* dropped rather than passed to `Internal.immutableCopyOf` (which would throw `contains(null)`).
* https://github.com/square/wire/issues/3353
*/
@Test fun nullElementInRepeatedMessageFieldIsDropped() {
val json = """{"pizzas":[{"toppings":["pineapple"]},null]}"""
val expected = PizzaDelivery.Builder()
.pizzas(listOf(Pizza.Builder().toppings(listOf("pineapple")).build()))
.build()
val parsed = jsonLibrary.fromJson(json, PizzaDelivery::class.java)
assertThat(parsed).isEqualTo(expected)
}

/** Same as above but for a repeated scalar field. */
@Test fun nullElementInRepeatedScalarFieldIsDropped() {
val json = """{"toppings":["pineapple",null,"onion"]}"""
val expected = Pizza.Builder().toppings(listOf("pineapple", "onion")).build()
val parsed = jsonLibrary.fromJson(json, Pizza::class.java)
assertThat(parsed).isEqualTo(expected)
}

/**
* Struct values legitimately represent null entries (e.g. inside a `google.protobuf.ListValue`),
* so a null element in a struct list must be preserved rather than stripped along with the stray
* nulls above.
*/
@Test fun nullElementsInStructListArePreserved() {
if (jsonLibrary.writeIdentityValues) return

val json = """{"list":["a",null,3.0]}"""
val expected = AllStructs.Builder()
.list(listOf("a", null, 3.0))
.build()
val parsed = jsonLibrary.fromJson(json, AllStructs::class.java)
assertThat(parsed).isEqualTo(expected)
}

@Test fun enumCanBeDecodedFromInt() {
val json = """{"drink":9}"""
val value = jsonLibrary.fromJson(json, FreeDrinkPromotion::class.java)
Expand Down