lambdaisland / uri

A pure Clojure/ClojureScript URI library
Mozilla Public License 2.0
243 stars 21 forks source link

Add query-coll, coll->query-string and query-string->coll helpers #47

Closed velios closed 9 months ago

velios commented 10 months ago

First of all, thank you very much for your work. Having worked with the library for some time, I can assume that there are two ways to handle request parameters. The first is the main way for the library to use maps and all their convenience. For some users, it is important to preserve the order of arguments, and the documentation describes how this can be achieved using flatland. I find it interesting to retain this ability without adding unnecessary dependencies, and vector seems like a great fit for this. The PR is not complete, it is just a prototype so we can discuss how appropriate you see this approach for your project. If you find my arguments convincing, then let me know and I will add documentation and unit tests to implement it. I would also be glad to receive your recommendations on naming. I illustrated my other PR with more detailed examples.

plexus commented 9 months ago

This is a pretty substantial change, and it's not really clear to me what problem this is trying to solve. Maybe you can elaborate on the problem statement and use cases, and then we can take it from there.

Note that uri is basically finished software, meaning we are very conservative in making changes, especially if there's any chance that it might impact existing consumers. For instance I notice immediately in your PR that you drop a :keywordize? param in one of the functions. That's an immediate red flag.

velios commented 9 months ago

In the url, the order of the query attributes is significant information for some cases. When we convert query-string to map, the order is no longer guaranteed, because of essence of map type. From the Readme I got the impression that order preservation is offered using an external dependency like flatland.ordered.map. While trying to use this solution, I came across a problem that I tried to solve in #46. As I continued to try to use it, I kept getting new problems, like this one:

(require '[lambdaisland.uri :as uri]
              '[flatland.ordered.map :refer [ordered-map]])
(def ordered-query (uri/query-string->map "a=1&b=2&a=3" {:into (ordered-map)}))
(assoc (uri/uri "https://example.com") :query (uri/map->query-string ordered-query))
=> "https://example.com?a=1&a=3&b=2" ;; but expected initial order a=1&b=2&a=3

The order of the arguments has changed, although this is exactly what I tried to avoid. I saw that the into parameter was added recently (in version 1.15.125) and I would like to clarify what problems it was intended to solve? (I thought it was added to solve an order problem, now I think maybe I'm trying to solve a problem with a tool that was not designed for this purpose).

Then I remembered about collections in clojure. They are a basic part of the language and guarantee order. Since for record URI it is only care about query is a string, this PR proposed helpers that are essentially similar to map->query-string and query-string->map, only instead of converting to map, they make similar conversions to coll (the default is a vector, but user can choose another one, I chose the argument name acc, because into intersects with the name of the standard function). Since all the work happens in helpers, I don’t consider these changes significant. These functions can be added to the external namespace, which is what I did on my project now, but the functionality seemed in demand to me to share it and include it in the library.

In my opinion this also reduces the problem raised in #46, #49. If into was nevertheless introduced to solve problems with order, then with the new helpers it was possible to abandon it and not solve the problem with the inverse conversion of nil. But that would already be a significant change.

Not so important for PR, but I wanted to mention what exactly the problem I was solving was that I needed to keep things in order. I was solving the problem of adding utm tags to urls that come from external users. The problem is that some of the urls are more fragile than they should be. For example, changing the order of parameters affects the performance of the query. I know it shouldn't work this way, but I have no power on external users' servers. At the same time, adding parameters to the end of the request has never led to problems in real cases. And I started studying your library with the goal of making such additions to the url.

velios commented 9 months ago

I'm glad that you follow the belief that uri is finished software and I fully support you in that. About :keywordize? attribute. I tried to reuse the code wherever possible and, as a first approximation, came to this solution. We can use keywordize? for coll, but it seems to me that this makes less practical sense than in maps, but I think it can be returned and it will not interfere.

(query-string->coll "http://example.com?a=1&b=2") ;; => [["a" 1] ["b" 2"]

if we use keywordize it be [[:a 1] [:b 2]] which is not feel useful, but why not

velios commented 9 months ago

The required functionality was implemented within #51