Array support for params (or better error message...)

[jwoertink]

I'm not sure if arrays can be supported, so if not, then I'm cool with just a better error message too..

Current, if you try to add an array type, you'll get the error

class SomeJob < Mosquito::QueuedJob
  params ids : Array(String)
  def perform
    # ...
  end
end

Error: unexpected token: "nil?"

This comes from this line

mosquito/src/mosquito/queued_job.cr

Line 49 in d0e40c3

@{{ parameter["name"] }} : {{ parameter["simplified_type"] }}?

because simplified_type is defaulted to nil and only set if the type declaration type is_a? Path, but it's not... it's a Generic.
The code ends up trying to make @ids : nil? which is a syntax error.

Which means this block is skipped

mosquito/src/mosquito/queued_job.cr

Lines 36 to 40 in d0e40c3

	if type.is_a? Union
	raise "Mosquito Job: Unable to generate a constructor for Union Types: #{type}"
	elsif type.is_a? Path
	simplified_type = type.resolve
	end

An easy solution could be just adding the else condition to raise an error saying the type isn't supported

Crystal version: 1.8.1
Mosquito Shard version: 1.0.0.rc3+git.commit.d0245eba8514e238c253267b0bdf7663c6b01590

[wout]

I've got a similar issue using an array value as the param:

web          | There was a problem expanding macro 'finished'
web          | 
web          | Called macro defined in macro 'inherited'
web          | 
web          |  64 | macro finished
web          | 
web          | Which expanded to:
web          | 
web          |  >  1 |         
web          |  >  2 |           def initialize; end
web          |  >  3 | 
web          |  >  4 |           def initialize(@keys : Array(String))
web          |  >  5 |           end
web          |  >  6 | 
web          |  >  7 |           # Methods declared in here have the side effect over overwriting any overrides which may have been implemented
web          |  >  8 |           # otherwise in the job class. In order to allow folks to override the behavior here, these methods are only
web          |  >  9 |           # injected if none already exists.
web          |  > 10 | 
web          |  > 11 |           
web          |  > 12 |             def vars_from(config : Hash(String, String))
web          |  > 13 |               
web          |  > 14 |                 @keys = deserialize_array(string)(config["keys"])
web          |  > 15 |               
web          |  > 16 |             end
web          |  > 17 |           
web          |  > 18 | 
web          |  > 19 |           
web          |  > 20 |             def build_job_run
web          |  > 21 |               job_run = Mosquito::JobRun.new self.class.job_name
web          |  > 22 | 
web          |  > 23 |               
web          |  > 24 |                 job_run.config["keys"] = serialize_array(string)(@keys.not_nil!)
web          |  > 25 |               
web          |  > 26 | 
web          |  > 27 |               job_run
web          |  > 28 |             end
web          |  > 29 |           
web          |  > 30 |         
web          |  > 31 |       
web          | Error: unexpected token: "("

The job:

class SomeJob < Mosquito::QueuedJob
  param keys : Array(String)
  ...
end

My solution in the meantime is to serialize to JSON.

[robacarp]

The mosquito serializer logic is certainly primitive. For this particular case, it's kind of straightforward to emit an error suggesting the developer do something else:

            elsif type.id =~ /Array|Hash/
              raise "Mosquito Job: Unable to build serialization for #{type}. Have you considered serializing using json?"

That looks like this:

Error: Mosquito Job: Unable to build serialization for Array(Int32). Have you considered serializing using json?

There's certainly a future in better serialization logic, but it's a lot of lift and I'm not sure the payoff is worth it at this time. I looked for a bit but I didn't see any better way in crystal macros to inspect the type to see if it's an array or a hash -- this problem would probably also rear if it were specified as an Enum or a Tuple. The logic for any of these things is trivial for the single case but significantly harder to generalize -- Avram has a lot of infrastructure around making this work.

In the mean time, is it reasonable to add an error message like that? Should it also regex on Tuple, Enum, or anything else?

[jwoertink]

I think it's reasonable to just add the error message for now with a suggestion.

Avram actually just checks {% if @type.is_a?(Generic) %} , but you can also do {% if @type < Array %}. Though opening up Array will end up leading to "How come X isn't supported" for other types, so having that error for unsupported types would be great either way.

What would be nice is something like Ruby's Marshal 😂 though, I guess JSON works here too? Is that what Sidekiq is doing?

[wout]

A better error is definitely an improvement. As @jwoertink says, opening the door for Array will inevitably lead to more requests like these.

That said, what about supporting simple serialization? Something along the lines of:

class SomeJob < Mosquito::QueuedJob
  param keys : Array(String), as: :json
end

Or perhaps:

class SomeJob < Mosquito::QueuedJob
  json_param keys : Array(String)
end

In the background that would call .from_json on the passed type with the variable (always String). It would even allow more complex data structures without additional work:

class SomeJob < Mosquito::QueuedJob
  json_param data : SomeData

  struct SomeData
    include JSON::Serializable
    ...
  end
end

I'm not sure, but maybe JSON serialization like that can be made to work on the current param macro anyway without breaking changes?

It's just an idea, I'm not trying to get you to work or anything 😊️.

[robacarp]

Yes, I think if I was going to support serialization it'd need to have some sort of interface module similar to what avram does.

I toyed around with like:

module Mosqito::Serializer(T)
   abstract serialize(input : T) : String
   abstract deserialize(input : String) : T
end

class KlassSerializer
   include Mosquito::Serializer(Klass)

   # ...
end

class Job < QueuedJob
  param data : Klass, serialize_with: KlassSerializer
end

It's a great idea and I'd like to implement it...but for now I think I'll just use Jeremy's suggestion to check for Array or maybe Generic so the error is less opaque and let it be for now.