possible improvement of json.RawMessage instead of LexiconTypeDecoder for unknown data
#773
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
ericvolp12
reviewed
Oct 4, 2024
| } | ||
|
|
||
| ops = append(ops, op) | ||
| case w.RepoApplyWrites_Update != nil: | ||
| u := w.RepoApplyWrites_Update | ||
|
|
||
| cc, err := r.PutRecord(ctx, u.Collection+"/"+u.Rkey, u.Value.Val) | ||
| var ltd lexutil.LexiconTypeDecoder | ||
| if err = ltd.UnmarshalJSON(*u.Value); err != nil { |
There was a problem hiding this comment.
Is this adding a marshalling step in repomanager where we didn't have one before? Wondering if there will be performance impacts on this cause this is critical path on relay write-path iirc.
brianolson
reviewed
Oct 4, 2024
| @@ -36,8 +37,12 @@ func FetchAndProcessRecord(ctx context.Context, eng *automod.Engine, aturi synta | |||
| return fmt.Errorf("expected a CID in getRecord response") | |||
| } | |||
| recCID := syntax.CID(*out.Cid) | |||
| var ltd lexutil.LexiconTypeDecoder | |||
There was a problem hiding this comment.
I think it would be cleaner to use util.JsonDecodeValue()
maybe make a utility function that returns (cbg.CBORMarshaler, error) if we want better than (any, error) ?
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.
The indigo codebase supports a couple areas of use:
When writing application code, there is an expectation that all the relevant Lexicon types are known and have pre-generated golang struct types. In that context, LexiconTypeDecoder as a wrapper mostly makes sense. There are some issues with round-tripping (decoding and re-encoding doesn't always yield the same data, if there are new or unexpected fields), but we can ignore that for now.
For the later situation,
LexiconTypeDecoderdoesn't work. If we are only dealing with CBOR blocks (like the relay or raw repo manipulation), it doesn't really matter, can just just avoid serialization, or use theatproto/datapackage for JSON/CBOR round-tripping. But sometimes we are dealing with HTTP APIs and the Lexicon-generated API helpers, and we need to deal with data of unknown type. This can beunknown, or it can be open unions. In this case,LexiconTypeDecoderdoesn't really work.This PR proposes one way of handling
unknown: usingjson.RawMessage(which is basically just[]byteunder the hood). This allows passing arbitrary data around loss-less.A related approach is: #485
Having tried this approach, I think something like the above might be better: make it so LexiconTypeDecoder can deal with unknown types.
A related issue we'll need to address is "open unions" when we don't know the type. Maybe we always have an "Other" field in the enum struct and stuff LexiconTypeDecoder in there.
bluesky-social/atproto#2860