[WIP] Birth of the new ParamSerializable objects.

[jwoertink]

Purpose

New Lucky::ParamSerializable module which takes param data incoming from a user, and serializes it in to usable objects. The values in this object become "permitted" params based on how the data was passed in.

Description

This is the start to moving param permissions from Avram up to Lucky. This also starts to open up the possibility as to what we can pass through params.

Issues

Here's a basic example of how things work right now:

post "/users" do
  # This requires the SaveOperation to understand how to parse params, and what data to permit
  #  and by which keys, etc...
  SaveUser.create(params) do |op, user|
    #...
  end
end

Because of this, Avram needs to have a Param type object. If we need Lucky::Params to implement something, we have to also tell Avram::Params to update it as well.

On top of that, Operations in Avram also aren't able to take in arrays from params.

# user:name=Toejam&user:friends[]=Earl&user:friends[]=Funkster
post "/users" do
  # name can be saved, but friends[] can't
  SaveUser.create(params) do |o, u|

  end
end

Solution

These new "Param" objects can tell you which data it needs, and it knows how to take params from JSON, query string, multipart request, or just a form body, and then convert that data in to an object.

Now imagine we have something that looks like this:

post "/users" do
  user_params = UserParams.from_params(params)
  SaveUser.create(user_params) do |o, u|
    #...
  end
end

In this case, user_params would be an instance that has methods like name that returns a String, and friends that returns Array(String). The SaveUser object now just decides which of those values it needs, and worries about the validation of the data, and storing in the database.

So what does this UserParams look like?

class UserParams
  include Lucky::ParamSerializable
  param_key :user

  param name : String
  param friends : Array(String)
end

What else do we get from this? We can now create params that look like this:

q=tacos&page=2&sort[]=name&sort[]=asc&filter:city=Las+Vegas

class SearchParams
  include Lucky::ParamSerializable
  skip_param_key

  param q : String
  param page : Int32
  param per : Int32 = 50
  param sort : Array(String)
  param active : Bool = false, param_key: :filter
  param city : String, param_key: :filter
end

Notice how the q, page, and sort aren't nested keys, but city is. Also notice how we don't have to send some values as they have defaults.

get "/search" do
  search_params = SearchParams.from_params(params)
  AppSearch.run(search_params) do |op, results|
   #....
  end
end

Current Issues

• HTML Form elements should now take a Lucky::PermittedParam. Maybe we can support both Avram::Attribute, and this so it's not a breaking change?

• Associated objects do work, but they are a bit of a hack. They won't support nested multipart or json params.

• This works great when you're dealing with small forms, but large forms could get really tedious. I'm not sure how we could proxy a model data over, but maybe we have a UserParams.from_model(user)? This could be an extension that exists in lucky_avram.

• The specs pass locally for me, but seem to fail in the CI. There's a bit of weirdness that surrounds the files due to trying to typecast Avram::Uploadable and Lucky::UploadedFile...

• We still need to discuss how this will be integrated in to Avram itself. How does the SaveOperation know to get the value from this param object? Where does that method live? If it's in Avram, then Avram will have to include Lucky as a dependency. If Avram implements some contract, then Lucky will need to know about that, and what does that look like?

• Including this module in to your models won't work, but with this path, the idea is that you wouldn't want to do that anyway. I still need to write out a clear and concise description of where this thing fits in the stack so a newcomer can understand the thought process behind why we chose this route.

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[matthewmcgarvey]

Personally, I prefer this being explicit rather than implicit. Then skip_param_key would not be necessary because that would be the default

[jkthorne]

I think there is an abstraction here that is in between params and model properties. What do you think about adding an object like Mapping or InputGraph. This would make more semantic sense then UserParams.from_params(params).

[jwoertink]

I had to change this from .adapter to ::Lucky to work with the uploaded file. I kept getting this error:

 > 38 | else
 > 39 |   # NOTE: these come from Avram directly
 > 40 |   result = Lucky::UploadedFile.adapter.parse(val)
                                       ^------
Error: undefined method 'adapter' for Lucky::UploadedFile.class

It's at the compile level, so it must be some sort of require order..

[jwoertink]

This is sort of a hack because I couldn't think of a better way. This should be fine for uri encoded params, but would fail for json or multipart... My guess is we could maybe make some overloads? I'm open to ideas on a better way

[jwoertink]

I've added this in to a project to test how it would work with a model, but the unfortunate sad news is, I don't think it will be able to.

Here's a few issues I ran in to.

1. Float64 columns are actually defined as Union(Float64, PG::Numeric) due to how the pg shard casts these. This means that when you have a float column, you'll get a compile-time error about (Float64 | PG::Numeric).adapter.parse(val)

2. I added a little hack just to make it compile and ignore PG::Numeric, but after submitting a form, it fails because the required fields id, created_at, and updated_at weren't filled in. This is obviously because the model sets them as required, but my form didn't send those params as they shouldn't...

3. I added the ignore annotations to the default columns, and then I get this compile error

 > 4 |     @[Lucky::ParamField(ignore: true)]
           ^
Error: class variables can only be annotated with ThreadLocal

I don't understand what the error means, or how I would go about fixing it...

Maybe these are things that we could eventually find a way around? I'm not sure... One though I had was if we took another look at luckyframework/avram#468 and used the DB::Serializable, maybe that would allow us to handle this better? 🤷‍♂️

Now, with that said, I created a separate params object using all of the same columns (which included some enums), and it worked perfectly 👌 I was able to set an avram_enum in a field, as well as some arrays, submit the form, and they were all converted to the proper objects on the action side.

I'm not really sure what to do at this point, or which direction to take. I'm leaning towards taking some of this code from the params, and trying a separate branch that uses that and attempt to just make SaveOperation handle them... If I go that route, it would keep param parsing and such on the Avram side as it is now. That also means that we need to continue to keep Avram and Lucky pretty tightly coupled.

Let me know your thoughts, or ideas.
/cc @matthewmcgarvey @stephendolan @edwardloveall @paulcsmith

[stephendolan]

@jwoertink I had one thought re: item 2 that you pointed out...

It sounds like these param objects are validating the required nature of fields, but could that be the responsibility of the operation? Like, could we basically make all of the properties optional on the ParamSerializable object, then the operation figures out which items are required (as well as all of the other uniqueness/numerical validations)?

[jwoertink]

hmm... 🤔 That's not a bad idea. I guess you'd never really use the param object directly. The operation would receive this param object, and then figure out from that what it needs. I still haven't figured out how that looks, but essentially you'd end up with something like

# in the operation (under the hood)
name.value = param_obj.name if param_obj.has_source?("name")
email.value = param_obj.email if param_obj.has_source?("email")
#...

[jwoertink]

oh, just realized something @stephendolan. Right now I'm just basing off your instance variable which would come from property or column (in the case of models). To make them nilable, I'd have to basically tell the compiler to ignore the fact that you said property name : String... An alternative would be to make a different method like param name : String which could always be nilable. But then how does that work with the columns in a model?

[matthewmcgarvey]

I didn't realize that the intention of the was to include it in Avram models. With the annotation dictating what to ignore, is it possible to include it in a model since they have an id?

[stephendolan]

Hmm... I think another reason for the params from a model not to care about required/optional fields is because different forms will naturally contain different fields.

Even if User.first_name is a required (not null) field on my model, an admin interface meant to update the User.is_admin? flag still wouldn't be sending User.first_name, whereas a settings page would.

Maybe rather than this params object knowing how to serialize a model from a form, it should know how to serialize an operation from a form. Avram still wouldn't have to know how that works, but could the ParamSerializer look at all of the permit and attribute calls on the Operation to figure out what should come in?

That might make even more sense from a usage standpoint, too:

get "/search" do
  user_params = SaveUser.from_params(params)
  SaveUser.create(user_params) do |op, results|
   #....
  end
end

[jwoertink]

I tried to ignore those fields, but it was throwing Error: class variables can only be annotated with ThreadLocal error. The more I thought about it though, I think it would fail anyway just because you tell the compiler "to instantiate this object, you need to give id a value", but then you instantiate the object without an id value... Because User.from_params would return a User instance, but User.from_params(params).id would be.... nil? random value? I'm not sure.

My initial take on this PR was that it wouldn't be included in models at all. I made the assumption that you would always create a separate object. But I had a chat with @stephendolan on that, and we agreed it felt a lot cleaner and more intuitive if you could just use the model since the columns were already defined. It's less duplication.

I gave it a solid effort though. I have a good 16+ hours in to this PR, and I just don't see how we can make this work seamlessly with models as they exist now. Currently where I'm at is one of two options:

1. We keep going down this route, but it's documented and stated that you'd always create a separate param object for each action that you need to handle custom params on.
2. We ditch the idea of a separate param object, and look in to Working out Array param stuff #1527 to keep the param -> operation flow exactly the same. We just beef up how Operations handle params to make Arrays and such work...

If y'all have some other ideas, I'm open.

[paulcsmith]

I've only had time to briefly look at this. I like the idea of a separate object, but I think that will break the type-safety of HTML forms with permitted attributes. I could be wrong about that, but I couldn't figure out a way to get that to work smoothly. I guess you could pass a UserParams object to form inputs instead of an operation, but I'm not entirely sure.

I think what may work better would be to move the logic in Avram into Lucky, and have a LuckyAvram shard that adds param parsing to Avram from Lucky by implementing the Lucky::ParamSerializable or whatever. I don't know exactly how this would work since it has been forever since I looked at the codebase.

Does that make sense? I can try to look into this more later and offer something more concrete

[edwardloveall]

Thanks everyone for all these thoughts. This is a obviously a tricky problem that would be great to solve in a type-safe, user-friendly way. I think the core problem that's been hinted at here is that Params are tightly coupled with Forms, Operations, Models, and Actions. @wontruefree was getting at that here.

I really like the seed of this idea: capturing params in an object that is created with serialization. I think it allows us to break apart some of this coupling and provide the type-safety that we want in Lucky.

A downside appears

Now, it does come at a tradeoff: more code. For every configuration of params in your application you have to create some object like this:

class UserParams
  include Lucky::ParamSerializable

  property email : String
  property password : String
  property interests : Array(String)
end

@stephendolan and @jwoertink you're right that it does feel really nice to just use the Model here, but the problem is that users don't fill out forms that always match models. Imagine a user confirming their email address from a link in an email. That link has a base64 encoded string with all the properties needed to confirm that user. The params in that case just look like this:

class EmailConfirmationParams
  property encoded_user : String
end

This doesn't match any model in the system, because that's not how we store a user. It's not even a subset of the User model. This feels like a mismatch in our API to me. Instead of trying to smooth that over or place these concerns in an operation, I'd rather try to put an explicit line where there is one while remaining user-customizable. Params come from a request, not a model or operation, and we can make that explicit.

One thing we can to make this easier in the default case is to provide a UserParams object based off the fields from the model. It can include all of the fields except for id, created_at, and updated_at.

What about operations?

Right now, operations do two things (or at least two relevant things here):

• Facilitate a creation of a form in a view: text_input operation.name
• Save data to the database: SaveUser.create(params)

I came to the same idea that @paulcsmith is circling around: use a Params object to create the form instead of an Operation. This allows forms to reflect what the user needs to fill out instead of tying themselves to the way the developer modeled the database.

All together

Here's some code to help visualize.

Models

• Reflect a database table and its relations

class User < BaseModel
  table do
    column email : String
    column encrypted_password : String
    column admin : Bool = false
    has_many interests : Interest
  end
end

class Interest
  table do
    column title : String
    belongs_to user : User
  end
end

Params

• Safely model the data for a form

class SignUpParams
  include Lucky::ParamSerializable
  property email : String
  property password : String
  property interests : Array(String)
  # Note: no Admin here
end

Form

• Renders each param as a field based on the type or customized if a user needs it

form_for Users::Create do
  text_input params.email
  password_input params.password
  fancy_input_for_interests params.interests
  # submit, etc
end

Action

• The glue that connects the web request to an operation

class Users::Create < BrowserAction
  get "/users" do
    user_params = SignUpParams.from_params(params)
    SignUpUser.create(user_params) do |operation, user|
      # ...
    end
  end
end

Operation

• Take in params and save records to the database

class SignUpUser < User::SaveOperation
  params SignUpParams # <-- spitballing here

  permit_columns email # What's being saved to the User *only*. Doesn't need to relate to params at all.

  def save_interests
    # runs after save so we have an ID
    params.interests.each do |interest|
      params = UserInterestParams.new(interest: interest, user_id: id)
      CreateAndLinkUserInterests.create!(params) # <-- this is wishful thinking, I know, I know
    end
  end
end

class CreateAndLinkUserInterests < User::SaveOperation
  params UserInterestParams

  permit_columns title

  attribute title
end

whew that's a lot! I'm 100% positive there are flaws here, so help me find them. Rip it apart, even! 😄 I think if we can make something like this work though, it strengthens our type-safety in a relatively easy to understand way. It also potentially cleans up a lot of weirdness that might be hiding, like HTTP params living in Avram or database operations being tightly tied to a form. What do y'all think?

[jwoertink]

Thanks for putting all that together @edwardloveall! It actually got me motivated to work on this more. I think I was getting a bit too much tunnel vision.

Also, just to keep everyone else in the loop, @edwardloveall and I actually spent a bit of time on a call going over this and we even went spelunking in to other projects to see how others handled this. Overall, this feels like a decent direction, and I'd like to explore this a bit more.

I do like the idea of making the html inputs take this param object instead. I'm looking at this as, you're not building a form around a model, but more around a use case. GraphQL works similar in this sense where you have these mutations and queries, but they're not necessarily around a specific table in the DB, but more collective data for a single purpose (which may be comprised of several tables or views..). I will start working out that part, but we're still not out of the woods, yet.

Here's a few items that I'm foggy on and would love some ideas for direction.

• We have an edge case where you have an existing record with valid data in a nilable column. We need the param object to know the difference between a nil value because you didn't assign a value from params AND a nil value because you DID assign a value from params, but it was an intention blank value so you can "nil" out the column.
• Once this object is sent to operations, Avram will need to know and understand this object. Does this mean Avram has a requirement to know about Lucky::ParamSerializable? Does Avram make some sort of empty module that we include in Lucky so Lucky implements that contract? Is this where the idea of a lucky_avram shard comes in that acts as the glue between the two?
• The association with sub param objects is a little shaky. It "works", but what I'm doing is taking the original params, creating a hash from the nested data, then building a new request object and setting that hash to the query string and running through the whole deal again. This means that nested file won't work since that would require building a multipart body... Maybe there's another way we can handle this?
• We also need to keep to the Lucky roots. What sort of things do we need to look at to ensure we catch potential errors in development? I think for the most part, much of this is taken care of, but I'm sure there's some cases we (I) may not be thinking of.

To address the first item, I have this metadata field that knows what data has been passed. That's where this has_key? method comes in to play.

# imagine this in the operation
name.value = param_object.name if param_object.has_key?("name")

Maybe this will work? 🤷‍♂️ No clue, but if anyone has more ideas on that, I'm open. I'll add more as I think of stuff. Thanks all!

[edwardloveall]

For your default value/nillable column edge case, what if param objects needed to be populated?

class Users::Edit < BrowserAction
  get "users/:user_id/edit" do
    user = UserQuery.find(user_id)
    user_params = UserParams.new(email: user.email, optional_nickname: user.optional_nickname)
    html Users::EditPage, params: user_params
  end
end

We could write shortcuts that made some of the default cases easier:

user = UserQuery.find(user_id)
user_params = user.to_params # everything but id, created_at, updated_at

[edwardloveall]

For your second bullet, I don't mind the idea of a separate shard to handle either a glue between concerns, or splitting out a params shard that each project can have a dependency. We saw that yesterday when looking at Elixir/Phoenix.

[jwoertink]

Decided to go with a custom macro here because as the user, you specify name : String, but it's really name : Lucky::PermittedParam(String). This also gives us the ability to determine if the value is nil by intention, or nil because no value was passed in.

[jwoertink]

If the type is nilable (String?), then type is String, but if it's an Array(String?), then core_type ends up being String? and not String.... I think that will cause some issues 🤔

[jwoertink]

This would be great to have so you don't try to do something like:

user_params = UserParams.new
user_params.name = "Jeremy"

However, I couldn't get it to stop raising the exception... Maybe I missed something?

[jwoertink]

I think this won't be used anymore. It seems you can't read an annotation on a macro directly. It would need to be attached to the instance variable or something.