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

Conform strings while leaving everything else unformed #65

Closed
jeaye opened this issue Jul 4, 2017 · 17 comments
Closed

Conform strings while leaving everything else unformed #65

jeaye opened this issue Jul 4, 2017 · 17 comments

Comments

@jeaye
Copy link

jeaye commented Jul 4, 2017

This is either a question of a feature request, depending on whether or not it's already available. Based on what I've read from the docs, this hasn't been mentioned.

Use case

I'm working with ClojureScript, where my map values which are keyword are often turned into strings against my will. In hopes of fighting this problem, I've been trying out spec-tools. He's an example of the conforming I'm doing:

user=> (require '[cljs.spec.alpha :as s]
                '[spec-tools.core :as st]
                '[spec-tools.spec :as sts])
user=> (s/def ::foo (s/keys :req [::bar ::spam]))
:user/foo
user=> (s/def ::bar (s/and sts/keyword? #{::meow}))
:user/bar
user=> (s/def ::spam sts/string?)
:user/spam
user=> (st/conform ::foo {::bar "user/meow" ::spam "user/meow"} st/string-conforming)
{:user/bar :user/meow :user/spam "user/meow"}

Desired functionality

As shown above, this conforming is sweeet. Unfortunately, it also does the normal conforming stuff, like turning s/or and s/alt into their named pairs. Here's an example, based on the same REPL session above.

user=> (s/def ::spam (s/or :string sts/string? :integer sts/integer?))
:user/spam
user=> (st/conform ::foo {::bar "user/meow" ::spam "user/meow"} st/string-conforming)
{:user/bar :user/meow :user/spam [:string "user/meow"]}

As soon as ::spam has an alternative, the value I get back is less than ideal.

Short-term work-around

For now, I've been subsequently unforming my conform, which adds the keywords in the right spot, but removes the named pairs. As you can imagine, for performance, efficiency, and cleanliness, this isn't ideal. Here's an example, again, based on the above session:

user=> (s/unform ::foo (st/conform ::foo {::bar "user/meow" ::spam "user/meow"} st/string-conforming))
{:user/bar :user/meow :user/spam "user/meow"}

Long-term solution

As this is a common problem in both ClojureScript and Clojure code working with JSON and other formats which don't support EDN keywords, a cleaner solution would be preferred. I'd be happy to discuss more.

Thanks!

@jeaye jeaye changed the title Conform strings while leaving everything else unconformed Conform strings while leaving everything else unformed Jul 4, 2017
@ikitommi
Copy link
Member

ikitommi commented Jul 4, 2017

Hi. And thanks for the detailed description. The conform+unform is the way to do it with spec, not sure there is any other way to do it. For most Specs, the unform is just no-op, so should be fast, despite it requires second walk over the value. I would add a conform-unform fn into the client project. You could verify that form Cognitect people, at least Alex is hanging on #clojure-spec in Slack.

One thing that would help a bit is to resolve #60 - enum types could be extracted automatically (mostly homogenous).

We are btw using Transit in clj<->cljs remoting, which retains types.

@jeaye
Copy link
Author

jeaye commented Jul 4, 2017

Hey, thanks for the reply.

not sure there is any other way to do it

One thing that has occurred to me as potentially useful, in spec-tools, would be to extend the visitor pattern implementation to allow for visiting spec'd values. With this, spec-tools could provide a visitor-transform fn. The way this could work is to have the visiting fn take in the value and its (nilable) spec. It then returns the new value, which is used to build the transformed data. So, basically clojure.walk/postwalk, but where you get both the spec and the value. Would you be open to something like this?

We are btw using Transit in clj<->cljs remoting, which retains types.

We're using EDN for our transit data, between cljs and clj. This loss of data is actually happening when giving cljs values to ReactNative, through Reagent. I didn't dig into the details in the first post, since this sort of thing can come up in various ways when JSON and JS are involved.

@miikka
Copy link
Contributor

miikka commented Jul 5, 2017

So, basically clojure.walk/postwalk, but where you get both the spec and the value. Would you be open to something like this?

IIRC this was basically my suggestion for implementing custom coercions. We ended up doing the dynamic variable thing because it was much simpler to bolt it to clojure.spec's conform. I've started to think that maybe it's a mistake to use conform for this kind of coercion at all (like your example and like what we want to do in the compojure-api ecosystem). conform is good for macro parsers and that kind of stuff, but it just not what we want for these use cases. Instead of insisting on using conform, maybe we should just build our own coercion tools.

What you suggest would be an important building block for this, so I'm definitely open to it. What do you think, @ikitommi?

@ikitommi
Copy link
Member

Good discussion. I did a spike on holidays on adding coercion support to the spec itself, found here: clojure/spec.alpha#1. Waiting for comments on that via CLJ-2116 on Jira.

I'm always open for better ways on doing this, if you have time, a spike/gist/PR would be most welcome. Performance is also a value, now the spec conform is quite fast as it's walking through the protocol methods (instead of generic tree walking).

Initially was about to add conform-unform into spec-tools.core, but then there would be need to have conform-unform! too => left it to the client-projects.

@ikitommi
Copy link
Member

Did a perf test on the conform-unform. Actually, the unform for maps is dead slow. One would be to have the spec conform to be changed to walk where one could parametrize for it to do either normal conform or just coerce (not expand to contain branch info). I'll write an issuue to clj jira about this.

Any progress / ideas for this (on the spec-tools side)?

@jeaye
Copy link
Author

jeaye commented Oct 6, 2017

Actually, the unform for maps is dead slow.

Gah. We not only use this (conform + unform) for our ReactNative stuff, as I mentioned in an earlier comment, but now also for all of our Firebase Real-Time Database data. I took a look at your PR and something like that would be perfect; I'm just not optimistic for it catching on with the Clojure team. Is this something which can only be addressed in s/conform, at least without reimplementing s/conform in spec-tools?

From what it sounds like, the coercion @miikka brought up would essentially be a reimplementation of s/conform, but in a much more general way. Do you have a link to that previous discussion, or do you recall why spec-tools didn't go down that route?

@miikka
Copy link
Contributor

miikka commented Oct 6, 2017

It was just simpler to do the dynamic variable thing, esp. since clojure.spec was/is a bit of moving target. We didn't want to re-implement s/conform and at least I didn't understand clojure.spec very well anyway back then.

@ikitommi
Copy link
Member

ikitommi commented Oct 6, 2017

It's impossible implement the better s/conformin spec-tools without re-writing the most of clojure.spec. Root cause is that Specs dont' have any type-information to dispach on: they are implemented with reified Protocols and we just see clojure.spec.alpha.Spec, not KeysSpec, OrSpec etc.

We could read the type from s/from, and create spesific types for all Specs at spec-tools side but we would have to copy all the current conform* and explain* implementations. And we would still need to wrap everything into Spec Records. There is an issue of that #28.

It seems that Rich hasn't assessed the CLJ-2116, so motivation to write a better version is not that hight at moment. I could draft that thou.

@miikka
Copy link
Contributor

miikka commented Oct 6, 2017

Tommi, all the information you need is in s/form. It's perfectly valid route. The only obstacle is that parsing s/form is hard - but luckily Rich Hickey has created a library for parsing data structures. It's called clojure.spec, you may have heard of it! I'm just waiting for CLJ-2112 to happen.

Seriously, s/conform should be called s/parse.

@ikitommi
Copy link
Member

ikitommi commented Oct 6, 2017

Funny :)

Ok, let's thing about the s/formpath:

  1. we have a clojure.spec.alpha.Spec

  2. to enable fast coercion at runtime, we need to preprocess it: analyze the s/form to collect extra info, like the type to dispatch coercion on. We basically have this already: with Spec Record creation, we run the custom parser (https://github.com/metosin/spec-tools/blob/master/src/spec_tools/parse.cljc) - let's just store the original spec macro name too. The custom parser could be replaced partially by CLJ-2112, I guess.

  3. to be really fast at runtime, we want to use Protocol Dispatch. To get this, we need to create different types for all Specs, e.g. KeysSpec, OrSpec, CatSpec etc (the Extract reified clojure.spec Specs as Records  #28). These should implement the clojure.spec.alpha.Spec and something extra like spec-tools.core/Walk, which would have function walk to walk each Spec type (with modes like :validate, :conform, :coercion etc.). Implementation would be a copy-paste from the clojure.spec. Multimethods might be ok too, bit slower but less code.

The good:

  • it works!
  • it's fast!

Downsides:

  • we still need to wrap everything into Spec Records
  • all the conform* & explain* codes are duplicated into spec-tools

Did I miss something? Some better ways to do this? Want to implement this?

And of course, right place to implement this would in clojure.spec. I can draft this into Jira. As Cognitect doesn't seem to be interested in the runtime stuff, so woudn't hold by breath with this.

I don't think we are interested in turning spec into a transformation engine via conformers

cheers,

Tommi

@ikitommi
Copy link
Member

ikitommi commented Oct 6, 2017

Oh, forgot. Specs have a lot of internal state that is not visible outside of the Protocol. To re-implement coercion, we would have to copy that state mungling to spec-tools too.

s/keys example:

A backwards compatible fork of clojure.spec with the coercion inside as an add-on?

@jeaye
Copy link
Author

jeaye commented Oct 6, 2017

A backwards compatible fork of clojure.spec with the coercion inside as an add-on?

Now that's an idea. As you described in detail, all of this could make it into spec-tools. A fair amount would need to be duplicated from clojure.spec though, simply because Cognitect isn't interested in these transformations.

Doing this in spec-tools

Pros

  • People are already using spec-tools
  • People get to use the official clojure.spec

Cons

  • Looks like it's much more work, compared to doing this in clojure.spec

Forking clojure.spec

Pros

  • Simpler integration, without needing to "duplicate" anything
  • spec-tools can remain mostly as it is

Cons

  • People now need to use an unofficial clojure.spec
  • That fork needs to be kept up to date with a moving target (as well as spec-tools)
  • There actually needs to be two forks: Clojure & ClojureScript

If there were only one clojure.spec repo, for both Clojure and ClojureScript, I'd suggest going the forking route. Dealing with those two mostly-duplicated repos is a real pain though, as I've seen with orchestra.

@miikka
Copy link
Contributor

miikka commented Oct 6, 2017

clojure.spec consists of two parts:

  1. a language for describing EDN data structures
  2. a library for parsing, validating, and generating those data structures

A part of what spec-tools does is ditching the language -- see e.g. data specs which replace it with a Schema-like language. The other part, which we're now talking about, is ditching the library, since it's not really suitable for high-performance coercions.

I'm inclined to ask if there's any point in this project. If the Schema language and the Schema library are better for e.g. libraries like ring-swagger, why bother with clojure.spec?

@ikitommi
Copy link
Member

https://dev.clojure.org/jira/browse/CLJ-2251

@jeaye
Copy link
Author

jeaye commented Oct 11, 2017

Awesome. Thanks, @ikitommi. One addition which may be helpful is to illustrate an example usage of s/walk*.

@ikitommi
Copy link
Member

After a year of waiting the 2251, just hacked together an minimalistic external walker. It can be used to build a simple coercion which does just coercion without validation (just like spec-coerce): walker sees both the spec and the value. Is not optimized for perf and can only walk over simple specs - re-parsing regexp specs would mean to rewrite most of the spec.

But, there will be st/coerce in the next version to do coercion without validation and st/decode will use it to pre-process the specs.

(require '[clojure.spec.alpha :as s])
(require '[spec-tools.core :as st])
(require '[clojure.test :refer :all])

(s/def ::c1 int?)
(s/def ::c2 keyword?)

(let [spec (s/nilable
             (s/nilable
               (s/coll-of
                 (s/or :keys (s/keys :req-un [::c1])
                       :ks (s/coll-of (s/and int?)))
                 :into #{})))
      value [{:c1 "1" ::c2 "kikka"} {:c1 true} [1 "2" "invalid" 3]]]
  (is (= #{{:c1 1, ::c2 :kikka} {:c1 true} [1 2 "invalid" 3]}
         (st/coerce spec value st/string-transformer)))
  (is (= #{{:c1 "1", ::c2 :kikka} {:c1 true} [1 "2" "invalid" 3]}
         (st/coerce spec value st/json-transformer))))
; true

@ikitommi ikitommi mentioned this issue Oct 16, 2018
@ikitommi
Copy link
Member

0.8.0-SNAPSHOT

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

No branches or pull requests

3 participants