Fix mixed context key types on reify #393
Conversation
This happens when creating a ledger in reify (i.e. loading from storage).
| context)) | ||
| (if (= :keyword context-type) | ||
| context | ||
| (keywordize-keys context))) |
There was a problem hiding this comment.
I'm sort of confused about this, keywordize-keys already checks if keys are a string. Is this just meant to save that work when it's known to be unnecessary?
And the breakage here was because we were skipping out on keywordization when we shouldn't have?
Sort of feels weird to me to have an implicit enum of context types (keyword or string), but we just sometimes violate that by having no context type (or do they sometimes end up mixed?), and then we have two runtime-check-style fns to clean things up.
(Not a blocker for merging this, more just thinking out loud/trying to make sure I understand.)
There was a problem hiding this comment.
Yeah we could just always call keywordize-keys but if :context-type :keyword is set (the default) then it should be unnecessary.
The case where we have no context-type is when we're reifying from storage. So it's a side effect of these functions be used in two rather different scenarios:
- A user is creating a ledger for their own use and supplies a context-type or gets the default of
:keyword. - We are internally reloading an existing ledger from storage. We have full control over the context in this case, so we could, in theory, make it always strings or always keywords and set the context-type explicitly.
I just seem to keep playing mixed-keyword-and-string-keys whack-a-mole in here. So trying to balance that against not doing more work than is necessary. Definitely an area for additional cleanup / refactoring.
I found I needed this for my additional tests in http-api-gateway to pass. Builds on #392 so we should discuss that first.