-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add URI::Params::Serializable
#14684
Add URI::Params::Serializable
#14684
Conversation
src/uri/params/from_form_data.cr
Outdated
def Number.from_form_data(params : URI::Params, name : String) | ||
return nil unless value = params[name]? | ||
|
||
new value, whitespace: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using whitespace: false
here so that it doesn't consider age= 25
to be valid.
src/uri/params/serializable.cr
Outdated
def initialize(*, __uri_params params : ::URI::Params, name : String?) | ||
{% begin %} | ||
{% for ivar, idx in @type.instance_vars %} | ||
%name{idx} = name.nil? ? {{ivar.name.stringify}} : "#{name}[#{{{ivar.name.stringify}}}]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A top level call to .from_form_data
will have name
be nil
, otherwise the names will be appended to handle nested data via the internal overloads. E.g. filter[direction]
.
Would be possible to change this, but this is what I went with because I tested ['foo' => 'bar','biz' => ['age' => 10,'alive' => true]]
with PHP and it was encoded as foo=bar&biz[age]=10&biz[alive]=1
, which Crystal parses as URI::Params{"foo" => ["bar"], "biz[age]" => ["10"], "biz[alive]" => ["1"]}
. So seemed reasonable enough to use same key format as how URI::Params
represents it.
PHP does parse it out into a nested assoc array tho, but that would require making URI::Params
more like JSON::Any
to handle the possible recursion...
nitpick: the suggestion: the JSON and YAML Serializable use their PullParser to parse directly into the type (no Any or Hash intermediary), but you're mapping the already parsed |
👍 I been also feeling that It would probably be possible to have an overload accept a string directly to which we could then use the block EDIT: Actually after typing that out, I'll look into making it accept a EDIT2: Arrays are going to be the hard thing to handle as it'll yield each item in an array separately, possibly not continuously either. Open to ideas, but using the already parsed |
Another option would have it be |
Direct parse is half of the interest of Serializable IMO. Without it, it feels more like mapping a generic Hash(String, Value) into a type T, that may be more interesting than mapping URI::Params only? Indeed, with a single level such as |
Having some more generic "data mapper" would be pretty cool thing to pair with #10886 tho. I know Symfony Serializer makes use of an intermediary data representation, which has its own pros and cons. That would be kinda hard in Crystal land, unless you settle on merging |
I thought about it some more and think the sensible option would be to have Ideally |
Add spec for child type inheritence
Alright, think this is ready for a review. Provides an opt-in, initial MVP implementation. Could maybe even throw on a |
More spec coverage
@straight-shoota Pushed up a commit to address your comments. What are your thoughts on adding an |
I don't think we need |
👍 sounds good. I'm planning on monkey patching this into the Athena code for now so I don't have to wait until the next release. So I'll be able to play with it a bit as part of that and can PR any fixes/improvements I come across. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: It could be nice to have some more specs for edge cases of from_www_form
.
For example, specify what happens when the value an empty string. And for the variants that take a key name, what happens when the key is missing or matches only case-insensitively.
Actually, the empty string test case would be nice for every data type, not just |
expect_raises Exception do | ||
Float64.from_www_form "" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be worth its own issue:
$ crystal eval 'pp Float64.new "", whitespace: false'
Unhandled exception: Index out of bounds (IndexError)
from /usr/lib/crystal/string.cr:996:16 in '[]'
from /usr/lib/crystal/string.cr:755:40 in 'to_f64?'
from /usr/lib/crystal/string.cr:710:5 in 'to_f64'
from /usr/lib/crystal/float.cr:327:5 in 'new:whitespace'
from eval:1:1 in '__crystal_main'
from /usr/lib/crystal/crystal/main.cr:118:5 in 'main_user_code'
from /usr/lib/crystal/crystal/main.cr:104:7 in 'main'
from /usr/lib/crystal/crystal/main.cr:130:3 in 'main'
from /usr/lib/libc.so.6 in '??'
from /usr/lib/libc.so.6 in '__libc_start_main'
from /home/george/.cache/crystal/crystal-run-eval.tmp in '_start'
from ???
I'd expect this to bubble up an ArgumentError
. At the moment it seems to bubble up an exception from the implementation when using whitespace: false
.
This pull request has been mentioned on Crystal Forum. There might be relevant details there: https://forum.crystal-lang.org/t/uri-params-serializable/6157/4 |
Resolves #14680
Still a WIP, but wanted to get something opened to at least get some eyes on it. Made it so you have to explicitly require it via
require "uri/params/serializable"
, which pulls inuri
itself. I also didn't add support for every type like JSON does, but only the most common ones for now.Open questions:
ArgumentError
and re-raise aURI::SerializableError
maybe?missing nilable nested data
test case it'll useFilter.new(nil, nil)
instead of justnil
. I'm not too worried about it for this first pass at least tho.Left to do:
#to_form_data
Field
annotation, at least for converters, mainly forTime
types to customize format