Re-attach proto2 extensions when loading meta-data from JSON - #4477
Open
hatyo wants to merge 2 commits into
Open
Re-attach proto2 extensions when loading meta-data from JSON#4477hatyo wants to merge 2 commits into
hatyo wants to merge 2 commits into
Conversation
`load schema template ... from <file>.json` parses the meta-data with `JsonFormat`, which implements the proto3 JSON mapping and therefore discards proto2 extensions. Options carried by an extension are lost even though the file spells them out, which silently turns a vector field into a plain bytes field, and drops the CloudKit extensions of `Index`, `RecordType` and `FormerIndex` along the way. After the ordinary parse, walk the JSON alongside the builder it was parsed into and set every key that names a known extension of the message at hand. The extensions are looked up in a registry built from the generated classes of the dependencies the loader already resolves.
ScottDugas
requested changes
Aug 24, 2026
| // the fields of the extension value are regular fields, so JsonFormat handles them just fine | ||
| final Message.Builder valueBuilder = Objects.requireNonNull(defaultInstance).newBuilderForType(); | ||
| try { | ||
| JsonFormat.parser().ignoringUnknownFields().merge(value.toString(), valueBuilder); |
Collaborator
There was a problem hiding this comment.
Why ignore unknown fields?
| private static Object extensionValue(@Nonnull Descriptors.FieldDescriptor extension, | ||
| @Nullable Message defaultInstance, | ||
| @Nonnull JsonElement value) { | ||
| switch (extension.getJavaType()) { |
Collaborator
There was a problem hiding this comment.
Should this be a switch expression?
| return byName; | ||
| } | ||
| for (Descriptors.FieldDescriptor field : descriptor.getFields()) { | ||
| if (field.getJsonName().equals(name)) { |
Collaborator
There was a problem hiding this comment.
Is it possible that the json name of a field equals the regular name of a different field?
| return result; | ||
| } | ||
|
|
||
| private static RecordMetaData loadRecordMetaDataFromJson(String jsonFileName) { |
Collaborator
There was a problem hiding this comment.
It seems worthwhile to add at least two tests:
- This method specifically parsing out a sample metadata with some vectors
- A YamlTest that depends on a json metadata that has vectors in it
It might also make sense to pull this out into a separate utility class
| @Nonnull ExtensionRegistry registry) { | ||
| final Descriptors.Descriptor descriptor = builder.getDescriptorForType(); | ||
| for (Map.Entry<String, JsonElement> entry : json.entrySet()) { | ||
| final ExtensionRegistry.ExtensionInfo extension = registry.findImmutableExtensionByName(entry.getKey()); |
Collaborator
There was a problem hiding this comment.
This gets called for every json key, right, so it would get called for things like name/number/label/type
?
| } catch (InvalidProtocolBufferException e) { | ||
| throw new RuntimeException("unable to parse value of extension " + extension.getFullName(), e); | ||
| } | ||
| return valueBuilder.build(); |
Collaborator
There was a problem hiding this comment.
It's irrelevant for the vector case, and probably any other existing case, but should this also call mergeExtension, recursing in case the extension has fields with extensions.
I'm ok with concluding that it would perhaps be nice to do later when we have a use case
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
load schema template ... from <file>.jsoncommand parses meta-data withJsonFormat, which implements the proto3 JSON mapping and, as its javadoc says, treats proto2 extensions as if they did not exist. The extensions are present in the file, they are just dropped on the way in, and with them goes information the meta-data cannot do without. A vector field is abytesfield whose precision and dimensions live in an extension ofgoogle.protobuf.FieldOptions, so a schema template loaded this way silently ends up with a plainbytesfield where avectorwas meant.This change adds a pass over the meta-data that runs after the ordinary parse and walks the JSON alongside the builder it was parsed into, setting every key that names a known extension of the message at hand and recursing into the message-valued fields, so that options nested anywhere in the descriptor are covered. The value of an extension is itself an ordinary message, so
JsonFormathandles its contents; only the hook-up was missing. The extensions are looked up by full name in a registry built from the generated classes of the resolved dependencies together with the option protos of the record layer, which keeps the meta-data format diffable JSON rather than raw bytes.This fixes #4478.