Support for (virtual) array attributes

[oneiros]

I tried to setup a Avram::Operation with an array attribute:

class CreateRoles < Avram::Operation
  attribute role_names : Array(String)
end

Sadly this will not compile, because crystal does not currently support nested constants in generics. In #151 @jwoertink added array support for column values and found a nice workaround.

So as a first step, I tried to use this workaround and change

avram/src/avram/define_attribute.cr

Line 95 in af99911

parse_result = {{ type }}::Lucky.parse({{ name }}_param)

to

parse_result = {{ type }}.adapter.parse({{ name }}_param)

Then my app did compile successfully. I have not checked if it actually works yet, because I thought this would be a nice little fix I could prepare a PR for.

Sadly, I failed because when I tried to add specs, they blew up:

There was a problem expanding macro 'setter'

Code in src/avram/attribute.cr:4:3

 4 | setter :value
     ^
Called macro defined in macro 'macro_139913478760976'

 483 | macro setter(*names)

Which expanded to:

 > 1 |       
 > 2 |         
 > 3 |           def value=(@value)
                            ^----
Error: instance variable '@value' of Avram::Attribute(Array(String) | Nil) must be (Array(String) | Nil), not (String | Nil)

I think the reason is that the specs use Avram::Params which only supports a flat String => String mapping. If my theory is correct, my app compiles, because it uses Lucky::Params which does support nested structures.

If someone knows an easy fix, I would be happy to continue the work on a PR.

But more importanty I think this could be valuable input for luckyframework/lucky#1209

[wout]

I've been in the same place while testing file params. The way I worked around this was to create a subclass for Avram::Params just for testing purposes:

https://github.com/luckyframework/avram/pull/404/files#diff-dcf82d31916d537fe7e9380227ffc216

But that's obviously not an ideal solution. I think Avram::Params needs some work to bring it more in line with Lucky::Params.

[matthewmcgarvey]

I'd like to refactor Avram to be able to remove Avram::Paramable and Avram::Params. They are rough stand-ins for Lucky::Params. This won't be usable until luckyframework/lucky#1380 is finished but I'd like to cut the dependency and allow Avram::Attribute to be any type as long as it can extract the type from whatever object is passed in (like Lucky::Params)

[matthewmcgarvey]

Now that luckyframework/lucky#1380 is done, here's what we need to do:

• Update https://github.com/luckyframework/avram/blob/master/src/avram/paramable.cr to add the get_all? and get_all methods
• Update https://github.com/luckyframework/avram/blob/master/src/avram/params.cr to add some sort of fake implementation of those methods
• Add an overload of Avram::Attribute#extract to handle arrays and use this new method

I think that will get array attributes working

[matthewmcgarvey]

Unfortunately I was wrong. luckyframework/lucky#1380 added get_all at the top level but we need it under a nested key. A query param would look like example.com?post:tags[]=crystal&post:tags[]=lucky. Calling params.nested("post") returns a Hash(String, String) right now. We should probably get it to return another param object or something equivalent to JSON::Any...

@jwoertink Got any opinions on this?

[jwoertink]

params.many_nested is already a method. We should be using that within the operations itself.

[matthewmcgarvey]

@jwoertink many_nested returns Array(Hash(String, String)) it's meant for nested operations not a nested array of strings.

This is kind of why I think Lucky::Params needs to be overhauled. The API is pretty confusing

[jwoertink]

Oh that's right. Yeah, the params definitely need a bit of thought. @paulcsmith and I have tried to tackle it a few times, but it's not an easy problem to solve. I have some ideas that I think I sent over to you before, but I can send those again. I'm just limited on time at the moment to really sit down and attack it.

Does doing something like this work? params.nested("post").get_all("tags")? Probably not because nested() returns a Hash or something instead of returning a param object... 🤔 Oh, what about something like params.get_all("#{param_key}:tags")?

I think it all just goes back to the fact that Avram shouldn't care about HTTP Params. We should bring back this discussion luckyframework/lucky#727. Though, that's about just the Lucky side.

Let's figure out a time we can hop on a chat to go over a path for this. Then we can document that path, and figure out if there's an easy migration/change, or if it'll just be a single massive overhaul from the ground up (most likely lol)