Array param support for operations #847
Conversation
… the integration better with actual params. Starting work on the array support
jwoertink
changed the title
| @@ -1,6 +1,6 @@ | |||
| require "../../spec_helper" | |||
|
|
|||
| include LazyLoadHelpers | |||
There was a problem hiding this comment.
This wasn't being used.
| @@ -4,6 +4,7 @@ class Avram::UploadParams < Avram::Params | |||
| @uploads : Hash(String, Avram::UploadedFile) = {} of String => Avram::UploadedFile | |||
|
|
|||
| def initialize(@uploads) | |||
| @hash = {} of String => Array(String) | |||
There was a problem hiding this comment.
The specs wouldn't compile without this... How it was working before is magic....
| # This module creates methods for each column in a model that map | ||
| # to an `Avram::Attribute` as well as methods that fill those attributes | ||
| # with values that comes from params. | ||
| module Avram::AddColumnAttributes |
There was a problem hiding this comment.
This code was in both SaveOperation and DeleteOperation, but seemed to have a few slight differences. One being that it was generating a permitted_params method for every column, but that method never did anything at the compiler level. So this becomes a small refactor since I had to change how things are checked.
| 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) | ||
| new_params.select(@@permitted_param_keys) |
There was a problem hiding this comment.
This will use some autocast magic which becomes Hash(String, Array(String) | String). Maybe I should just add the return type?
| set_{{ attribute[:name] }}_from_param(value.as(Array(String))) if key == {{ attribute[:name].stringify }} | ||
| {% else %} | ||
| set_{{ attribute[:name] }}_from_param(value.as(String)) if key == {{ attribute[:name].stringify }} |
There was a problem hiding this comment.
If we don't cast these here, then typeof(value) is Array(String) | String, and the compiler starts to get confused
| {% if attribute[:type].is_a?(Generic) %} | ||
| def set_{{ attribute[:name] }}_from_param(_value : Array(String)) | ||
| parse_result = {{ attribute[:type] }}.adapter.parse(_value) | ||
|
|
||
| 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 | ||
| {% else %} | ||
| def set_{{ attribute[:name] }}_from_param(_value) | ||
| # 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) | ||
|
|
||
| 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 |
There was a problem hiding this comment.
This was just 1 method before that did a quick check if the type was Generic or not before parsing. However, I think there was a bug doing it that way. We call if _value.blank? because if it's an empty string, treat it as nil. But in the case of an empty array, that could be a very valid value, and shouldn't be treated as nil.
Now, if we still want that same functionality (treating [] as nil), then I can add that back in, but I think that may have been a mistake.
| attribute brand : String | ||
| end | ||
|
|
||
| describe "OperationParams" do |
There was a problem hiding this comment.
This spec file is intended to utilize Lucky directly now that Lucky is a dependency of Avram. We should be able to test real legit params instead of stubbing out params all over.
|
I'm going to merge this in so I can move on to a few other things, but if anyone comes across issues, let me know before next release, or open up an issue. |
Fixes #408
This PR adds in the ability to pass arrays through params in to operations. Prior to this, you could only set array values through named args, or manually by splitting a string or something like that.
Now you can do this:
In doing this, there was quite a bit that needed some refactoring. One major breaking change is that
Avram::Paramsnow requires that all Hash values areArray(String)instead ofString.Commit 15d7741 causes this error when I run specs:
This is the spec that causes the crash.
spec/avram/operations/define_attribute_spec.cr:227