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 support for uri? in malli.transform (Clojure only) #966

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

stig
Copy link
Contributor

@stig stig commented Oct 11, 2023

With Reitit (0.7.0-alpha7), using the below route definition, I get a failure during the coercion when passing the uri "http://example.com".

["/uri"
  {:post {:summary "Post a URI - fails coercion"
          :coercion reitit.coercion.malli/coercion
          :parameters {:body {:uri uri?}}
          :responses {200 {:body {:uri uri?}}}
          :handler (fn [{{{uri :uri} :body} :parameters}]
                     {:status 200 :body {:uri uri}})}}]]

When I replaced the :coercion with the Clojure Spec version, the coercion works.

With this change the Malli coercer should also work.

I made it Clojure specific since it relies on java.net.URI.

(if (string? x)
(try
(URI. x)
(catch Exception _
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to catch java.net.URISyntaxException here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your call. The body of the try is so small there's little possibility of masking unrelated exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that the body is small, but I prefer URISyntaxException for explicitness - so I have updated it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that fails the Babashka job, so I guess I change it back :-)

https://github.com/metosin/malli/actions/runs/6493910884/job/17635777011

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked in #babashka on clojurians slack (thread here), and @borkdude said that he added it to babashka so it will be available in the next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get the recommended macro approach to work, so I just put a comment in to tighten up the catch clause once our version of babashka has the required commit.

@stig stig force-pushed the transform-uri branch 3 times, most recently from 612ac73 to 51e2c8c Compare October 12, 2023 15:58
With Reitit (0.7.0-alpha7), using the below route definition, I get a
failure during the coercion when passing the uri "http://example.com".

    ["/uri"
      {:post {:summary "Post a URI - fails coercion"
              :coercion reitit.coercion.malli/coercion
              :parameters {:body {:uri uri?}}
              :responses {200 {:body {:uri uri?}}}
              :handler (fn [{{{uri :uri} :body} :parameters}]
                         {:status 200 :body {:uri uri}})}}]]

When I replaced the :coercion with the Clojure Spec version, the
coercion works.

With this change the Malli coercer should also work.

I made it Clojure specific since it relies on `java.net.URI`.
@opqdonut
Copy link
Member

good catch re: babashka!

@opqdonut opqdonut merged commit a43c28d into metosin:master Oct 13, 2023
9 checks passed
@stig stig deleted the transform-uri branch October 13, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants