Use prototext v2 for rendering options

[marcoferrer]

The original proto text printer would use <> for denoting when a struct start and ends when rendering options. This PR updates the option rendering to use the new v2 APIs which emit {}.

I do need some guidance on the previous usage of theExpandAny flag so that I can see if there is a v2 equivalent.

[jhump]

ExpandAny causes it to use the expanded format for google.protobuf.Any.

So instead of this:

// unexpanded, binary-encoded message as value
type_url: "type.googleapis.com/google.protobuf.Duration"
value: "\b\xf0\x86\x05"

We want this:

// expanded, using special format for Any
[type.googleapis.com/google.protobuf.Duration]: {
  seconds: 82800
}

[marcoferrer]

Just checked and [fqn]: {} is the default and only observable behavior of the new proto printer. So the expected output should be correct

[jhump]

only observable behavior of the new proto printer

Well, it can't do this if it doesn't know about the message type. So it's likely observable if you put in a message name that your program doesn't recognize.

One thing the old API did not provide was a way to provide a custom resolver. The old API always just consulted the global registry of compiled-in types in the proto package (now available in the V2 API in the protoregistry package, in GlobalTypes.

But the new API does allow you to provide a custom resolver. It's a bit of a pain to use since this repo uses the v1 API everywhere else and was designed to work with that old API (as you can tell). To create a resolver, you have to convert the containing *desc.FileDescriptor and its visible imports (all imports, plus transitive public imports) into a set of protoreflect.FileDescriptor objects. This can be done using desc.ToFileDescriptorSet and then protodesc.NewFiles. Then you can construct a protoregistry.Types and populate it by enumerating all types and extensions in the just-converted files and registering them. To create MessageType and ExtensionType from the descriptors, you have to use the dynamicpb package. The resulting types value can be used as a resolver for prototext.

I know that's a lot. Not asking you to do all that (unless you just want to). This is largely a note to self for what this should be doing. Maybe just drop a TODO that this should use a custom resolver on the prototext.MarshalOptions.

[jhump]

This breaks the protoprint tests. You can try simply re-generating the golden output files by changing this var to true, run the test, and then change it back to false. That will then let you diff the changes to the golden output files to make sure they are the changes you want/expect.

[marcoferrer]

So it's likely observable if you put in a message name that your program doesn't recognize. Thats probably it. Since the extension I used was known to the program.

So I found one breakage in the tests. Specifically the test-unrecognized-options.proto

So I implemented the resolver based on the guidance you gave.

func createTypeRegistry(fd *desc.FileDescriptor) (*protoregistry.Types, error) {
	types := &protoregistry.Types{}
	if err := appendToTypeRegistry(types, fd); err != nil {
		return nil, err
	}

	return types, nil
}

func appendToTypeRegistry(registry *protoregistry.Types, fd *desc.FileDescriptor) error {
	fds := desc.ToFileDescriptorSet(fd)

	fr, err := protodesc.NewFiles(fds)
	if err != nil {
		return fmt.Errorf("creating new descriptor files, %w", err)
	}

	var fileErr error
	fr.RangeFiles(func(fileDescriptor protoreflect.FileDescriptor) bool {
		if err := visitFile(registry, fileDescriptor); err != nil {
			fileErr = fmt.Errorf("visiting file %s, %w", fileDescriptor.FullName(), err)
			return false
		}
		return true
	})
	if fileErr != nil {
		return fileErr
	}

	return nil
}

func visitFile(types *protoregistry.Types, fd protoreflect.FileDescriptor) error {

	messages := fd.Messages()
	for i := 0; i < messages.Len(); i++ {
		msg := messages.Get(i)
		if err := visitMessage(types, msg); err != nil {
			return fmt.Errorf("visiting message %s, %w", msg.FullName(), err)
		}
	}

	enums := fd.Enums()
	for i := 0; i < enums.Len(); i++ {
		enumDesc := enums.Get(i)
		if err := visitEnum(types, enumDesc); err != nil {
			return fmt.Errorf("visiting enum %s, %w", enumDesc.FullName(), err)
		}
	}

	exts := fd.Extensions()
	for i := 0; i < exts.Len(); i++ {
		extDesc := exts.Get(i)
		if err := visitExt(types, extDesc); err != nil {
			return fmt.Errorf("visiting extension %s, %w", extDesc.FullName(), err)
		}
	}

	return nil
}

func visitMessage(types *protoregistry.Types, msgDesc protoreflect.MessageDescriptor) error {
	if err := types.RegisterMessage(dynamicpb.NewMessageType(msgDesc)); err != nil {
		return fmt.Errorf("registering message %s, %w", msgDesc.FullName(), err)
	}

	nestedMessages := msgDesc.Messages()
	for i := 0; i < nestedMessages.Len(); i++ {
		nestedMessage := nestedMessages.Get(i)
		if err := visitMessage(types, nestedMessage); err != nil {
			return fmt.Errorf("visiting message %s, %w", nestedMessage.FullName(), err)
		}
	}

	nestedEnums := msgDesc.Enums()
	for i := 0; i < nestedEnums.Len(); i++ {
		nestedEnum := nestedEnums.Get(i)
		if err := visitEnum(types, nestedEnum); err != nil {
			return fmt.Errorf("visiting enum %s, %w", nestedEnum.FullName(), err)
		}
	}

	return nil
}

func visitEnum(types *protoregistry.Types, enumDesc protoreflect.EnumDescriptor) error {
	return types.RegisterEnum(dynamicpb.NewEnumType(enumDesc))
}

func visitExt(types *protoregistry.Types, extDesc protoreflect.ExtensionDescriptor) error {
	return types.RegisterExtension(dynamicpb.NewExtensionType(extDesc))
}

It still didn't fix the output. I need to spend some time tinkering with this to see if I can figure out the source of the issue.

Separately would it make sense to expose a printer config that allows users to choose between v1 and v2 printing of options?

[marcoferrer]

Hit a bit of a dead end / limits to my understanding of the dynamic proto implementation.

I think there maybe be a way to get this to work if the dynamic message factory was able to consult the protoregistry returned by createTypeRegistry instead of the existing extension registry. So far I'm stuck. I think this breakage of the "unknown options" rendering is probably a blocker for progressing with PR. Open to any ideas on what can be done to move forward.

[jhump]

Yeah. This was really why I put the big note on the README about this using API v1.

It's probably best to just punt on this until a future major version (v2) of this package, that directly uses v2 of the protobuf API. What's really needed here is to rewrite this whole package to use the v2 version of a dynamic message (in the types/dynamicpb package).

Sorry.

[marcoferrer]

So I actually got it working. Wanted to see what you thought about the implementation.

[jhump]

So I actually got it working. Wanted to see what you thought about the implementation.

While I admire your perseverance, I think the result is too much. Given that the main objective is just to replace angle brackets with curly ones, adding exported API to dynamic.MessageFactory is overkill.

While I think this change could be done without touching any exported API, and keeping it all local to protoprint, I also think that using the v2 prototext package should really just wait for a v2 of this whole repo (which will use the v2 protobuf API natively and not require all of these shenanigans). For now, it's probably best to leave the use of the v2 text format and just focus on the intent: changing the enclosing brace styles.

[jhump]

FWIW, I ended up taking a completely different approach, to address a TODO in here and also to have more control over the line breaks and expansion of nested message literals and array literals. I basically re-implemented the text format in a custom way. And it also uses the V2 API to manage extensions/custom options, since it's in a PR that should allow much better interop with the V2 API. The tests are still red, but I know why and will fix them soon. If you want to see what's coming, you can check it out here: #354.

[jhump]

I've finally merged #354 to master and plan to cut a release candidate for v1.15 soon. You might check it out to see if it solves the problem you originally encountered.