fix: no conn context on load #415
Conversation
We were merging the ledger context into the conn context when loading a ledger, since `load` calls into `create`. This commit changes the `create` function to only pass along the context it was provided with unchanged. We really need to stop merging contexts, as that's not how contexts work, and we need to stop normalizing them to keywords, but that's a change for another day.
| ledger-context (->> context | ||
| (util/normalize-context context-type) | ||
| (clojure.core/merge conn-context)) | ||
| res-ch (jld-ledger/create conn ledger-alias (assoc opts :context ledger-context))] |
There was a problem hiding this comment.
General question: why is it desirable in the first place to default to merging conn and ledger contexts at all? Personally I feel like if I were to provide a ledger-level context, my expectation could easily be that it would be the context (ie supplant other context). Merging feels like a more nuanced behavior that I might choose to opt into (and maybe have some control over even), and having it happen automatically behind the scenes feels a bit scary 😅 .
Is this kind of context merging a common workflow for developers using json-ld?
There was a problem hiding this comment.
To clarify, I do understand the idea of a conn context that serves as a fallback for when a ledger context isn't provided. It's the case where a ledger context is provided where I'm wondering about what developer experience expectations might be.
There was a problem hiding this comment.
I think that's a great question, and when it comes to playing the expectations game I think we could make a case for either way - merging or clobbering.
I think we should just get rid of connection default context, as I believe the only thing it makes easier is our job as core developers, because we create ledgers for testing left and right, at the expense of the user experience, where they could assume either behavior might be reasonable and will have to check the docs to see which behavior is actually implemented.
I would extend that to say that the connection shouldn't have any defaults. Every time you create a ledger, you should specify the ledger index configuration, the ledger did, and the ledger context. And I think you should be able to change these by loading the ledger, and maybe provide an api to change them at runtime, though I think that complicates things.
By removing sources of default behavior we can simplify the user experience by removing sources of confusion about how those defaults interact.
To answer your question explicitly: I don't know if context merging is a common workflow for json-ld users, but it would certainly simplify to just not even introduce the question.
In the context of this PR, we can do whatever the team decides the behavior should be, but I'd like to advocate for removing conn defaults.
There was a problem hiding this comment.
I think we should just get rid of connection default context, as I believe the only thing it makes easier is our job as core developers
I think it is pretty critical usability tool for stateless interfaces. Fluree itself focuses on leveraging xsd, f, sh, rdf, rdfs - so any example using at least those (which would be most of them) simply wouldn't work if someone didn't explicitly add them as a first step.
Same goes for private keys if running a central ledger... so I think the defaults play a very important role in easy and DX.
There was a problem hiding this comment.
why is it desirable in the first place to default to merging conn and ledger contexts at all
It didn't used to work like that. When actually using the solution as an end-user I at least found this a tremendous convenience to extend the 'normal', industry-standard defaults (aka merge). I suppose it could be argued the other way, but I think the best usage examples I have today (mostly my own) say this is a nice and I'd be disappointed to see it go.
I suggest we wait for additional feedback and determine if other disagree with me and feel like it should go, but I expect they will find it as convenient as I have.
There was a problem hiding this comment.
I think that's a great question, and when it comes to playing the expectations game I think we could make a case for either way - merging or clobbering.
Yeah, I think you're right about this. It could definitely go either way.
I think it is pretty critical usability tool for stateless interfaces. Fluree itself focuses on leveraging xsd, f, sh, rdf, rdfs - so any example using at least those (which would be most of them) simply wouldn't work if someone didn't explicitly add them as a first step.
That makes sense, thanks.
I suggest we wait for additional feedback and determine if other disagree with me and feel like it should go, but I expect they will find it as convenient as I have.
Yes definitely, I agree. Even if we did unanimously agree here, this is exactly the sort of thing we should have confirmed by feedback before doing anything imo. Right now, if having these defaults is not preventing us from doing our work, I say we wait to hear from users.
There was a problem hiding this comment.
I think the expectations can be made much clearer with an idea we’ve discussed before (though not in awhile): Have two different options for extending the default context versus clobbering that context. So like :extra-context and :replace-context or similar. I’m all for supporting the standard way to clobber contexts, but I find it unintuitive and undiscoverable in this case. So I think we should support something like this too.
There was a problem hiding this comment.
After a conversation with @bplatz recently I realized that there really isn't a right or wrong way to do these various default contexts, because they're just a convenience at heart. They don't affect anything about the actual data that gets retrieved, or anything vital like that. So my initial uneasiness about merging is relieved now, this is really just a DX issue.
That being said, I like your idea @cap10morgan about making the behaviors explicit with some options.
Either way, I don't see a reason not to merge this as-is, we can keep talking about what's most convenient and follow up if need be.
Fixes #413
The conn only holds a default context for creating new ledgers, it's pure syntactic sugar to make that task easier. After ledger creation, a ledger should never again interact with the conn default context.
This PR makes it so.
The actual change is to move any changing of the context out of the ledger
createfunction - now it just stores the context it has been given with no additional processing. Nowfluree/createbuilds its own ledger context and passes it along, whilefluree/loadrecovers the ledger context from the commit and passes that along.