Fix context bugs #373
Fix context bugs #373
Conversation
|
I'll get those conflicts resolved shortly. |
There was a problem hiding this comment.
This is great to see. I'd like to have the context written as its own content-addressed file. Some contexts can be very large... but great first step.
Content-addressed also helps as often the same context is reused, so we only need to store it once.
I'll just comment here so others on the team can approve, but looks great to me.
| [{:keys [conn] :as ledger} commit] | ||
| (go-try | ||
| (let [context (get commit (keyword const/iri-default-context)) | ||
| stringify? (-> context keys first keyword?) ; (too?) simple check if we need to stringify the keys before storing |
There was a problem hiding this comment.
I think they should be JSON in the commit, so don't think this should ever be a keyword unless the JSON read function does recursive keywordizing, which maybe it does. Recursive keywordizing has given us many unintentional bugs, but if thats whats happening for now probably best to leave.
src/fluree/db/json_ld/commit.cljc
Outdated
| (let [context (get commit (keyword const/iri-default-context)) | ||
| stringify? (-> context keys first keyword?) ; (too?) simple check if we need to stringify the keys before storing | ||
| context-str (if stringify? | ||
| (walk/stringify-keys context) |
There was a problem hiding this comment.
We have a simpler util/stringify-keys that I think would work better and fits this use case. Walk not only brings in a whole new namespace + its backing functions for a simple task, it also is more generic to apply to many data structures - where our internal function is a simple reduce designed for simple maps just like this.
| [:rdfs/Class :id "http://www.w3.org/2000/01/rdf-schema#Class"] | ||
| [:rdf/type :id "http://www.w3.org/1999/02/22-rdf-syntax-ns#type"] | ||
| ["fluree-default-context" :id "fluree-default-context"] | ||
| ["fluree-default-context" :rdf/type :f/Context] | ||
| ["fluree-default-context" :f/context "{\"schema\":\"http://schema.org/\",\"wiki\":\"https://www.wikidata.org/wiki/\",\"xsd\":\"http://www.w3.org/2001/XMLSchema#\",\"type\":\"@type\",\"rdfs\":\"http://www.w3.org/2000/01/rdf-schema#\",\"ex\":\"http://example.org/ns/\",\"id\":\"@id\",\"f\":\"https://ns.flur.ee/ledger#\",\"sh\":\"http://www.w3.org/ns/shacl#\",\"skos\":\"http://www.w3.org/2008/05/skos#\",\"rdf\":\"http://www.w3.org/1999/02/22-rdf-syntax-ns#\"}"] |
There was a problem hiding this comment.
Yay!!! So happy to see this gone!
| @@ -62,7 +62,8 @@ | |||
| {:extra-paths ["test" "dev-resources"] | |||
| :extra-deps {lambdaisland/kaocha {:mvn/version "1.71.1119"} | |||
| org.clojure/test.check {:mvn/version "1.1.1"} | |||
| com.magnars/test-with-files {:mvn/version "2021-02-17"}} | |||
| io.github.cap10morgan/test-with-files {:git/tag "v1.0.0" | |||
| :git/sha "9181a2e"}} | |||
There was a problem hiding this comment.
I assume a bug? Might be worth a comment, so in the future we know when/if we can revert to the main repo.
There was a problem hiding this comment.
Nope. The explanation is in the commit message. I forked it to add a handy debugging feature (namely, keeping the tmp dir around after this goes out of scope). Given the age of at least one other PR upstream, I doubt it's worth the trouble to try to get it accepted upstream. Between that and the fact that it's strictly a dev / test dep, I wasn't going to worry about it too much.
Happy to move the fork to the Fluree GH org though, if anyone would prefer that. It probably does make more sense in this case.
That is already in here! :) That's what |
Seeing this when it fails: WARN fluree.db.json-ld.vocab - "Invalid db default context, unable to parse: 150" And the loaded context doesn't match the live one (missing the values set on the ledger)
I hope this doesn't screw up anyone else's contrast, but on my (solarized light) editor color scheme, I can't see the white text at all.
...in reify by making it use the same logic as transact. Fixes #370
...so we can use the {::test-with-files.tools/delete-dir false} option I added when debugging tests.
My editor thinks it should be one way, but it's wrong.
We should either do all of these like this or none of them (I vote none b/c passed-in fn args like this are harder to debug IMO). I did this once already in this branch but accidentally reverted it in a conflict resolution.
cap10morgan
force-pushed
the
fix/context-bug
branch
from
d475f61
to
ca0c481
Compare
Fixes #370 & #371.
This is definitely a "simplest possible thing that could work" approach to #371, and specifically the implementation we decided on of moving the context from db flakes into the commits. So, for example, it leaves it in
(-> db :schema :context)and(-> db :schema :context-str)which feels kind of like the wrong place now (there's a few hacks to make sure the vocab-flakes don't clobber it).Also the imminent PR for #342 is going to conflict with this in some important ways, but I've promised to lead the effort to clean that up.
I also deleted some files that were deleted in the merge of #372 rather than update them to work with the changes in here.