Implement Typed Documents and TypeRegistry #282
Conversation
jterapin
changed the title
jterapin
changed the title
| # shape = Smithy::Schema::StructureShape.new | ||
| # data = Document::Data.new({ "name" => "example" }, shape: shape) | ||
| # | ||
| module Document |
There was a problem hiding this comment.
I was expecting document to be a class and have it be the delegator. What was the intention of making another data subclass?
| # @param [Hash<String, Shapes::StructureShape>] registry | ||
| def initialize(registry = {}) | ||
| @registry = registry | ||
| @shapes_by_type = register_shape_types(registry.values) |
There was a problem hiding this comment.
Is there a way to populate this from the code generated side? If we must iterate shapes, we may as well backtrack and populate both maps in one pass. That at least reduces generated code. Otherwise is shapes_by_type even necessary?
|
|
||
| # @api private | ||
| # @return [Hash<String, Shapes::StructureShape>] | ||
| attr_accessor :registry |
There was a problem hiding this comment.
Both of these accessors shouldn't exist. Our public methods should hide this detail.
| end | ||
|
|
||
| def typed_document?(values) | ||
| (values.is_a?(Smithy::Schema::Structure) && @type_registry.shape_by_type(values.class)) || |
There was a problem hiding this comment.
Isn't this already checked? And wouldn't this always be true if it was a structure, because it would already be registered?
| ref.shape.member(name) || find_member_ref_by_names(ref, name) | ||
| end | ||
|
|
||
| def find_member_ref_by_names(ref, name) |
There was a problem hiding this comment.
This seems inefficient. Similar to what we do in codecs, for structure and union, you will want to iterate the shape members and not the values, then you can check json name that way. You're doing a loop for every member, so it's n^2 performance.
| end | ||
| end | ||
|
|
||
| def resolve_member_name(member_ref, opts) |
There was a problem hiding this comment.
Check out the location_name approach in my PR - we should use similar terms. You can easily handle this with || optionality.
| describe Serializer do | ||
| let(:shapes) do | ||
| shapes = SchemaHelper.sample_shapes | ||
| shapes['smithy.ruby.tests#Structure']['members']['timestampDateTime'] = { |
There was a problem hiding this comment.
I would prefer if you move these definitions closer to the test (in the actual tests where they are needed) - it's easier to manage tests that way if they are discrete.
There was a problem hiding this comment.
Nice - its generally looking good.
I think the functionality from the Document::Data class could be moved into Document as a class (unless theres some reason I'm missing). I also understand why the Document serializer and deserializer exist separately and require a type registery - but I think I would lean towards the public interface for serializing/deserializing documents living on the top level class - it could still require a type registry to be provided and could use these classes under the hood to implement it (and they could then be api private).
|
That's effectively what I was also saying but I agree. |
Description: Implementation for Typed Documents and TypeRegistry. Currently only supports JSON documents.
It is highly likely that we have to revisit this implementation in the future since typed document/type registry still being evolved.