Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Jun 10, 2024

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 in uri itself. I also didn't add support for every type like JSON does, but only the most common ones for now.

Open questions:

  • How in-depth do we want to be with error handling?
    • At the moment I have it only raising if the value is not nilable, nor has a default value, but was missing or unable to be parsed from the data.
      • Compared to the JSON errors that includes information about where it failed. Form data probably isn't likely to be as large so just showing name only is prob fine for now.
    • E.g. catch the ArgumentError and re-raise a URI::SerializableError maybe?
  • How nested data can get a bit wonky.
    • E.g. in missing nilable nested data test case it'll use Filter.new(nil, nil) instead of just nil. I'm not too worried about it for this first pass at least tho.

Left to do:

  • Handle #to_form_data
  • Do a pass on documentation for new module and related types
  • Add support for Field annotation, at least for converters, mainly for Time types to customize format

def Number.from_form_data(params : URI::Params, name : String)
return nil unless value = params[name]?

new value, whitespace: false
Copy link
Member Author

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.

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}}}]"
Copy link
Member Author

@Blacksmoke16 Blacksmoke16 Jun 10, 2024

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

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jun 11, 2024

nitpick: the form_data name is reminiscent of multipart/form-data but you're parsing a x-www-form-urlencoded string for which we use the www_form name in URI::Params.

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 URI::Params. Do you think we could leverage URI::Params.parse(&) to map the String directly into the type? Given that the format is very loose (completely unordered) it might be difficult.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Jun 11, 2024

👍 I been also feeling that form_data isn't really the best naming, so good call on www_form. Think that makes a bit more sense given what we're actually doing.

It would probably be possible to have an overload accept a string directly to which we could then use the block .parse method. Tho at the moment, do you think it would be worth it? I feel like the www_form payloads/query strings are not going to be as big as some of the large JSON files and such that need the parsing aspect for memory reasons. I'd be fine treating it as an enhancement for when/if there is a more concrete use case after this is out and people are able to start playing around with it.

EDIT: Actually after typing that out, I'll look into making it accept a String directly, that way it would be more in-line with the other serializable types in that they accept a string or IO. That brings up another point of it would be nice to have a .parse(io, &) method...

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 URI::Params would deff be the simpler MVP implementation.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Jun 11, 2024

Another option would have it be from_uri_params/to_uri_params where it better lines up with the URI::Params type itself. The to method tho wouldn't be entirely correct tho, unless we actually do have it return a URI::Params object and user could call to_s on that? 🤷.

@ysbaddaden
Copy link
Contributor

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 some=1&more=2 it's easy to parse directly, but once we start having data[key]=1&other=3&data[more]=2 then it becomes much more complex 😰

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Jun 11, 2024

it feels more like mapping a generic Hash(String, Value) into a type T

URI::Params is a bit more restricted which makes things easier. E.g. we don't really have to handle nested hashes, or unions between the types. In your example it's parsed as "data[more]" => "2", so even that would be straightforward enough, just would have to figure out a good way to handle arrays.

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 YAML::Any, JSON::Any, and URI::Params into singular Serialized::Any common between all formats. But that's a whole diff discussion...

@Blacksmoke16
Copy link
Member Author

I thought about it some more and think the sensible option would be to have .from_www_form accept a String and use URI::Params internally vs accepting it directly. This would keep the input and output both being www_form encoded strings, which makes more sense than accepting an object and outputting a string.

Ideally URI::Params.parse would accept an IO too, but that can be done separately i'd say.

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Jun 14, 2024

Alright, think this is ready for a review. Provides an opt-in, initial MVP implementation. Could maybe even throw on a @[Experimental] if we wanted 🤷. Mainly to allow for breaking changes as we get feedback from people using it.

@Blacksmoke16 Blacksmoke16 marked this pull request as ready for review June 14, 2024 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] Add URI::Params::Serializable
2 participants