Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten shapes when fetching with Model.shape #269

Merged
merged 16 commits into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,27 @@ namespace :smithy do

desc 'Convert all fixture smithy models to JSON AST representation.'
task 'sync-fixtures' do
smithy_build_files = Dir.glob('gems/smithy/spec/fixtures/**/smithy-build.json')
Dir.glob('gems/smithy/spec/fixtures/**/model.smithy') do |model_path|
out_path = model_path.sub('.smithy', '.json')
sh("smithy ast --aut #{model_path} > #{out_path}")
config_files = smithy_build_files.map do |file|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there ever be multiple smithy build files? That seems maybe invalid?

Copy link
Contributor

@mullermp mullermp Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AST command takes a config file and it says multiple ones are merged. The intent was to allow this to be at all folder hierarchies.

" --config #{file}" if model_path.include?(File.dirname(file))
end
sh("smithy ast#{config_files.join(' ')} #{model_path} > #{out_path}")
end
end

desc 'Validate that all fixtures JSON models are up to date.'
task 'validate-fixtures' do
require 'json'
failures = []
smithy_build_files = Dir.glob('gems/smithy/spec/fixtures/**/smithy-build.json')
Dir.glob('gems/smithy/spec/fixtures/**/model.smithy') do |model_path|
old = JSON.load_file(model_path.sub('.smithy', '.json'))
new = JSON.parse(`smithy ast --aut #{model_path}`)
config_files = smithy_build_files.map do |file|
" --config #{file}" if model_path.include?(File.dirname(file))
end
new = JSON.parse(`smithy ast#{config_files.join(' ')} #{model_path}`)
failures << model_path if old != new
end
if failures.any?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,7 @@ def validate(params, expected_errors = [])
def match_errors(error, expected_errors)
expected_errors = [expected_errors] unless expected_errors.is_a?(Array)
expected_errors.each do |expected_error|
if expected_error.is_a?(String)
expect(error.message).to include(expected_error)
else
expect(error.message).to match(expected_error)
end
expect(error.message).to include(expected_error)
end
end

Expand Down
17 changes: 9 additions & 8 deletions gems/smithy/lib/smithy/model.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require_relative 'model/flattener'
require_relative 'model/rbs'
require_relative 'model/shape'
require_relative 'model/operation_parser'
Expand Down Expand Up @@ -37,15 +38,15 @@ module Model
}.freeze

# @param [Hash] model Model
# @param [String] target Target shape
# @return [Hash] The shape
def self.shape(model, target)
if model['shapes'].key?(target)
model['shapes'][target]
elsif PRELUDE_SHAPES.key?(target)
PRELUDE_SHAPES[target]
# @param [String] id Shape ID
# @return [Hash]
def self.shape(model, id)
if model['shapes'].key?(id)
Flattener.new(model).shape(id)
elsif PRELUDE_SHAPES.key?(id)
PRELUDE_SHAPES[id]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possible interaction between mixins and prelude shapes? I don't think there would be, but just want to check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so but I'll check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this: Cannot apply smithy.api#mixin to an immutable prelude shape defined in smithy.api.

else
raise ArgumentError, "Shape not found: #{target}"
raise ArgumentError, "Shape not found: #{id}"
end
end
end
Expand Down
114 changes: 114 additions & 0 deletions gems/smithy/lib/smithy/model/flattener.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
# frozen_string_literal: true

module Smithy
module Model
# @api private
class Flattener
def initialize(model)
@model = model
end

def shape(id)
shape = @model['shapes'][id]
return shape unless shape['mixins']

shape['mixins'].reverse_each do |mixin|
mixin_shape = shape(mixin['target'])
shape = deep_merge(mixin_shape, shape, exclude_traits(mixin_shape))
apply_traits(id, shape)
shape.delete('mixins')
end

shape
end

private

def exclude_traits(shape)
[
'smithy.api#mixin',
*shape.fetch('traits', {}).fetch('smithy.api#mixin', {}).fetch('localTraits', [])
]
end

def deep_merge(hash1, hash2, exclude_traits = [], context = nil)
hash1 = hash1.dup
if hash1['traits']
hash1['traits'] = hash1['traits'].except(*exclude_traits)
hash1.delete('traits') if hash1['traits'].empty?
end
deep_merge!(hash1, hash2, exclude_traits, context)
end

def deep_merge!(hash1, hash2, exclude_traits, context)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference between deep_merge and deep_merge! is confusing - maybe these could use comments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One returns a new hash and the other modifies the current hash, just like merge and merge!. I got inspiration from the rails implementations. I can add that here.

hash1.merge!(hash2) do |key, v1, v2|
if v1.is_a?(Hash) && v2.is_a?(Hash)
deep_merge(v1, v2, exclude_traits, key)
elsif v1.is_a?(Array) && v2.is_a?(Array) && context != 'traits'
# Merge arrays, but only if the key is not a trait
v1 + v2
else
v2
end
end
end

def apply_traits(id, shape)
case shape['type']
when 'structure'
structure(id, shape)
when 'union'
union(id, shape)
when 'list'
list(id, shape)
when 'map'
map_key(id, shape)
map_value(id, shape)
end
end

def structure(id, shape)
shape['members'].each do |member_name, member_shape|
member_id = "#{id}$#{member_name}"
next unless apply_shape_exists?(member_id)

apply_shape = shape(member_id)
member_keys = shape['members'][member_name].keys
shape['members'][member_name] = deep_merge(member_shape, apply_shape).slice(*member_keys)
end
end
alias union structure

def list(id, shape)
member_id = "#{id}$member"
return unless apply_shape_exists?(member_id)

apply_shape = shape(member_id)
member_keys = shape['member'].keys
shape['member'] = deep_merge(shape['member'], apply_shape).slice(*member_keys)
end

def map_key(id, shape)
key_id = "#{id}$key"
return unless apply_shape_exists?(key_id)

key_shape = shape(key_id)
key_keys = shape['key'].keys
shape['key'] = deep_merge(shape['key'], key_shape).slice(*key_keys)
end

def map_value(id, shape)
value_id = "#{id}$value"
return unless apply_shape_exists?(value_id)

value_shape = shape(value_id)
value_keys = shape['value'].keys
shape['value'] = deep_merge(shape['value'], value_shape).slice(*value_keys)
end

def apply_shape_exists?(id)
@model['shapes'][id] && @model['shapes'][id]['type'] == 'apply'
end
end
end
end
3 changes: 0 additions & 3 deletions gems/smithy/spec/fixtures/README.md

This file was deleted.

Loading
Loading