Closed avocade closed 3 years ago
eneroth
commented
3 years ago The problem is this line:
If replaced with
(binding [*print-namespace-maps* false] (pprint-str (summary-form show-valid-values? form in)))… it works fine. I.e., it seems like a similar issue to previously: walk tries to get into the map-like object and do replacements.
avocade
commented
3 years ago Maybe use something similar to the recursive walk used in groups-walk to fix the previous bug:
bhb
commented
3 years ago Thanks for reporting this!
bhb
commented
3 years ago Do you happen to have a repro including the spec? I'm having trouble reproducing this.
bhb
commented
3 years ago Also, what options are enabled? https://github.com/bhb/expound#printer-options
bhb
commented
3 years ago Actually, I found a repro. Nonetheless, if you could provide details about your repro and options, that would help ensure I fix the core bug.
My repro:
(s/def ::id int?)
(is (= ""
(expound/expound-str
(s/cat :db (s/keys
:req-un [::id]))
[db]
{:show-valid-values? true})))))Do you have :show-valid-values? set to true? What happens if you set that to false?
bhb
commented
3 years ago In particular, I'm curious what specs you use to validate your Datomic DB.
avocade
commented
3 years ago Hey, sorry we should have posted some repro code. Will try to do that on Monday 👍
bhb
commented
3 years ago @avocade No problem at all, I appreciate you reporting the bug.
I think I have a fix for my simple repro, but I'm curious about how you spec the db value. I'm a little worried my first attempt at a first won't work if someone uses {:show-valid-values? false}, but I'm having trouble reproducing in that case.
eneroth
commented
3 years ago I think I've managed to produce a small failing example. The error it gives is slightly different depending on whether I use a Datomic DB or your fake DB.
(def fake-db (FakeDB. {:a 1}))
(s/def ::value string?)
(expound/expound-str
(s/keys :req [::value])
{::value 'not-a-string
:db fake-db} ;;or (d/db conn)
{:show-valid-values? true, :print-specs? true})With fake-db, it gives
Execution error (AbstractMethodError) at poc.db.attribute.query.FakeDB/iterator (REPL:-1).
Method poc/db/attribute/query/FakeDB.iterator()Ljava/util/Iterator; is abstractWith (d/db conn), it gives
Execution error (ClassCastException) at (REPL:1).
nullAs previously with walk, it mostly works OK until there's a failing spec. In this case, I can't get it to fail by only giving it the DB and some failing spec, such as nil?. It seems to be that I have to have a failing spec in the vicinity of the DB.
Edit: I just now saw that you have already produced a failing case, sorry 🙂
eneroth
commented
3 years ago Actually, I found a repro. Nonetheless, if you could provide details about your repro and options, that would help ensure I fix the core bug.
My repro:
(s/def ::id int?) (is (= "" (expound/expound-str (s/cat :db (s/keys :req-un [::id])) [db] {:show-valid-values? true})))))Do you have
:show-valid-values?set totrue? What happens if you set that tofalse?
Yeah, we encounter these problems via guardrails, which uniformly uses {:show-valid-values? true, :print-specs? true} as args to expound-str.
I can't reproduce the problem with these switches off (or, I think, only :print-specs?). But in general, I think the takeaway is that walk always is an unsafe option when it comes to processing structures where "map-like" objects may occur, due to the excessive deconstruction and reconstruction it does as it walks through a tree.
bhb
commented
3 years ago Thanks for the info! If you're able, do you mind posting the real-world spec you use to spec your db, if any? This will inform my possible solutions here.
But in general, I think the takeaway is that
walkalways is an unsafe option when it comes to processing structures where "map-like" objects may occur, due to the excessive deconstruction and reconstruction it does as it walks through a tree.
I agree, but doing some sort of traversal and modification of the data is necessary for summarizing failures when :show-valid-values? is set to false. I'd like to preserve that feature, since it can significantly shrink the output. That feature would seem particularly useful for this case, since I'm guessing printing out the db is quite large (although I could be wrong, I don't use Datomic).
One way to make this work is just to skip this step when :show-valid-values? is true. I don't see any downside to doing that, but my concern is this is going to come up again when someone changes that option to false.
I'm wondering what Expound features can reasonably be supported on a Datomic DB. Maybe I can support all features if I take some inspiration from Specter and find a safer way to modify a DB. Or maybe that's not necessary and I can just skip certain features if someone is trying to spec a non-data value, like an atom or a Datomic DB 🤔
eneroth
commented
3 years ago Thanks for the info! If you're able, do you mind posting the real-world spec you use to spec your
db, if any? This will inform my possible solutions here.
We create an env map with some information in it (the DB at a revision, the conn, a timestamp, and so on). This we throw around many places, and perhaps most significantly, we merge it into Pathoms env map, meaning that it's subject to any spec failure that might occur on Pathom's side (since any spec failing will cause walk to try to rewrite the entire data structure, even when the DB isn't involved directly in that particular spec).
We have given up on speccing conn and db themselves, since it's very hard to assert what's a correct value here (and Datomic doesn't include any db? or conn? predicates 🙂). So it's always the case that it's not about the specs for the DB itself, but the specs for the values in the map around it, and those can by anything.
I agree, but doing some sort of traversal and modification of the data is necessary for summarizing failures when
:show-valid-values?is set tofalse. I'd like to preserve that feature, since it can significantly shrink the output. That feature would seem particularly useful for this case, since I'm guessing printing out thedbis quite large (although I could be wrong, I don't use Datomic).
I don't think one thing has to be sacrificed for the other, rather I think it's about using a more careful algorithm. I.e., ensure that mutation only occurs at the site where the predicate is actually true.
This is partly the reason why I tend to break updates into two steps, like you saw with the Specter function in the previous commit: first collect the sites at which the predicate is true, then surgically alter them exactly where it is needed, as opposed to taking the structure apart and putting it back together while chasing the sites where the predicate is true (which walk secretly seems to do).
I'm wondering what Expound features can reasonably be supported on a Datomic DB. Maybe I can support all features if I take some inspiration from Specter and find a safer way to modify a DB. Or maybe that's not necessary and I can just skip certain features if someone is trying to spec a non-data value, like an atom or a Datomic DB 🤔
I think the answer lies in where your predicates might reasonably return true: :expound.problems/irrelevant, for example seems very, very unlikely to show up in any map-like objects that might occur in the specced data (unless you try to inject it into the map-like object I suppose!). Therefore, you can update those values however you want, given that those updates don't "spill over" and try to disassemble everything else while doing so. 😁
Problems seem to arise when the structure is updated in places you did not intend.
bhb
commented
3 years ago So it's always the case that it's not about the specs for the DB itself, but the specs for the values in the map around it, and those can by anything.
Thanks, that's very helpful!
bhb
commented
3 years ago I think the answer lies in where your predicates might reasonably return true:
:expound.problems/irrelevant, for example seems very, very unlikely to show up in any map-like objects that might occur in the specced data (unless you try to inject it into the map-like object I suppose!).
Sorry, I was unclear. In order to support hiding irrelevant data, Expound makes a new version of the specced data structure that will contain many copies of :expound.problems/irrelevant (along with other data that allows underlining the bad value). This works fine in practice if the specced data is plain data, but the problem occurs when we try to spec values like Datomic DBs.
One solution is to rework the algorithm for finding irrelevant data entirely, but that seems potentially out of scope to solve this case. I'll think more on this.
bhb
commented
3 years ago If you have the ability to depend on a SHA version in your project, do you mind trying https://github.com/bhb/expound/pull/210/commits/18c82da80e6bc8f467301dc18750d7013a78f885 and seeing if it fixes the issue for you?
avocade
commented
3 years ago I did a very quick check and it seems to work fine now with https://github.com/bhb/expound/commit/18c82da80e6bc8f467301dc18750d7013a78f885 👍 Having a datomic db value in a failing spec just prints the spec error in the console as we'd expect. Thanks!
bhb
commented
3 years ago Thanks again for reporting this and testing the fix! This has been fixed in 0.8.9
avocade
commented
3 years ago Thanks!
There seems to still be a crash issue when a spec fails and a datomic db is involved.
This time it's the
highlighted-valuefn (inexpound.printer), which fails in a similar way (datomic.core.db.Datum cannot be cast to class java.lang.Number) as the previous issue that was fixed in:https://github.com/bhb/expound/issues/205
The printer uses
walk, which was also implicated in the issue before.Here is a stacktrace, starting from
expound.printer$highlighted_value: