Defaults #297

mullermp · 2025-05-07T20:16:06Z

Handle the default trait.

Currently have two approaches to consider:

Handle via types. The biggest issue here is that input types are not used. It will require work to convert hashy input into types prior to serialization.

There are two sub-approaches - one that overrides initialize and the other that sets a defaults instance variable for Smithy::Schema::Structure to use implicitly.

Handle via codecs. This approach would not change anything major, but it requires all codecs to understand defaults. It also means that types do not have defaults built in. The codec would look at the default value and apply it if applicable.

(Note: we could implement both approaches but that seems wasteful)

I chose option 2 here after going down a rabbit hole. It seems cbor protocol should not serialize top level members on the client. Because of this, types would need to know protocol specific information. Protocols have client and server implementations, making it ideal to handle it at runtime as codecs (with a configurable option regarding top level).

I chose option 1 again, by using synthetic input/output. These synthetic shapes will have input/output traits, and if those traits are present, defaults generation does not apply.

gems/smithy-schema/lib/smithy-schema/structure.rb

This reverts commit ae78f16.

jterapin

Nice, I'll follow this PR format to update my Document PR to stay aligned. We should chat about what you mean by going with option 1.

gems/smithy-cbor/lib/smithy-cbor/deserializer.rb

gems/smithy-client/lib/smithy-client/signers/http_basic.rb

gems/smithy-client/lib/smithy-client/stubbing/data_applicator.rb

gems/smithy-json/spec/smithy-json/codec_spec.rb

alextwoods

Nice - looks good overall - I think the approach makes sense

alextwoods · 2025-05-13T20:59:24Z

gems/smithy-client/lib/smithy-client.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true

+require 'base64'
+require 'bigdecimal'


Should we be adding bigdecimal as a required, general dependency? If I remember it can cause some issues for users without compilers/jruby and SDKs dont generally support it. Maybe add it as a dependency in client gems only when the model requires it?

I think @hsbt has work to make it compatible everywhere. We can check.

alextwoods · 2025-05-16T15:49:41Z

gems/smithy-cbor/spec/smithy-cbor/codec_spec.rb

        it 'deserializes unknown union members' do
          unknown_union_type = shape.member(:union).shape.member_type(:unknown)
          data = { union: { 'someThing' => 'someValue' } }
          deserialized = subject.deserialize(shape, CBOR.encode(data))
          expect(deserialized.union).to be_a(unknown_union_type)
          expect(deserialized.union.to_h).to eq(unknown: { name: 'someThing', value: 'someValue' })
        end
-
-        it 'raises when deserializing unions with more than one member' do


Is there a reason we're removing these tests?

I think this was wrong behavior when I had chatted with Kevin. I'm not really sure if we should be raising. Let me think on this.

alextwoods · 2025-05-16T15:52:23Z

gems/smithy-client/lib/smithy-client/handler_context.rb

@@ -33,7 +33,7 @@ def initialize(options = {})
      # @return [Base]
      attr_accessor :client

-      # @return [Hash] The hash of request parameters.
+      # @return [Hash, Struct] The request parameters as a Hash or a Struct.


I assume Struct here would be the runtime input shape?

This is a little confusing - I assume the reason for the union of types here is that at the start of the request we have hash params and after the parameter conversion handler runs, we have runtime shapes?

Yeah, at the start of the request, we would have a hash OR struct, but eventually it will always be a struct. I'm not sure if we want to do this though - but it allows us to get values from types (like defaults) if we use the input type.

alextwoods · 2025-05-16T15:55:46Z

gems/smithy-client/lib/smithy-client/param_converter.rb


-            next unless ref.shape.member?(k)
+        type = @convert_structures ? ref.shape.type.new : values


When convert_structures is false, this should copy values right? Otherwise the input is being modified

dup is called by the converter prior to this function call if it's a hash.

alextwoods · 2025-05-16T16:13:04Z

gems/smithy/lib/smithy/views/client/types.rb

+
+          def default?
+            traits = @member.fetch('traits', {})
+            traits.key?('smithy.api#default') && !traits.key?('smithy.api#clientOptional')


This gets a little confusing for shape only gems / server generation - the smithy docs say

The clientOptional trait applied to a member marked with the default trait causes non-authoritative generators to ignore the default trait.

So, in a client context, we should use it, but in an authoritative (server) type we shouldn't. I'm not sure if theres a good way to handle that in a server/client agnostic generation though and since we're targeting client generation, maybe this is good for now.

Yeah. Good call out.. I'm not sure how else to handle this? I'll chat with Michael.

alextwoods · 2025-05-16T16:15:05Z

gems/smithy/lib/smithy/views/client/types.rb

+
+          def document(default)
+            case default
+            when nil then 'nil'


Should a check for nil default be done above and default just be skipped completely if its nil or is this a special document case?

This is a document case.

alextwoods · 2025-05-16T16:18:47Z

gems/smithy/lib/smithy/welds/synthetic_input_output.rb

+        input_shape = {
+          'type' => 'structure',
+          'traits' => target.fetch('traits', {}).merge({ 'smithy.api#input' => {} }),
+          'members' => target.fetch('members', {}).merge({})


Do we need to do a deep copy of members and traits so that another weld that modifies the synthetic shape don't modify the original. I know the merge will do a shallow copy and I can't remember the details of how these are structured so this may not apply.

Yes. This was on my radar. We need a deep copy.

alextwoods · 2025-05-16T16:20:38Z

gems/smithy/lib/smithy/welds/synthetic_input_output.rb

+module Smithy
+  module Welds
+    # Creates synthetic input and output shapes for operations that do not have them.
+    class SyntheticInputOutput < Weld


I don't remember all of the history/details on this - but I remember with our java generator we had similar logic which we ended up removing because the logic was pushed up stream.

I believe it needs to be handled by us - the java ModelTransformer does this effectively for us, but I don't have that. The issue is that defaults only apply to shapes not input/output, but if you have synthetics, it not only helps with model evolution, but it allows you to apply defaults at the non-top-level scope which is required by protocol tests.

In thinking of speculative "typed document" cases - us manipulating the shape ids on our end will have a trickle effect when we receive a typed document from a service and the underlying discriminator does not match our structures within type registry (since we replaced the shape id with a new one) - unless we intend to keep both synthetic and modeled shapes.

Disregard this comment if this was handled already.

gems/smithy-schema/lib/smithy-schema/shapes.rb

jterapin · 2025-05-20T14:56:35Z

gems/smithy-schema/spec/support/schema_helper.rb

+              'traits' => {
+                'smithy.api#required' => {},
+                'smithy.api#clientOptional' => {}
+              }


Should we move this closer to the tests itself? Feedback: #282 (comment)

Probably could look over the sample_shapes to see if there's any opportunity to move test-specific traits closer.

jterapin · 2025-05-20T15:30:15Z

gems/smithy/lib/smithy/welds/synthetic_input_output.rb

+module Smithy
+  module Welds
+    # Creates synthetic input and output shapes for operations that do not have them.
+    class SyntheticInputOutput < Weld


In thinking of speculative "typed document" cases - us manipulating the shape ids on our end will have a trickle effect when we receive a typed document from a service and the underlying discriminator does not match our structures within type registry (since we replaced the shape id with a new one) - unless we intend to keep both synthetic and modeled shapes.

Disregard this comment if this was handled already.

mullermp added 3 commits May 7, 2025 13:59

WIP

5e4c880

Working output tests

a4aac5b

Regenerate

8db39a1

mullermp commented May 7, 2025

View reviewed changes

gems/smithy-schema/lib/smithy-schema/structure.rb Outdated Show resolved Hide resolved

mullermp added 6 commits May 7, 2025 16:31

Support clientOptional

688829d

Revert me: types approach

ae78f16

Revert "Revert me: types approach"

67fa77d

This reverts commit ae78f16.

Revert initialize approach

2220b93

Pass cbor protocol tests

7cffe7d

Rubocop

5d37bc0

mullermp marked this pull request as ready for review May 7, 2025 23:40

mullermp added 8 commits May 8, 2025 13:59

More fixes for defaults in AWS

9662011

Give prelude unit a type of empty structure

0b3bb17

Flip top level default option

7e1cadc

Remove prelude unit check

583e1a1

more options piping

8301c53

Fix json codec specs

3045d7f

Correct unions

927c26b

Fix cbor union specs

9cd1318

jterapin approved these changes May 10, 2025

View reviewed changes

mullermp added 2 commits May 12, 2025 21:14

Use input types and conversion, with synthetic input output shapes

3da4fa0

Fix CI

c759002

alextwoods reviewed May 16, 2025

View reviewed changes

mullermp added 2 commits May 16, 2025 18:33

synthetic shape tests

7f8c65a

Rubocop

748557a

mullermp merged commit 715fa2a into decaf May 17, 2025
17 checks passed

mullermp deleted the defaults branch May 17, 2025 14:36

jterapin reviewed May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defaults #297

Defaults #297

mullermp commented May 7, 2025 •

edited

Loading

jterapin left a comment

alextwoods left a comment

alextwoods May 13, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

alextwoods May 16, 2025

mullermp May 16, 2025

jterapin May 20, 2025

jterapin May 20, 2025

jterapin May 20, 2025


		next unless ref.shape.member?(k)
		type = @convert_structures ? ref.shape.type.new : values

Defaults #297

Defaults #297

Conversation

mullermp commented May 7, 2025 • edited Loading

jterapin left a comment

Choose a reason for hiding this comment

alextwoods left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mullermp commented May 7, 2025 •

edited

Loading