URI::Params is a struct? Why?

Help & Support
1

Hello!

Why is URI::Params a struct? It is not possible to extend it now (non-abstract struct can’t be extended).
Is it really needed? For performance?

URI::Params::Builder is still a class (good) and builder is created/instantiated on every URI::Params.to_s call, so where is the struct performance of URI::Params now?

Can someone explain? What about to change URI::Params to the proper class?

Thanks! pf

2

URI::Params is just a thin wrapper around Hash and not expected to be mutable or extended.
URI::Params::Builder should probably be a struct as well. There’s no reason for it to be a class.

Could you explain what you want to do?

1 Like
3

Default URI::Params uses URI::Params::Builder and this builder translates spaces as “+” - unfortunately, Icinga API does not understand “+” as space (only %20), so my thought was to subclass URI::Params::Builder to treat space as %20 and then subclass URI::Params and redefine method to_s to use my new builder. Not possible.

Also - URI::Params has methods like []= and add etc. - so it’s not so immutable.

Subclassing is such a natural way to change the behavior of small details and one just can’t do it in real :( I just don’t understand why the stdlib designers make it difficult to subclass things (it’s everywhere, Java, Kotlin… final classes, private methods… it’s a pain in the end).

In the end, I solved it differently, of course.

[EDIT] Also you said “it’s a wrapper around Hash” - but Hash is a class :) That doesn’t make sense for me.

4

The feature you’re looking for has already a ticket, just hasn’t been implemented yet: Expose HTTP::Params#to_s encode %20 or + · Issue #7662 · crystal-lang/crystal · GitHub

I think it would really make sense to have that in the standard implementation.

For a quick solution, I don’t think subclassing is the right way. You can just reopen the type and add the additional #to_s implementation yourself.

Yes it is. These modifying methods delegate to the wrapped hash. URI::Params itself is immutable, it only has a single instance variable (@raw_hash) and that can never be changed. The contens that are hold in that hash can be changed, of course.

What of that doesn’t make sense?

1 Like
5

If I reopen the type I change the behaviour for the whole system/program. But I have more HTTP APIs in my program, not only Icinga, but also Director and our internal systems. So reopening and rewrite behaviour globaly is not always the solution.

I just don’t understand why to enclose raw Hash class in the struct and disable subclassing. Even class can be sort of immutable.

6

My two cents:

  • If it’s missing functionality, I think it’s just simpler to add that functionality to the standard library. It’s good for you and also good for everyone else: no need to define a shard with this or anything.
  • We can’t change this struct to a class because it breaks backwards compatibitliy.

In general I don’t feel like subclassing something to change behavior is a good solution, ever.

2 Likes
7

Subclassing - you can take any class (imagine a more complex one), quickly rewrite some small method, add another method or two and then use that thing and be happy. Situations where you can’t do it just hurt (in every language). It’s that simple. I just think you should be careful with structs.

But I don’t want to repeat myself too much.

8

If I understand things correctly, this patch is all what’s needed (plus docs update):

diff --git a/src/uri/params.cr b/src/uri/params.cr
index e0720341e..d1b8dff4a 100644
--- a/src/uri/params.cr
+++ b/src/uri/params.cr
@@ -350,6 +350,12 @@ class URI
       raw_params.delete(name)
     end
 
+    def to_s(*, space_to_plus : Bool = true)
+      String.build do |io|
+        to_s(io, space_to_plus: space_to_plus)
+      end
+    end
+
     # Serializes to string representation as http url-encoded form.
     #
     # ```
@@ -358,8 +364,8 @@ class URI
     # params = URI::Params.parse("item=keychain&item=keynote&email=john@example.org")
     # params.to_s # => "item=keychain&item=keynote&email=john%40example.org"
     # ```
-    def to_s(io : IO) : Nil
-      builder = Builder.new(io)
+    def to_s(io : IO, *, space_to_plus : Bool = true) : Nil
+      builder = Builder.new(io, space_to_plus: space_to_plus)
       each do |name, value|
         builder.add(name, value)
       end
@@ -375,7 +381,7 @@ class URI
     # Every parameter added is directly written to an `IO`,
     # where keys and values are properly escaped.
     class Builder
-      def initialize(@io : IO)
+      def initialize(@io : IO, *, @space_to_plus : Bool = true)
         @first = true
       end
 
@@ -383,9 +389,9 @@ class URI
       def add(key, value : String?)
         @io << '&' unless @first
         @first = false
-        URI.encode_www_form key, @io
+        URI.encode_www_form key, @io, space_to_plus: @space_to_plus
         @io << '='
-        URI.encode_www_form value, @io if value
+        URI.encode_www_form value, @io, space_to_plus: @space_to_plus if value
         self
       end

Then you can do something like:

require "uri"

params = URI::Params{"a b" => "c d"}
params.to_s(space_to_plus: false) # => "a%20b=c%20d"
2 Likes
9

As a solution for this exact mentioned problem with %20 - yes, it is the solution.

10

Also throwing in my two cents on the subclassing discussion. Subclassing itself isn’t bad, but it’s when you get into subclassing types you don’t own is when things get tricky. This can lead to massive headaches if/when the upstream type changes. Speaking from experience here.

A better approach is to decorate the other type, such that you can do something before/after a specific method, or add more methods, all the while not touching anything related to the upstream type itself. In this way you are insulated from internal refactors that do not change the public API, but may break your subclassing, (assuming you are decorating against the public API as well).

HOWEVER, with the way the stdlib is implemented at the moment, this doesn’t work as well as it could since everything is typed as concrete implementations instead of abstractions. I.e. you can’t do something like:

class CustomRequest
  getter request : HTTP::Request

  forward_missing_to @request

  def self.new(method : String, path : String, headers : HTTP::Headers? = nil, body : String | Bytes | IO | Nil = nil, version : String = "HTTP/1.1") : self
    new HTTP::Request.new method, path, headers, body, version
  end

  def initialize(@request : HTTP::Request); end

  def safe? : Bool
    @request.method.in? "GET", "HEAD", "OPTIONS", "TRACE"
  end
end

and have it work as a drop in replacement to HTTP::Request, even if it is 100% compatible with it. For decorators to work, you really need to have the interfaces to build against such that you can use them interchangeably. E.g. RequestInterface. But that’s probably a conversation in its own.

4 Likes
11

Just a note that what’s requested in this thread will be available in the next release: Add `space_to_plus` option in `URI::Params` everywhere by asterite · Pull Request #11821 · crystal-lang/crystal · GitHub

3 Likes