Conversation
Adds an open-content document value type to the schema-serde runtime and wires it through the JSON/CBOR/XML/Query serializers and deserializers. - smithy::schema::Document: a copyable value handle over a polymorphic, AWS_CORE_LOCAL DocumentImpl. The agnostic base serves serialize-built (and future CBOR) documents; JsonDocumentImpl adds JSON string->blob and string/number->timestamp coercion, parameterized by the protocol-default timestamp format threaded from JsonCodec. - Serialization: JSON and CBOR emit documents; REST-XML, AWS/Query, and EC2/Query reject with a SerializationException. - Deserialization: JSON builds the document tree; CBOR and XML reject (CBOR ReadDocument consumes the value so sibling members stay aligned). - JSON numeric safety: ReadLong's double fallback rejects non-finite / out-of-range doubles instead of casting them (undefined behavior), while a plain-integer overflow still clamps to INT64_MAX/MIN; open-content document integer overflow preserves magnitude as a double; Document::AsFloat rejects finite doubles beyond float range (out-of-range double->float is undefined behavior). - JSON floats and doubles serialize with shortest-round-trip precision (float precision for floats, not the double expansion of their imprecision) and a decimal marker, so a document float or double round-trips without losing precision or type. Deferred (documented): map insertion order, int/bigInteger reporting, unsigned 64-bit, a nesting depth cap of 64, and the tagged-union node layout.
| class Schema; | ||
|
|
||
| // Per-protocol timestamp default threaded to a codec's serializer and deserializer. | ||
| struct CodecSettings { |
There was a problem hiding this comment.
Non-blocking: could we add useJsonName and useTimestampFormat booleans to CodecSettings, like smithy-java's JsonCodecbuilder. Fine as a follow-up.
| } | ||
|
|
||
| private: | ||
| static constexpr int MAX_DOCUMENT_DEPTH = 64; |
There was a problem hiding this comment.
MAX_DOCUMENT_DEPTH = 64 may be too low: cJSON (Aws::Utils::Document) allows 1000
| m_bool = value; | ||
| } | ||
|
|
||
| void DocumentImpl::SetInteger(int64_t value) { |
There was a problem hiding this comment.
SetInteger always reports ShapeType::Long, so a JSON 0 comes back as Long. I think it should report the first type that holds the value without loss, in this order: Integer, Long, BigInteger, Double, BigDecimal, like smithy-java (parsing, type mapping).
| // Per-protocol timestamp default threaded to a codec's serializer and deserializer. | ||
| struct CodecSettings { | ||
| explicit CodecSettings(TimestampFormatTrait::Format defaultTimestampFormat) : defaultTimestampFormat(defaultTimestampFormat) {} | ||
| TimestampFormatTrait::Format defaultTimestampFormat; |
There was a problem hiding this comment.
nit prolly should be m_timestampForamt
| Document MakeDocument(std::shared_ptr<const DocumentImpl> impl) { return Document(std::move(impl)); } | ||
| } // namespace detail | ||
|
|
||
| Document Document::Null() { return detail::MakeDocument(DocumentImpl::MakeNull()); } |
There was a problem hiding this comment.
feels like DocumentImpl:: we shoudlnt be accessing this things statically if we have a pointer to implementation in this class. it feels like we are side-caring static functions in polymorphic containter, where we should just be delegating to the underlying implementation.
| return HashingUtils::Base64Decode(*encoded); | ||
| } | ||
|
|
||
| Aws::Crt::Optional<Document> ReadDocument(const Schema&) override { return ReadDocumentValue(0); } |
There was a problem hiding this comment.
the other reads all skip the value before returning empty when they fail, like ReadLong and ReadBoolean both call SkipValue first. ReadDocument doesn't, so if ReadDocumentValue fails partway through we're left sitting in the middle of the value and the struct reader just keeps going from there. so something like
{"doc": [[[[ ...65 levels... ]]]], "name": "bob"}
hits the depth limit and returns empty, which is fine for doc, but then ReadStruct sees the leftover ], thinks the object is done, and name never gets read. no error either, the data's just missing. pretty niche since you need a 64+ deep document but it's an easy fix to match the other reads
Aws::Crt::Optional<Document> ReadDocument(const Schema&) override {
const size_t start = m_pos;
auto doc = ReadDocumentValue(0);
if (!doc.has_value()) {
m_pos = start;
SkipValue();
}
return doc;
}might also be worth adding a member after the document in ReadDocumentRejectsExcessiveNesting, since right now it only checks a document on its own
Issue #, if available:
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.