bhb / expound

Human-optimized error messages for clojure.spec
Eclipse Public License 1.0
924 stars 24 forks source link

Internal error in `lift-singleton-groups` when having a datomic db value in fn args #205

Closed avocade closed 3 years ago

avocade commented 3 years ago

We've found an issue where lift-singleton-groups crashes when a datomic DB is one of the arguments. The crash is related to its use of walk/postwalk, which assumes DB can be treated as a regular map.

The problem can be reproduced by running:

(require '[datomic.client.api :as d])
(walk/postwalk identity [(d/db conn)])
eneroth commented 3 years ago

The specific error message:

java.lang.ClassCastException: class datomic.core.db.Datum cannot be cast to class java.lang.Number
(datomic.core.db.Datum is in unnamed module of loader 'app'; 
java.lang.Number is in module java.base of loader 'bootstrap')
Jaretbinford commented 3 years ago

A Datomic DB (d/db conn) is not a map and does not return a map. It is a database.

bhb commented 3 years ago

I’m afraid I don’t fully understand (I don’t use Datomic, so I may be missing something).

Are you writing a spec for your database? What happens if you try to check the spec without using Expound e.g. using s/valid? or s/explain using spec?

eneroth commented 3 years ago

Screenshot 2020-11-12 at 17 48 03

It implements the map interface, which causes postwalk to blow up.

eneroth commented 3 years ago

I’m afraid I don’t fully understand (I don’t use Datomic, so I may be missing something).

Are you writing a spec for your database? What happens if you try to check the spec without using Expound e.g. using s/valid? or s/explain using spec?

We encountered this when using Guardrails, which in turn leans on Expound. We have functions which take a DB as an argument, and we are specing those functions. If a Datomic DB is present as an argument to a function, postwalk will blow up, it doesn't matter if the spec passes or not (it can be some?, for example, and the error above still occurs).

The problem is reproducible when calling expound/expound-str directly as well.

We've solved this locally by leaning on Specter rather than postwalk. I'll issue a PR so you can look it over.

eneroth commented 3 years ago

I'm guessing the same problem would occur with any object that answers true to the predicate map?, but isn't actually an an instance of IPersistentHashMap. We just happened upon this with the Datomic DB object.

bhb commented 3 years ago

Ah, I see. Thanks for the context and for reporting this bug!

bhb commented 3 years ago

@avocade @eneroth Thanks for reporting this bug and for the patch! I ended up going a slightly different direction with the fix, but my patch should still solve the root cause.

If you're using deps.edn, do you mind trying out bdccedc96c65e0d104e6139121a501453007f398 and making sure it solves your issue?

Thanks!

avocade commented 3 years ago

Cool! 🙏🏻

eneroth commented 3 years ago

If you're using deps.edn, do you mind trying out bdccedc and making sure it solves your issue?

I've tested it out, and it all looks green!

avocade commented 3 years ago

Please let us know when you have the next release out, so we can ping Tony over at https://github.com/fulcrologic/guardrails with a PR! ☺️

bhb commented 3 years ago

@avocade I've just released 0.8.7 which includes this fix.

avocade commented 3 years ago

Cool that's great! Thanks a lot, will try it soon.