Changing params API

[paulcsmith]

The current way

params.get(:page) # Single value from top level params, e.g. "1"
params.nested(:user) # Hash of attributes under key, e.g. {"name" => "Jane"}
params.many_nested(:users) # Array of hash attributes under key, e.g.  [{"name" => "Jane"}]

These methods are mostly used for internal Lucky/Avram code and rarely used in user code, but I think we should make them clearer so when people try to contribute it makes sense.

What might be better

params.get(:page) 
params.hash(:user)
params.array(:users)

I think these new names read a lot better, make more sense, and are a tad bit shorter too :)

Thoughts? cc @HarrisonB @jwoertink @edwardloveall

[jwoertink]

Is there something that prevents using params.get to return a single value, hash, or array?

[paulcsmith]

That could work but then the compile time type would be String | Hash(String, String) | Array(Hash(String, String)) which makes it a lot harder to work with.

You'd have to do stuff like this:

page = params.get(:page).as(String)

user_params = params.get(:user).as(Hash(String, String))

users = params.get(:users).as(Array(Hash(String, String)))

The other bummer is that the error it throws when it is not the correct type is kind of opaque. Using methods like get|for|array_of means Lucky knows what you are expecting and can provide a more helpful error. That the main reason they were extracted. Alternatively we could make params just always return a hash but then once again the types would be quite unwieldy and you'd still have to do Hash(String, String).

The other option would be to have a wrapper class like Crystal uses with JSON::Any but that has also been confusing (at least to me) because you often need to use as and to_x a bunch of stuff and the errors are hard to understand when you get it wrong

[jwoertink]

Sounds like a decent reason to me. I don't think I've really needed to use the nested yet, however, I don't like many_nested. It's just a weird method name that I'm sure I'll never remember lol. I like the proposed names.

I haven't checked, but if you did params.array_of(:page), would that return ["1"], or blow up?

[paulcsmith]

Yeah for the most part the methods are used internally by Lucky and Avram but I figure if we can make them better we should haha.

In the case of array_of(:page) it would raise because the param is not an array

On Mar 14, 2019, at 12:10 PM, Jeremy Woertink [email protected] wrote:

Sounds like a decent reason to me. I don't think I've really needed to use the nested yet, however, I don't like many_nested. It's just a weird method name that I'm sure I'll never remember lol. I like the proposed names.

I haven't checked, but if you did params.array_of(:page), would that return ["1"], or blow up?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

[paulcsmith]

I also wonder if params.hash_for(:user) makes more sense. Since it isn't used much outside of internal Lucky/Avram code the extra clarity might be good. Does that make more sense or was params.for(:user) ok?

[hibachrach]

I think hash_for is a good change (on top of the suggested changes).

We also may want to look towards Onyx’s param handling for other ideas in dealing with params

Harrison Bachrach
On Mar 14, 2019, 9:31 AM -0700, Paul Smith [email protected], wrote:

I also wonder if params.hash_for(:user) makes more sense. Since it isn't used much outside of internal Lucky/Avram code the extra clarity might be good. Does that make more sense or was params.for(:user) ok?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[paulcsmith]

@HarrisonB Can you provide a bit more detail on what you mean? I think Onyx has a great type safe way to handle this, but this is for more ad-hoc param access. I do want to see if we could work with Onyx param handling for query params at the action level. That way Lucky doesn't need to roll its own!

[hibachrach]

@paulcsmith Ah, gotcha. Yes, I think integrating with Onyx itself for structured params makes sense.

The other option would be to have a wrapper class like Crystal uses with JSON::Any but that has also been confusing (at least to me) because you often need to use as and to_x a bunch of stuff and the errors are hard to understand when you get it wrong

This was the other thing I was thinking but these are valid points.

The only thing that I might suggest is changing "for" to another preposition? Maybe hash_at?

Also, what would params.get(:user) return? A hash?

[paulcsmith]

params.get(:user) would return a single value if there was one, or it would raise. I like hash_at/for. Thank you for the feedback!

[paulcsmith]

Maybe params.get could/should be params.value_at for extra clarity? params.value_at(:page)

[jwoertink]

Would this params.value_at only return a Hash? So params.value_at and params.array_at?

[paulcsmith]

I wrote the wrong thing at first. I meant params.get could be params.value_at. So it'd return a single value

[skojin]

I vote to have also params[:name] to return single value. value_at is too long.
Also I like "get" over "value_at" because it more short.
And I not like "_at" suffix, for me value, hash, and array is better

[paulcsmith]

One thing to keep in mind is that you should rarely ever use these methods yourself, so length may not matter as much. The only time I ever use these is when I'm writing code in Lucky or Avram.

With that said I kind of agree with the _at. I'm not sure if it actually helps. I'd rather not do params[] because then it looks like it is a hash, but it isn't really.

Then again maybe I should just make the param parser a hash :P I think that could be hard to traverse but maybe it is worth it for simplicity.

[edwardloveall]

I like the naming of hash_at or hash and others because it predictable and memorable once you've learned what they do. Using those methods encourages thinking about the return value you're working with.

value(:name) is a little weird to me because there are other places where we use that method name. cookie.value or field.value or where_clause.value. None of these methods take an argument. I don't think this is super critical and I can't think of a better name than params.value(:name), but I wanted to bring it up.

I don't love the idea of the params being a hash, mostly because then all the value would have to be some kind of union type like String | Hash | Array which weakens the type system. Like if I had a form that was creating multiple tags on the blog post, I could get them with params.array(:tags) and then call each to iterate. If it was just a hash value that could be one of three types, I could only iterate over them if I cast it to an Array first. At least I think that's the case.

[paulcsmith]

Well said @edwardloveall. I think value can be confusing because of what you said. I think I'll stick with get. It isn't super clear, but is not actively misleading. That plus the fact compiler will catch if you try array or hash methods should make it fine.

[paulcsmith]

Thoughts on array_of(:users) and hash_for(:user)? Any clearer/more confusing? Not worth the extra characters?

[hibachrach]

array_of(:users) to me suggests that the entries will be deserialized somehow, but maybe I'm sleep-deprived 😛

[paulcsmith]

I think I’ll just keep it simple and do array and hash then :) The extra stuff might be more confusing than helpful

On Mar 18, 2019, at 5:39 PM, Harrison Bachrach [email protected] wrote:

array_of(:users) to me suggests that the entries will be deserialized somehow, but maybe I'm mad 😛

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #727 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AABXemH_hkaF-VJIF1OwdaOw9MzSP-7wks5vYAeZgaJpZM4b0SDB.

[edwardloveall]

An alternative to get or value could also be single or scalar, which maybe fits into the set of array and hash.

[paulcsmith]

I’m not sure about single or scalar. Seems a bit unclear to me. What do you think about sticking with the types? params.string(:page) since it is always a string

On Mar 18, 2019, at 5:51 PM, Edward Loveall [email protected] wrote:

An alternative to get or value could also be single or scalar, which maybe fits into the set of array and hash.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #727 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AABXes06VCO2sCIOEotYwUG3TrFufHCTks5vYApvgaJpZM4b0SDB.

[jwoertink]

I still like get. I'm not sure what a "scalar" is, though, I feel like after programming for 15 years I probably should know what that means 😝 but at this point, I'm too afraid to ask.

[edwardloveall]

string is great @paulcsmith! That definitely fits with hash and array. get is fine too to be honest. The strongness of my opinion on this is about 2/5

@jwoertink me too 😬 Looking it up just now, my understanding is it's a single value, but possibly with the caveat of being able to be increased and decreased. I think it comes from vector math where you have a unit vector pointing in a direction, and you "scale" each axis by a value to get the final vector. So the vector (0, 5) is the result of the unit vector (0, 1) where the y value is scaled by 5, thus making the scalar: 5. But don't quote me on that 😄

[vlazar]

This makes a very consistent API:

params.string(:page) 
params.hash(:user)
params.array(:users)

[confact]

What I would like is params[:key] - As I usually try to avoid to send post like {user: {name: "Assasdas"}}.

I like to have one hash with everything, like:

{"user-agent": "asdasdasdas", "info": "dsfdssdf"}

(I know this is like JSON more than a hash, but bear with me :D)

What I do now in my production app to make it simple for myself is to just do params.to_h and do hash_params["key"]

I think I understand if it has to be sent through Operation, but what If I don't or have to process it somehow before sending it to operation.

[paulcsmith]

@confact Could you clarify the use case a bit?

I think you can do params.get(:key) to do what you're proposing, but maybe I misunderstood

[confact]

@paulcsmith didn't think a second thought about it. My first thought was that it would be easy to have same syntax on these places.

But like you said it would not make sense to have it in operation and when I use operation the parameters works fine. It is in the action it could be better :)

But I still like the permitted_params idea.

[paulcsmith]

I like it as well and I think we should have that, but we'd still need the regular way to access it from within operations. So I think this could be split into 2 things

• permitted_params
• new way to dynamically access params

Does that sound like a good idea?

[confact]

I like that idea. It would be good when you have to do some magic with the params before sending it to operation :)

[paulcsmith]

@confact Could you explain "good when you have to do some magic with the params before sending it to the operation"? I'm wondering if maybe this is a documentation issue because I had imagined operations as a good place to do some of that magic. Or to have one operation that normalizes params and then calls another operation to save

Could you share a use-case and maybe some sample code? If not comfortable sharing publicly feel free to DM me.

[confact]

First reason: I want to send without nesting it. Mostly because it makes it nicer to send an JSON hash with all the keys:

{"key":"value"}

then

{"post":{"key":"value"}}

No nesting is good for APIs, and make it cleaner. With permitted_params it will also be a safeguard that this params are of correct type automatically. Now I have to cast/raise errors if they are not of correct type somehow.

Second reason: I use this in action to create user_hashes (for not saving ip-addresses) and parsing "user-agent" string before sending to the database, Feels risky to send that to operation due to some mistake saving the sensitive data to the database.

I think you had the gist with that action @paulcsmith when I had issues with tests on just that action.

[paulcsmith]

@wout oh wow that is fantastic! Can't wait to hear how it goes.

I'm setting up my "sponsors" page soon and one of the tiers has some consulting time and you can choose a feature or bug for me to prioritize. Might be worth it for your company, but no pressure. Just an option in case you run into stuff and need some help!

Don't feel pressured though. I am happy to help in Gitter like I try to do anyway regardless of "sponsorship" :D

[wout]

@paulcsmith That's definitely in the cards. I'll get back to you on that. :)

[stephendolan]

@paulcsmith I don't suppose you'd mind reviewing this thread and providing a final summary we can mark as the answer? I don't have a strong opinion on how this is tackled, but am happy to create the issue after we decide on a final approach.

[paulcsmith]

Quite busy this week, but will try to get to it!

[matthewmcgarvey]

Was reminded of this discussion from this conversation luckyframework/avram#408 (comment)

The issue that we're having right now is that we need to be able to cover all these bases:

• single top-level value
• single nested values
• multiple top-level values
• multiple nested values
• nested key/values
• multiple nested key/values

All of these bases are covered except for the multiple nested values and that is only if you consider it ok that nested key/values can not have any values that are arrays or themselves nested.

As long as we are not defining expected params in the Action class, I think we need to convert Lucky::Params over to being a recursive data structure. The easiest way I can think of to do that is to convert everything into json, but that isn't possible with multipart data (file uploads).

The other thing I would like to see is that Lucky::Params should merge all the various data sources together before they're accessed. RIght now, when you call params.get(:query) it searches in various places for that key (path variables, query params, json) and returns the first it finds. Instead of doing that, I'd like for everything to be merged together in this recursive data structure. It will make the implementation so much easier.

We will need two different data stores, one for regular data and one for file uploads so that they don't overlap. Ideally, we could base Lucky::Params off a generic structure so that we could have one GenericNestedData(String) and one GenericNestedData(Lucky::UploadedFile).