Conversation
| /** | ||
| * Enables lenient decoding of numeric values, where numbers may be carried by JSON strings | ||
| * as well as JSON numbers. | ||
| */ | ||
| def withLenientNumericDecoding: JsoniterCodecCompiler |
There was a problem hiding this comment.
question: shall we just call it lenient decoding, in case we add more cases of leniency? such as, case-insensitivity (which might not be doable if there are members with names like foo and FOO, but might be possible in many cases)
There was a problem hiding this comment.
and we'd just mention number handling because it happens to be the only thing for now
There was a problem hiding this comment.
I'd rather have more granular options than a "one size fits all" option.
modules/json/test/src/smithy4s/json/SchemaVisitorJCodecCustomisationTests.scala
Show resolved
Hide resolved
Co-authored-by: Jakub Kozłowski <kubukoz@gmail.com>
lewisjkl
left a comment
lewisjkl
left a comment
There was a problem hiding this comment.
Just the one potential thing in the comment below, otherwise looks good 👍
|
|
||
| def decodeValue(cursor: Cursor, in: JsonReader): Int = in.readInt() | ||
| final def decodeValue(cursor: Cursor, in: JsonReader): A = | ||
| if (allowJsonStringNumerics) { |
There was a problem hiding this comment.
It's just one if statement, but I think this could be moved off the hot path?
There was a problem hiding this comment.
Good question. In this case, it's hard : the problem is that due to the imperative nature of jsoniter, the logic is not compositional. If we want to get checks on the hotpath at all cost, that in an ideal world we'd have one class per combination of flags
- disallowed strings but allowed infinity
- allowed strings and allowed infinity
- disallowed strings and disallowed infinity
- allowed strings but disallowed infinity
Right now I'm accepting this boolean check as a compromise to avoid having to repeat inherently error-prone bits of logic in 4 different implementations. I'm not too happy about it but I'd rather live with it.
There was a problem hiding this comment.
I think I'm following, for some reason I thought that you could also do at the class level something like:
private val decodeV: (Cursor, JsonReader) => A = if (allowJsonStringNumerics) {
(cursor, in) => // ...
} else {
(cursor, in) => // ...
}
final def decodeValue(cursor: Cursor, in: JsonReader): A = this.decodeV(cursor, in)There was a problem hiding this comment.
I mean, maybe you're seeing something I'm not seeing. I'd be delighted if you attempted and managed to avoid the boolean check on the critical path without inducing a lot of repetition in the code 😄
There was a problem hiding this comment.
also there's the question of : is a method dispatch faster than a boolean check in this context ? not sure
There was a problem hiding this comment.
Yeah good point, it probably isn't worth worrying about, was mostly just asking to clarify my understanding
3 participants
This adds opt-in logic to allow decoding numeric values from JSON strings.
This also exposes a new method on the SimpleRestJsonBuilder to configure the underlying JSON codecs, without having to duplicate the smart-builder methods every time we add new options.
PR Checklist (not all items are relevant to all PRs)