Added Unknown keys feature - #3189
Conversation
|
Just a few days ago, I was thinking that I'd utilize this feature. My use case: parsing open-api specs. Additional props may be added to the object and my generator's plugin system may want to read that (those "UnifiedApiResponseAdditionsReturnedTypeResponseDto" : {
"properties" : {
"error" : {
"type" : "string"
},
"flashes" : {
"items" : {
"$ref" : "#/components/schemas/FlashMessage"
},
"type" : "array"
},
"result" : {
"$ref" : "#/components/schemas/AdditionsReturnedTypeResponseDto"
},
"traceId" : {
"type" : "string"
}
},
"required" : [ "flashes" ],
"type" : "object",
"x-unified-response" : true,
"x-unified-response-result-required" : true
} |
Yes, I think this feature would be useful in your case :) |
|
@sandwwraith @pdvrieze Please take a look |
pdvrieze
left a comment
pdvrieze
left a comment
There was a problem hiding this comment.
I've had a look at it. The implementation is correct, but it adds a degree of complexity. There are some bits where this complexity can be reduced.
A key consideration is that the collection serializers actually support incremental parsing (updating). This would allow you to just handle/parse unknown keys incrementally (rather than having to collect them first). At that point you can just return the index of the "other attributes" element, and in the relevant (map) decode function handle parsing it.
The one gotcha is handling the class discriminator that you need to exclude from returning the index.
As optimization, the schemaCache already has code to create a map from name to index. As part of initialization it loops through all elements and annotations to determine the effective name(s) - supporting @JsonNames. It would certainly be more efficient to check for the @JsonExtraKeys element at the same time. (I know that this is a bit against the idea of SchemaCache, but the design is more extensible than it needs to be for an internal type (keep in mind multi-threading issues though - don't make the value mutable))
| for (i in 0 until elementsCount) { | ||
| if (getElementAnnotations(i).any { it is JsonExtraKeys }) { | ||
| if (foundIndex == -1) { | ||
| foundIndex = i | ||
| } else { | ||
| val list = duplicates | ||
| if (list == null) { | ||
| duplicates = mutableListOf(getElementName(foundIndex), getElementName(i)) | ||
| } else { | ||
| list.add(getElementName(i)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| duplicates?.let { | ||
| throw SerializationException( | ||
| "Class '$serialName' has more than one property annotated with @JsonExtraKeys: " + | ||
| it.joinToString(", ") { name -> "'$name'" } + | ||
| ". At most one such property is allowed per class." | ||
| ) | ||
| } | ||
| if (foundIndex == -1) return -1 | ||
| validateJsonExtraKeysProperty(foundIndex) |
There was a problem hiding this comment.
Some of this code is doing more than is needed on the "happy path". I am wondering if a configuration option to disable expensive validation would be worthwhile (so it for example doesn't check for duplicates, and thus may start from the last index instead of the first).
There was a problem hiding this comment.
validateJsonExtraKeysProperty() run exactly once per descriptor per Json instance (not once per encode/decode or anything like that), so I don't think this is worth trying to optimize.
|
@pdvrieze @sandwwraith I've made some significant changes as per suggestions, please take a look when you have a moment |
|
Thanks for this PR, IMO this is a must-have feature of any JSON library as this is the only way to ensure safe round-tripping. |
|
@sandwwraith @fzhinkin Do you think we can try to get this into the next release (maybe under experimental?) This would be very handy for some of our migrations. |
sandwwraith
left a comment
sandwwraith
left a comment
There was a problem hiding this comment.
I've looked through the implementation and have some general comments:
- Since this feature involves some logic on the hot parser path, it would be wise to put it under the feature flag, as it was done with
useAlternativeNames. - You need to have benchmarks with both states of this flag to verify that it doesn't slow down things a lot when enabled by default. Take a look at
TwitterFeedBenchmarkorCoerceInputValuesBenchmarkfor examples - If this flag would be implemented, there would be no need to care for weird try-catches around broken descriptors in
SerialDescriptor.jsonExtraKeysIndex - Why
Map<String, T>was chose instead of a simpleJsonObject? It seems highly inlikely to me that someone would not know what keys are there, but would know their shape. Using JsonObject for the unknown keys is the most straightforward way which also would likely simplify implementation. - Writing extra keys back. I understand this is a nice feature to have, but I'm not sure it is mandatory. It makes a lot of complications in encoding to flatten the keys. Again, if unknown keys holder is strictly a JsonObject, we could simply dump its .toString() onto the current stream and go on our way. Please investigate this approach, otherwise I doubt the benefits of having this functionality.
Fair points. I liked the encoding ability because if the user decided to serialize the deserialized object again (for logging or something) then the "extras" are spread back into JSON. For now, I will skip encoding this and document. |
Comparing this with XML (where the feature has been available for quite some time now) I would say that the ability to write back the additional keys would be relevant. In many ways, the extra entries would be opaque, so The biggest question is what to do when the unknownKeys map is updated/manipulated such that it contains overlapping keys. This would be an invalid state of the to-be-serialized object, but there is still the decision of how to handle it:
The undefined behaviour option is cheapest (it would be roughly dumping The other options do require either keeping track of written keys or looking up the key on the descriptor. There I would be inclined to go for the exception option as the others are surprising, especially if it ends up depending on the order of the elements in the structure (I would go to serialize the unknown keys after all "regular" keys). This can be implemented by recording the unknown keys element if it occurs (and is not empty), and on the endStructure writing the values (if needed). |
|
Yes I agree the ability to write back the unknown keys is necessary for this feature to be useful. And I agree it makes more sense to store the unrecognised keys in a |
Upon more thought, this does strike a good middle-ground. Will update PR shortly. |
|
@inemtsev Exceptions on duplicate keys is a good solution. The main reason to avoid it is if you want to avoid the check for a duplicate, but given the potential for duplicate keys to cause security challenges (inconsistent values used with different checks/systems) it is probably worthwhile to check for duplicates (and throw). |
BenchmarksJMH, JDK 21 (arm64 macOS). Micro: Hot path — classes without
|
| Scenario | master | this PR, useExtraKeys = false (default) |
this PR, useExtraKeys = true |
|---|---|---|---|
| decode 10-field object | 3.33 ± 0.12 | 3.51 ± 0.40 | 3.39 ± 0.31 |
| encode 10-field object | 10.13 ± 0.26 | 10.27 ± 0.37 | 9.84 ± 0.42 |
| decode TwitterFeed (macro) | 1562 ± 12 | 1596 ± 47 — parity | 1449 ± 10 (−7%) |
| encode TwitterFeed (macro) | 3221 ± 86 | 3223 ± 94 — parity | 3046 ± 113 (−5%) |
Feature cost — classes with a @JsonExtraKeys bucket (useExtraKeys = true)
| Scenario | Map<String, JsonElement> bucket | JsonObject bucket | Reference |
|---|---|---|---|
| decode, input has no unknown keys | 3.62 ± 0.10 | 3.61 ± 0.33 | ≥ plain decode: declaring a bucket is free on clean input |
| decode, capture 3 unknown keys | 1.76 ± 0.16 | 1.64 ± 0.12 | 2.54 ± 0.10 with ignoreUnknownKeys (keys dropped) |
| encode, write back 3 captured keys | 4.64 ± 0.10 | 4.67 ± 0.16 | 10.27 plain encode (10 keys vs 13 keys written) |
Summary
Looks like with flag off the cost is basically free. But, enabling the flag costs about 5-7%. Probably keeping it false, unless needed is ideal?
|
@sandwwraith I pushed changes listed above. I also discovered an optimization that would help operations like this, but I will open another PR for that: |
…JsonExtraKeys is used, now near 0 allocation
JsonExtraKeysBenchmark covers the plain hot path under both useExtraKeys states, bucket declaration on clean input, capture, and write-back. TwitterFeedBenchmark gains enabled-state variants for macro payloads; the stock decodeTwitter/encodeTwitter measure the default (disabled) state.
…k in decodeObjectIndex The per-key call into extraKeysIndexFor cost ~2% on TwitterFeed decode against the current dev base. Branching on the flag directly and going straight to decodeObjectIndexNoBucket restores parity (verified with paired sequential JMH runs vs dev).
5 participants
Details
This change introduces a JSON-only, runtime-level feature for collecting additional properties into a dedicated bucket property annotated with @JsonExtraKeys.
Supported behavior:
Map<String, V>where V is any@Serializabletype or a contextually serializable type like JsonElement, primitives, enums, classes, and polymorphic / sealed hierarchies are supportedignoreUnknownKeys/@JsonIgnoreUnknownKeysJsonEncodingExceptionis thrown on encode if the bucket mapcontains a key that would collide with either a declared property's
JSON name (including @JsonNames aliases and naming-strategy translations)
or the active class discriminator
Validation rules:
@JsonExtraKeysproperty per classMap<String, V>with the standard kotlinx.serialization Map serializer; the key serializer must be the standard String serializer (inline value classes wrapping String are rejected because the encode path casts each entry key to String)@JsonNamesis rejected on the bucket propertyImplementation notes:
JsonExtraKeysSpreadingEncoderfor streaming,JsonExtraKeysSpreadingTreeEncoderfor tree) that drive the user'sMapSerializerthrough the alternating-index encodeSerializableElement protocol; values are routed through the parent encoder's encodeSerializableValue so polymorphic-discriminator setup and the JsonElement short-circuit both applyKSerializer<Map<String, V>>that doesn't follow the alternating key/value index protocol is unsupported by design since the wrapper extends AbstractEncoder, whose default encodeValue throws SerializationException on any unexpected primitive callTicket: #1978