fix: sparse struct serializer

[0marperez]

Issue #

N/A

Description of changes

Fix rendering of sparse structure serializers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lauzadis]

simplification: Consider using writer.wrapBlockIf to handle isSparse like we do in other parts of this class

[ianbotsf]

Correctness: Needs unit tests

[ianbotsf]

Looks good!

[lauzadis]

correctness: should value be $valueName and key be $keyName?

[lauzadis]

Same comment here about keyName, valueName

[lauzadis]

possible simplification using wrapBlockIf:

writer.withBlock("$containerName$listMemberName.forEach { ($keyName, $valueName) ->", "}") {
    writer.wrapBlockIf(isSparse, "if ($valueName != null) {", "} else entry($keyName, null as String?)") {
        writer.write("entry($keyValue, $valueName.#T())", RuntimeTypes.Core.Text.Encoding.encodeBase64String)
    } 
}

[0marperez]

It's a good simplification but it would cause codegen changes to the serializers that are not sparse when not necessary

[lauzadis]

I'm not seeing the codegen change, unless you are you talking about the newline?

Non-sparse currently:

$containerName$#listMemberName.forEach { ($keyName, $valueName) -> entry($keyValue, $valueName.encodeBase64String()) }

Non-sparse with my suggestio:

$containerName$listMemberName.forEach { ($keyName, $valueName) ->
    entry($keyValue, $valueName.encodeBase64String())
}

[0marperez]

Yeah I'm talking about the new line

[lauzadis]

Ok, well if you think it's a good simplification, I don't think the newline makes much of a difference. Just a suggestion, up to you

[lauzadis]

question: Here we are assuming the value will always be a string (null as String?), I think that works because serializers will handle a null entry of any type the same way?

Is there a way to make this use the correct value type?

[0marperez]

question: Here we are assuming the value will always be a string (null as String?), I think that works because serializers will handle a null entry of any type the same way?

Yes that's right, there should be a way to use the correct value type but it's not simple and the results would be the same

[0marperez]

Sorry, actually that's only true for the JsonSerializer because of the XmlSerializer we need to use as String?

smithy-kotlin/runtime/serde/serde-xml/common/src/aws/smithy/kotlin/runtime/serde/xml/XmlSerializer.kt

Lines 231 to 263 in 7d2012a

	override fun entry(key: String, value: Int?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Long?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Float?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: String?): Unit = writeEntry(key) { xmlWriter.text(value ?: "") }
	override fun entry(key: String, value: SdkSerializable?): Unit = writeEntry(key) {
	if (value == null) {
	xmlWriter.text("")
	return@writeEntry
	}
	xmlSerializer.parentDescriptorStack.push(descriptor)
	value.serialize(xmlSerializer)
	xmlSerializer.parentDescriptorStack.pop()
	}
	override fun entry(key: String, value: Double?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Boolean?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Byte?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Short?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Char?): Unit = writeEntry(key) { xmlWriter.text(value.toString()) }
	override fun entry(key: String, value: Instant?, format: TimestampFormat): Unit = entry(key, value?.format(format))
	override fun entry(key: String, value: Document?) =
	throw SerializationException("document values not supported by xml serializer")

null.toString() returns the string "null" according to the docs

[github-actions]

Affected Artifacts

Significantly increased in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
telemetry-api-jvm.jar	121,407	101,389	20,018	19.74%
aws-protocol-core-jvm.jar	24,042	22,705	1,337	5.89%

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
runtime-core-jvm.jar	878,947	878,670	277	0.03%
http-client-jvm.jar	330,654	330,607	47	0.01%