Update transact/create handling to use new insert syntax #8
Conversation
| [expanded-txn] | ||
| (-> expanded-txn | ||
| (get const/iri-opts) | ||
| (get 0) |
| ;; monitor for new index files and push across network | ||
| _ (push-new-index-files config index-files-ch) | ||
| resp (fluree/commit! ledger db {:file-data? true | ||
| :index-files-ch index-files-ch})] |
There was a problem hiding this comment.
Can you re-align those let bindings?
| (-> ledger | ||
| fluree/db | ||
| (update-default-context defaultContext) | ||
| (fluree/stage txn' opts) | ||
| (update-default-context default-context) |
| opts :opts} parsed-txn | ||
| [tx-id txn*] (normalize-txn txn) | ||
| [p consensus conn watcher expanded-txn] | ||
| (let [ledger-id (-> expanded-txn (get const/iri-ledger) (get 0) (get "@value")) |
There was a problem hiding this comment.
Starting to feel like we could use a get-expanded-value (or similar) fn along the lines of:
(defn get-expanded-value
[expanded iri-key]
(-> expanded (get iri-key) first (get "@value"))There was a problem hiding this comment.
Since this is dealing with jld-processor/expand output and not json-ld/expand output, I thought it would be confusing to have two virtually identical functions that won't work if you pick the wrong one. I know the manual unthreading is verbose but it's very readable and nobody will get bit by using the wrong helper down the road. I can still try to abstract if you like, but in this case I didn't think the abstraction was worth the cost.
There was a problem hiding this comment.
Hmm... not sure I follow. I saw the exact same sequence of fns applied in several places and suggested turning that into a fn that gets called instead. But maybe I missed the places that were slightly different? Proper namespacing & docstring might be good if you think it might o/w look tempting to misuse. But yeah, I think it makes sense to turn into a named fn. I'll leave the final decision for this PR up to you, though.
| default-context (assoc :defaultContext default-context))] | ||
| (let [[expanded-txn] (util/sequential (jld-processor/expand body)) | ||
| ledger-id (-> expanded-txn (get const/iri-ledger) (get 0) (get "@value")) | ||
| resp-p (promise)] |
| tx-id (hash-txn txn*)] | ||
| [tx-id txn*])) | ||
| (let [canonized (jld-processor/canonize txn)] | ||
| (hash-txn canonized))) |
There was a problem hiding this comment.
Could be simplified to (-> txn jld-processor/canonize hash-txn)
| persist-resp-ch (consensus/queue-new-transaction | ||
| consensus conn-type ledger tx-id txn* default-context | ||
| opts)] | ||
| persist-resp-ch (consensus/queue-new-transaction consensus conn-type ledger tx-id expanded-txn nil nil)] |
There was a problem hiding this comment.
Can you split this up into two lines? (My tall, narrow editor windows thank you in advance!)
| @@ -11,8 +11,9 @@ | |||
| (let [ledger-name (create-rand-ledger "query-endpoint-basic-entity-test") | |||
| txn-req {:body | |||
| (json/write-value-as-string | |||
| {"f:ledger" ledger-name | |||
| "@graph" [{"id" "ex:query-test" | |||
| {"ledger" ledger-name | |||
There was a problem hiding this comment.
alignment (and others below)
| "@context" ["" {"foo" "http://foobar.com/"}] | ||
| "@graph" [{"id" "ex:create-test" | ||
| {"ledger" ledger-name | ||
| "@context" ["https://ns.flur.ee" {"foo" "http://foobar.com/"}] |
There was a problem hiding this comment.
I think this should be in the conn default context if we're going to need it in every transaction. I think it would make more sense to keep its f alias in there, though, to keep that namespace cleaner.
There was a problem hiding this comment.
I've updated the default conn default context to include it, but users will still need to specify at least the https://ns.flur.ee as an explicit @context value in every transaction, at least for now. That way credentials will work and we can always expand the transaction without having to check the conn default context.
I do think there's a path towards not including it, it just isn't implemented yet.
In the transaction handler we have to use the `jld-processor/expand` instead of the `json-ld/expand` because this transaction will get expanded again inside of `fluree/stage2`. Unfortunately, the output of `json-ld/expand` can't actually be put back in to another call to `json-ld/expand`. It is ok to use jld-processor in this context because there are only 5 top-level keys and the expansion doesn't proceed to nested nodes because top-level keys are @type @JSON, or scalar values ("ledger"). We also don't need to do much transaction parsing as it is all handled in `fluree/stage2`, so we can just pass the whole expanded txn without making modifications to it. Also deleted `do-default-context-update` as that function was unused. Updated all the tests to use the new transaction syntax. Some of the query results don't have the order guaranteed, so I put the results into a set to compare. In one case (in policy-test), the transaction was relying on the old upsert semantics, so I had to introduce a delete clause to maintain the expected state.
This is very similar to the transaction handling changes, with the added twist of handling defaultContext. To get this to work expediently I've just put the ledger creation opts under the `opts` key. This is @type @JSON and so doesn't get expanded. The map is extracted, the keys keywordized, and then it is passed without any further processing as the ledger create opts. I was able to get rid of the normalize-txn function because nobody uses it anymore. I also had to change the ref syntax in the basic-query-test. It seems like there's a bug in json-ld/expand that doesn't know how to deal with @type @id.
dpetran
force-pushed
the
feature/new-transaction-syntax
branch
from
aea7ae8 to
4bf02b0
Compare
|
Rebased on main to resolve conflicts. |
This updates server to use the transaction syntax.
Depends upon fluree/json-ld#27