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
| class Document; | ||
| class DocumentImpl; | ||
| namespace detail { | ||
| Document MakeDocument(std::shared_ptr<const DocumentImpl> impl); |
There was a problem hiding this comment.
so this looks like a pimpl but it's not really one. DocumentImpl.h is public, AsList and AsMap hand back pointers into its storage, and detail::MakeDocument lets anyone make one anyway. i think what's really going on is there's an interface hiding in here.
in smithy-java Document is an interface and each codec has its own implementation. the json deserializer makes its documents with JsonDocuments.of(value, settings) and that's how they know to base64 decode strings and parse timestamps. the From statics here return Document by value so it has to be a concrete class, and the interface ends up buried behind virtual calls on DocumentImpl. that's also why you can have two documents that are == but give you different answers from AsBlob.
i think we should just make it an interface and let each codec own its document type, something like
// Document.h
class SMITHY_API Document {
public:
virtual ~Document() = default;
virtual Aws::Crt::Optional<Aws::Utils::ByteBuffer> AsBlob() const = 0;
static std::shared_ptr<const Document> FromString(Aws::String value);
};
// Document.cpp
namespace {
class PlainDoc final : public Document { /* only returns a blob if it is a blob */ };
}
// JsonShapeDeserializer.cpp
namespace {
class JsonDoc final : public Document { /* base64 decodes the string */ };
}and using it would look like
// built by hand, no protocol rules
auto plain = Document::FromString("aGk=");
plain->AsBlob(); // empty
// read off the wire, the codec passes its settings to the deserializer which makes a JsonDoc
JsonCodec codec{settings};
auto deserializer = codec.CreateDeserializer(body);
auto parsed = deserializer->ReadDocument(schema);
parsed->AsBlob(); // {'h','i'}that way json's rules live with json and cbor can do its own thing later.
Document was shaped like a PIMPL but was not one: DocumentImpl was a public header, AsList/AsMap handed back pointers into its storage, and detail::MakeDocument let any caller build a Document over any impl. Coercion behavior was therefore smuggled through a hidden interface -- two documents could compare equal yet disagree on AsBlob(). Document is now a public abstract interface. AbstractDocument (internal, not installed) holds the tagged storage and the protocol-independent behavior, leaving AsBlob/AsTimestamp pure so a node cannot exist without a coercion rule. The concrete leaves live in Document.cpp's anonymous namespace: PlainDocument applies no reinterpretation and backs the From* factories, while JsonDocument base64-decodes strings and gates timestamp coercion on the configured format. The JSON deserializer builds its nodes through NewJsonDocument, so neither leaf is nameable outside that translation unit. ReadDocument now returns shared_ptr<const Document> and document collections carry shared_ptr<const Document>. Coercion, timestamp-format gating, CBOR trait-ignore adherence, and the numeric overflow guards are unchanged.
This branch has not been deployed
3 participants
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.