Use coercion to encode query-string values in match->path

[Deraen]

This is the initial version of extending Reitit code and the frontend module to encode query-string parameter values using the Malli schema and string transformer.

Coercion already allowed parsing query-string strings using Malli and even per schema property using the :decode/string option. Implementing this also for URL generation (href, push/replace-state, etc.) makes sense because Malli also supports value encoding using the schemas.

Changes:

• Coercion protocol needs to be extended with a method to get coercer fn for this use case
  Coercion implementation for Malli. Spec and Schema are a no-op at this point. We need to consider whether they can also support this case.
• The match->path function now gets a coercer for the match (creates it again for every call!!) and uses the coercer to encode query-string map values first before using the existing query-params url-encode function
  • TODO: Does forced url-encoding for the values make sense?
  • What if we had a separate URL-encoded transformer for Malli coercion? Current string-transformer options might differ from query-string use case.

TODO

• Frontend module tests
• Figure out where to place the match->path version with coercion, likely not reitit.core
• Use -compile-model correct to merge schemas etc., before use in match->path coercion
• Consider if query-coercer can be "precompiled" in reitit.frontend/router, so the coercer and transformers don't need to be recreated each time
• Check if we should avoid the malli/-coercer implementation, which does decode -> validate -> encode. For query strings, only encode might be better. Query string parameters are likely already decoded, so with this version, decode functions need to check if the value is encoded or not.

Separate or further work

• Frontend set-query-params and set-query currently don't create a Match from the current path, so schema and coercion options aren't available. It should be possible to extend these functions to get a match first, so schema and coercion options are available. (At least for reitit.frontend.easy.)

[opqdonut]

LGTM!

[opqdonut]

I think "By default, collections ..." would read a bit better

[lucassousaf]

Hello! We've been testing these changes and we have a problem when using custom transformers. We think the problem might be this line. This line is ignoring the transfomers (there is a typo too) parameter and hard-coding string-transfomer-provider instead of using the transformers defined in the Reitit router configuration. Maybe it should be something like this?:

(defn- -query-string-coercer
  "Create coercer for query-parameters, always allows extra params and does
  encoding using string-transformer."
  [schema transfomers options]
  (let [;; Always allow extra paramaters on query-parameters encoding
        open-schema (mu/open-schema schema)
        ;; Do not remove extra keys
        ;; BEFORE: (-transformer (get-in transfomers [:string :default]) (assoc options :strip-extra-keys false))
        string-transformer (get-in transfomers [:string :default])
        encoder (m/encoder open-schema (assoc options :strip-extra-keys true) string-transformer)]
    (fn [value format]
      (if encoder
        (encoder value)
        value)))) 

Is there any reason for hard-coding string-transformer-provider?

[Deraen]

The reason is that create fn processes transformers map and applies the options to TransformerProviders, then the transformer we see after that step is one with mt/strip-extra-keys-transformer combined with the string-transformer.

• We can probably use the transformers value before options are applied for this code
• the options are just applied to the default TransformationProvider values, when you provide a concrete transformer instance yourself, this doesn't apply
• while m/encoder takes options map, :strip-extra-keys isn't one of the options that would be used there (AFAIK). This option is only used for reitit.malli.coercion TransformationProviders: https://github.com/metosin/reitit/blob/master/modules/reitit-malli/src/reitit/coercion/malli.cljc#L27-L33

I'll see what I can do.

[lucassousaf]

I see, you're right. The encoder doesn't take the :strip-extra-keys option.

Maybe one solution could be to have a specific query string transformer provider that doesn't include the :strip-extra-keys transformer and include it by default in the Reitit router configuration.

[Deraen]

@lucassousaf Check the latest commit, :string transformer will be used now. I did consider adding a new transfomers option for query-string encoding, but I think decoding and encoding should use the same transformer so using :string does seem good idea now.

[lucassousaf]

Sounds good! At first glance it seems to work as expected. If we find anything (I don't think so, looks good) we can open an issue. Thanks a lot for your work @Deraen !