-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: get default context at t #525
Conversation
...which doesn't exist yet as of this commit
...so it doesn't need so much horizontal scrolling to read
This was a fn in this context, not a db value.
...that takes a ledger and a t value and returns the default-context at that t if it exists.
Realized I didn't yet implement support for ISO datetimes which the original ticket calls for. Marking this as draft until that's in place. |
@fluree/core This is ready for review now |
ff444d4
to
3509ee5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏸
src/fluree/db/ledger/json_ld.cljc
Outdated
_ (when (util/exception? commits) (throw commits)) | ||
[{:keys [f/commit]}] commits] | ||
(->> commit :f/defaultContext :f/address | ||
(jld-reify/load-default-context db-conn) <?)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny formatting nitpick: this is a little weird-looking to me with the <?
where it is, maybe it could go on its own line? or everything in this ->>
on its own line?
H/T to Dan for the suggestion
...to make it all consistent with a prior formatting commit
This arguably belongs in here, but the specific motivator (getting default-contexts at a specific t value) no longer needs it. So I'm reverting it for now.
...and a node test for defaultContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎛️
f.d.api is no more
Ugh... I wish this wasn't needed and that it would just respect the ^:export metadata
:f/address test-utils/address? | ||
:f/alias ledger-name | ||
:f/branch "main" | ||
:f/defaultContext {:id test-utils/context-id?} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of our files have a globally unique content-based hash (storage/location agnostic) @id, and also one or more :f/address where that file can be loaded from.
Not sure if defaultContext never had both of these, or if the :f/address got lost along the way, or maybe this isn't actually testing what physically exists in the file.
Just noticed it, so wanted to inquire to see if :f/defaultContext currently has that pattern, or if we need to create a ticket to get it updated.
This is the db component of https://github.com/fluree/core/issues/9. There will be an http-api-gateway counterpart coming up shortly.
In implementing this I added an
:f/address
key to the:f/defaultContext
in our commit details data structures so that this can use it to load the context from storage. Previously it was just a DID, which I don't think can be used directly for that purpose (yet?). This necessitated updating some test expectations inf.d.q.history
.I also fixed a looming bug I noticed where we were referencing a fn named
db
as though it were a db value, but it was being passed to a stub fn. So it was fine for now but would have turned into a subtle bug once we fleshed out the fn that was being called. Let me know if I should split that out into its own PR. This is in ffe56fd.The core of this feature is in 9620187.