Defaults by mullermp · Pull Request #297 · smithy-lang/smithy-ruby

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.

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.

alextwoods

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

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

 # 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

          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

      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

+        return if values.nil?

-            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

+
+          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

+
+          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

+        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

+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.

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

+              '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

+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.

Matt Muller added 3 commits May 7, 2025 13:59

WIP

5e4c880

Working output tests

a4aac5b

Regenerate

8db39a1

mullermp commented May 7, 2025

View reviewed changes

Comment thread gems/smithy-schema/lib/smithy-schema/structure.rb Outdated

Matt Muller 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

Matt Muller 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

Matt Muller 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

Matt Muller 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

Conversation

mullermp commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jterapin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alextwoods left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mullermp commented May 7, 2025 •

edited

Loading