Array support for params (or better error message...) about mosquito HOT 5 CLOSED

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?

from mosquito.

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.

from mosquito.

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.

from mosquito.

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?

from mosquito.

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 😊️.

from mosquito.