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

Datalevin doesn't play well with sci (through malli) #230

Closed
jeroenvandijk opened this issue Oct 24, 2023 · 6 comments
Closed

Datalevin doesn't play well with sci (through malli) #230

jeroenvandijk opened this issue Oct 24, 2023 · 6 comments

Comments

@jeroenvandijk
Copy link
Contributor

Thank you for creating Datalevin.

I ran into the error "No reader function for tag datalevin/regex" when using Datalevin in combination with a Malli validation.

The Slack discussion can be found here.

The problem is that Datalevin adds a print-method for all regular expressions. The resulting serialized form (e.g. # datalevin/regex "foo") cannot be understood by Sci which is used by Malli under the hood.

This issue can be reproduced by executing the following gist:

git clone https://gist.github.com/jeroenvandijk/71cea769b3b2850f896cd501a32395a5
clj -X magic-error/show-me

The gist also has a workaround.

@jeroenvandijk jeroenvandijk changed the title Datalevin doesn't play well with other libraries like sci and malli Datalevin doesn't play well with sci (through malli) Oct 24, 2023
@huahaiy
Copy link
Contributor

huahaiy commented Oct 27, 2023

Because some data contains regex, in order to store and retrieve such data, print-method for regex was added, e.g. they are often used in transaction functions.

We can make this print-method optional for regex in order to support your use case. I can add a function set-confg to allow setting some global configurations. In this case, (set-config :store-regex? false) can be called to disable adding print-method for regex.

@jeroenvandijk
Copy link
Contributor Author

jeroenvandijk commented Oct 28, 2023

I understand it is part of the serialization format. I would argue though that including the Datalevin namespace in your Clojure project should not break something that can be considered standard Clojure code. Especially for new users it would not be obvious to use your proposed setting. Also, what do you do when you need both. Maybe there is an alternative solution (idea below)?

Note: I haven't touched Datalevin much so I'm not sure about the impact of any change to this project.

;; in datalevin.bits

;; Don;t interfere with Clojure by default
(def :^dynamic *datalevin* false)

;; For Datalevin types *datalevin* is true
(defmethod print-method Indexable
  [^Indexable i, ^Writer w]
  (.write w "#datalevin/Indexable ")
  ;; This is Datalevin territory
  (binding [*datalevin* true]
    (.write w (pr-str (pr-indexable i)))))

;; Store original print-method
(defonce stdlib-print-method-pattern (get-method print-method java.util.regex.Pattern))

;; Depending on the *datalevin* value we will adapt printing for regex (and maybe others
(defmethod print-method Pattern
  [^Pattern p, ^Writer w]
  (if *datalevin*
    (doto w
      (.write "#datalevin/regex ")
      (.write "\"")
      (.write ^String (s/escape (.toString p) {\\ "\\\\"}))
      (.write "\""))
    ;; => Default Clojure's print behaviour here
    (stdlib-print-method-pattern p w)))

@huahaiy
Copy link
Contributor

huahaiy commented Oct 28, 2023

As per discussions on slack, requiring people to have a binding to use Datalevin functions seems a bit excessive, so we will keep the set-config function proposal, and hope people who have similar issues will land it here.

@den1k
Copy link
Contributor

den1k commented Oct 31, 2023

Because some data contains regex, in order to store and retrieve such data, print-method for regex was added, e.g. they are often used in transaction functions.

is this because pr-str is used to write this data? Would it help to use custom functions similar to nippy's serialize?

@huahaiy
Copy link
Contributor

huahaiy commented Oct 31, 2023

Using print for regex is when importing/exporting data as text, so it is possible to only add the print-method when exporting.

@huahaiy huahaiy closed this as completed in ac76197 Nov 1, 2023
@huahaiy
Copy link
Contributor

huahaiy commented Nov 1, 2023

Just limit the change of print-method to dumping the DB.

huahaiy added a commit that referenced this issue Nov 12, 2023
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