identity/redisdir: various fixes #534
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a bug where de-serializing the cached object was confusingly resulting in an unmarshal error.
bnewbold
force-pushed
the
bnewbold/redsdir-fixes
branch
from
ee9c835
to
f570fec
Compare
brianolson
approved these changes
Sep 25, 2024
|
Resolved the review things. My disposition is to merge this, but I want to monitor when this gets deployed (in various places), so i'll probably wait until i'm ready to do a controlled deploy and merge it then. |
bnewbold
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A bunch of cleanups and bug fixes.
The biggest fix is to apply a bug fix from the cache directory resulting in "nil/nil" responses. These were fixed in #479, but not copied to the redis directory implementation, which has the same coalescing code. This recently impacted @whyrusleeping's use in discover. This is a backwards-compatible fix.
Another fix handles cached error messages. An empty string was being stored for the DID. Because redis cache serializes, and the de-serialized DID is re-parsed, the empty strings fail to parse, resulting in an error. I guess we could consider having text parsing of DIDs not actually parse the string? but that would break auto-validation when it is expected. The fix was to make that field nullable. this might make the change not-backwards-compatible with existing cached metadata? this only impacts cached errors, which have a shorter TTL, so might not be that big of a deal in practice?
Other smaller cleanups and fixes, which rolled in here to make the code align better with the in-memory caching directory, so that merge/review is easier:
One remaining issue is that the cached error types come back as generic errors (with error kind/type striped). It would be nice if that wasn't the case, so that code could detect specific types of errors? Considering doing a manual enum/integer hack to encode the type and convert the errors to the actual type on read, for a small set of known identity errors.