Skip to content
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

Make defaultContext a node #457

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Conversation

cap10morgan
Copy link
Contributor

Closes #449

Depends on fluree/json-ld#18

A couple things I'm slightly unsure of:

  1. In commit-data/add-commit-flakes when the default context's sid already exists, do we need to add that flake (line 506) or is that redundant?
  2. Would default context flakes ever show up in retracts? I'm not filtering them out currently, but we should if they can. Probably depends on how the new defaultContext updating code works.

@cap10morgan cap10morgan requested a review from a team April 25, 2023 17:42
Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. My guess is that the flake added for the defaultContext is redundant, but perhaps @bplatz could chime in on that and whether defaultContext flakes could appear in retracts

src/fluree/db/json_ld/reify.cljc Outdated Show resolved Hide resolved
@@ -494,11 +501,20 @@
(flake/create new-issuer-sid const/$xsd:anyURI issuer-iri const/$xsd:string t true nil)])))
message-flakes (when message
[(flake/create t const/$_commit:message message const/$xsd:string t true nil)])
default-ctx-flakes (when-let [default-ctx-iri (:id defaultContext)]
(if-let [default-ctx-id (<? (dbproto/-subid db default-ctx-iri))]
[(flake/create t const/$_ledger:context default-ctx-id const/$xsd:anyURI t true nil)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this flake get created even when there was another context (different from the default) supplied with the commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't since that's just a context, not a default context. I don't think it would given the different key naming.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good call where you have it.

I think we could go any way, including not storing it as a Flake at all. I think the balance is thinking if it will be useful in a query for people to ask questions.

I think the most important question will be when did the context change and what are the addresses... so having flakes to answer this I think would be useful:

{:select [?t ?ctx-id ?ctx-address]
 :where [[?t :f/defaultContext ?ctx-id]
               [?ctx-id :f/address ?ctx-address]]}

Which is exactly what you have.

You are also linking the ?ctx-id to every t. That might be a bit excessive, but I can see it being useful for asking for it at a specific t (e.g. t=42 here) with:

{:select [?ctx-id ?ctx-address]
 :where [[42 :f/defaultContext ?ctx-id]
               [?ctx-id :f/address ?ctx-address]]}

Even perhaps excessive, it is a relatively compact amount of information so if not needed, its not like we are paying a super high price.

So I like your decision here. I think we'll get better feedback once we have more users and can change course if necessary then.

@cap10morgan cap10morgan merged commit 7464f65 into main Apr 26, 2023
@cap10morgan cap10morgan deleted the fix/default-context-in-commits branch April 26, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update how default context is stored in commit
3 participants