cast from Array(String) to String failed from Avram::Params

[jwoertink]

In #847 I had to change Avram::Params to take Hash(String, Array(String)) instead of Hash(String, String) so that we can allow array params to be passed in. This also aligns with URI::Params.

I wasn't able to make it a union because this caused a LOT of compiler headache trying to determine which type a value was, and constantly having to cast it.

To use this, you would do

params = Avram::Params({"username" => ["tacoguy"]})
SaveUser.create(params) do |_,_|
end

As it turns out, the specs never caught this because in the specs, we just inherit from Avram::Params

avram/spec/avram/params_spec.cr

Lines 21 to 37 in 9c8caec

	private class NestedParams < Avram::Params
	def nested?(key : String) : Hash(String, String)
	if @hash.keys.map(&.split(':').first).includes?(key)
	super
	else
	{} of String => String
	end
	end
	def nested_arrays?(key : String) : Hash(String, Array(String))
	if @hash.keys.map(&.split(':').first).includes?(key)
	super
	else
	{} of String => Array(String)
	end
	end
	end

The error actually borks on

avram/src/avram/add_column_attributes.cr

Line 17 in 9c8caec

set_{{ attribute[:name] }}_from_param(value.as(String)) if key == {{ attribute[:name].stringify }}

because in this case, value is Array(String) even though the column should just be String. The tricky part is typeof(value) here is actually Array(String) | String which ends up coming back to the original union issue. We need some sort of way to say that if value gets to this branch, and value is still an array, it needs to call .first, but if it's not an array, then make sure to cast as a String.

[jwoertink]

Ok, it turns out it's here

avram/src/avram/params.cr

Lines 29 to 31 in 9c8caec

	def nested_arrays(key : String) : Hash(String, Array(String))
	@hash
	end

I'm just saying that the nested_arrays is the raw passed in hash. Getting the permitted keys we pull data twice from params

avram/src/avram/add_column_attributes.cr

Lines 24 to 26 in 9c8caec

	single_values = @params.nested(self.class.param_key).reject {|k,v| k.ends_with?("[]")}
	array_values = @params.nested_arrays?(self.class.param_key) || {} of String => Array(String)
	new_params = single_values.merge(array_values)

when we merge them, if the nested_arrays is the raw hash, then it overrides the single_values.

Kinda like doing {"username" => "tacoguy"}.merge({"username" => ["tacoguy"]}).

[jwoertink]

So it seems that this doesn't affect normal operations...

This seems to work fine:

params = Avram::Params({"username" => ["tacoguy"]})
SignInUser.run(params) do |_,_|
end

[akadusei]

I'm running into the same issue. If it helps, here a trace:

cast from Array(String) to String failed, at /path/to/lucille/spec/support/app/src/models/user.cr:20:3:31 (TypeCastError)
         from spec/support/app/src/models/user.cr:20:3 in 'extract_changes_from_params'
         from spec/support/app/src/operations/create_user.cr:1:1 in 'set_attributes'
         from spec/support/app/src/operations/create_user.cr:1:1 in 'initialize'
         from spec/support/app/src/operations/create_user.cr:1:1 in 'new:active_at:inactive_at:id:email:level:metadata:height'
         from spec/support/app/src/operations/create_user.cr:1:1 in '->'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/example.cr:45:13 in 'internal_run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/example.cr:32:73 in '->'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/example/procsy.cr:16:15 in 'run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:365:11 in '->'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/example/procsy.cr:16:15 in 'run'
         from lib/avram/src/avram/spec_helper.cr:30:5 in 'wrap_spec_in_transaction'
         from spec/spec_helper.cr:12:1 in '->'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:71:26 in 'run_around_each_hook'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:66:7 in 'internal_run_around_each_hooks'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:59:7 in 'run_around_each_hooks'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:357:13 in 'run_around_each_hooks'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/example.cr:32:15 in 'run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:18:23 in 'internal_run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:339:7 in 'run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:18:23 in 'internal_run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/context.cr:156:7 in 'run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/spec/dsl.cr:220:7 in '->'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/crystal/at_exit_handlers.cr:14:19 in 'run'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/crystal/main.cr:50:5 in 'exit'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/crystal/main.cr:45:5 in 'main'
         from /path/to/crenv/versions/1.4.0/share/crystal/src/crystal/main.cr:127:3 in 'main'
         from /lib/x86_64-linux-gnu/libc.so.6 in '__libc_start_main'
         from /path/to/.cache/crystal/crystal-run-spec.tmp in '_start'
         from ???

[jwoertink]

To get around this for now, I just inherited from Avram::Params, and overrode the methods to do what I needed.

# TODO: https://github.com/luckyframework/avram/issues/884
class GraphParams < Avram::Params
  def nested?(key : String) : Hash(String, String)
    @hash.select(&.ends_with?("[]").!).transform_values(&.first)
  end

  def nested_arrays?(key : String) : Hash(String, Array(String))
    @hash.select(&.ends_with?("[]")).transform_keys(&.chomp("[]"))
  end
end

GraphParams.new({
  "username" => ["tacoguy"],
  "meats[]" => ["pollo", "carne"]
})

It's a bit wonky having to add [] on to the key, but technically that's how it's passed through params anyway... but if I can get it to work without needing that, I'll try to figure that out. The main part is trying to decipher between what should be considered a single value, and what should be multiple values. This may end up just coming back to allowing a Union here of Hash(String, Array(String) | String), and then just dealing with casting .as(String) or .as(Array(String)) all over the place 😝

[akadusei]

This seems to fix it:

1. Change Avram::AddColumnAttributes#extract_changes_from_params to:

   private def extract_changes_from_params
     permitted_params.each do |key, value|
       {% for attribute in attributes %}
         set_{{ attribute[:name] }}_from_param(value.as(Array(String))) if key == {{ attribute[:name].stringify }}
       {% end %}
     end
   end

   We don't need the is_a?(Generic) check, since we are already defining #set_{{ attribute[:name] }}_from_param conditionally.

2. Change Avram::AddColumnAttributes#set_{{ attribute[:name] }}_from_param to:

   # ...
   {% else %}
   def set_{{ attribute[:name] }}_from_param(_value : Array(String))
     _value = _value.first?
   
     # In nilable types, `nil` is ok, and non-nilable types we will get the
     # "is required" error.
     if _value.blank?
       {{ attribute[:name] }}.value = nil
       return
     end
   
     parse_result = {{ attribute[:type] }}.adapter.parse(_value.not_nil!)
   
     if parse_result.is_a? Avram::Type::SuccessfulCast
       {{ attribute[:name] }}.value = parse_result.value.as({{ attribute[:type] }})
     else
       {{ attribute[:name] }}.add_error "is invalid"
     end
   end
   {% end %}
   #...

I haven't tested this with the specs though, just fiddling with code in my lib/ directory.

[akadusei]

avram/src/avram/add_column_attributes.cr

Line 24 in 9c8caec

single_values = @params.nested(self.class.param_key).reject {|k,v| k.ends_with?("[]")}

This should probably change too, to return an array with a single element:

single_values = @params.nested(self.class.param_key).reject {|k,v| k.ends_with?("[]")}.transform_values { |value| [value] }

And, just a question, shouldn't the .reject...ends_with? part be already handled by Lucky::Params?