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

Calling valid? within a custom type transform fn causes an StackOverflowError #240

Open
timclemons opened this issue Aug 5, 2020 · 3 comments

Comments

@timclemons
Copy link

Given a setup like the following:

(s/def ::epoch (s/int-in Long/MIN_VALUE Long/MAX_VALUE)
(s/def ::nano (s/int-in 0 (Math/pow 10 9)))
(s/def ::time-basis #{:UTC :TAI})
(s/def ::instant
  (st/spec {:spec (s/keys [::epoch ::nano ::time-basis]) :type :instant}))

(defn tai->utc
  [_ {:keys [epoch nano time-basis]
  (if (and (s/valid? ::instant x) (= :TAI time-basis))
    {:epoch (convert-epoch-to-utc epoch)
     :nano nano
     :time-basis :UTC}
    x))

(def utc-xform
  (st/type-transformer
    {:name :utc
     :encoders {:instant tai->utc}
     :default-encoder st/any->any}))

Attempting to run (st/encode ::instant val utc-xform) on any valid ::instant value results in the transform function running recursively until a stack overflow occurs. Removing the call to valid? resolves this.

Can we get around this? Seems like part of the point of using spec is being able to validate data incoming to functions like the transforms.

@wandersoncferreira
Copy link
Contributor

Hi @timclemons how are you? I reproduce your report from my side and the problem is a mutual recursion hard to avoid here. Both stt/encode and s/valid? fns calls conform*. However there is a way to avoid the Overflow. When we look at the s/valid? call, it never provide a transformer to conform*.

Therefore, you can avoid the recursion by re-writing your transformer to include a nullable binding to *transformer*.

(defn tai->utc
  [_ {:keys [epoch nano time-basis] :as x}]
  (binding [*transformer* nil]
    (if (and (s/valid? ::instant x) (= :TAI time-basis))
      {:epoch "Epoch converted"
       :nano nano
       :time-basis :UTC}
      x)))

When the transformer is called at

(let [transformed (transform this x)]
you will disable the transformation step performed by conform* when the s/valid call happen.

It is not elegant because you need to understand the internal workings of both s/valid? and st/encode. The best thing here would be able to spot when conform* is being called from a s/valid? and automatically disable the transformers. But I don't know if this will be possible because from spec-tools perspective this is just a regular call as any other.

@wandersoncferreira
Copy link
Contributor

wandersoncferreira commented Nov 9, 2020

@timclemons quick update, I had an idea about how to solve this issue... The PR attached here will disable the transformer variable when a transformer function is called. i think this is a reasonable constraint to avoid stackoverflows.

Therefore, with this PR you do not need to use binding [*transformer* nil] inside your transformer. o/

@timclemons
Copy link
Author

timclemons commented Nov 11, 2020

That PR looks like a sensible approach. Thanks for the work you put into this!

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

2 participants