Fix/remove internal context #701
Conversation
src/fluree/db/json_ld/transact.cljc
Outdated
| (let [ctx (:context parsed-opts) | ||
| s-ctx (:supplied-context parsed-opts) | ||
| parsed-txn (q-parse/parse-txn txn ctx) | ||
| db* (if-let [policy-identity (perm/parse-policy-identity parsed-opts s-ctx)] |
There was a problem hiding this comment.
Do we need a separate ctx and s-ctx? I think the ctx removes the fluree named context just so we don't shadow the user's vocab terms by accident (if they want to define where or insert or whatever. I think we do need the fluree named context for the policy-identity, though, so maybe that should just be a documentation note.
There was a problem hiding this comment.
This is a good point. I don't think the policy stuff is done. I have a TODO somewhere else regarding that, but I'll add a todo here too so we can clean this up when we streamline the apis and do the required parsing/validation/transformation steps all at once.
There was a problem hiding this comment.
I think we do need the fluree named context for the policy-identity, though
The context passed to the policy-identity process is just used to expand the role iri. That doesn't require the fluree named context
There was a problem hiding this comment.
When I added expansion for role iris that doesn't use default context (in this PR #693), I added the :supplied-context opt to differentiate it from the context used elsewhere in the transaction process. At the time, the "txn context" was the result of a calculation which included default context. So this was a workaround to remove dependency on default-context from role iri expansion, without ripping out default context altogether. But I think there should just be one context, and it should be the one supplied by the user in the request. That's what should be used to expand role iris
I see now that the TODO I left was misleading, I didn't mean that we were to preserve the separate :supplied-context opt itself, rather the opposite
There was a problem hiding this comment.
I think ideally we would just have ctx, and if I understand Marcela's point then the s-ctx idea was just a workaround until we removed default context, so I think we would be safe just using ctx here.
There was a problem hiding this comment.
I just pushed a commit removing it and cleaning up some more context handling.
This patch removes default contexts from connection, ledger, and database objects and updates the tests not to rely on them. Now the only contexts ever considered while processing a query or transaction are the contexts supplied along with said query or transaction.