APIv2: encoding/prototext: document instability of Marshal

[vadimsht]

I'm writing a tool that produces a bunch of configuration files which are text-serialized protos. These configuration files are checked in into a repository. There may be multiple versions of this tool used as the same time (different developers may use slightly different binaries).

https://godoc.org/google.golang.org/protobuf/internal/detrand makes it produce confusing whitespace diffs when two or more developer work in this config files repo. I believe a use case of checking in text-serialized proto files is quite popular.

Additionally, when writing unit tests, I was very confused why changing a completely unrelated portion of the code makes tests suddenly fail. It looked like a compiler bug :) (it is never a compiler bug...) Consider documenting non-determinism in https://godoc.org/google.golang.org/protobuf/encoding/prototext

But ideally, something like this would be the best:

type MarshalOptions struct {
    ...

    // Deterministic allows to disable randomizations added by the marshaller by
    // default.
    //
    // Note that the output still can arbitrarily depend on the exact version of 
    // protobuf library. Setting this option to true only guarantees that 
    // identical messages serialized by the exact same version of protobuf 
    // library result in the exact same output.
    Deterministic bool
}

That way clueless users would get randomization by default, and more inquisitive ones will discover the disclaimer and proceed on their own risk.

[vadimsht]

Note that Deterministic: true would also imply that map<...> needs to be traversed in deterministic order.

[cybrcodr]

Sent CL to document this -- https://go-review.googlesource.com/c/protobuf/+/188937.

[dsnet]

For your use case, "deterministic" marshaling is not what you want, but rather "stable" marshaling. Deterministic only means that some specific implementation of Marshal produces the same output for the same given input. However, it does not guarantee anything about the output across differing implementations over time. The property you desire is "stability", which means that the output of Marshal is guaranteed to be identical for all time given the same input.

While "stability" is a convenient property for the user, it is harmful to the maintainers since it prevents us from having the ability to make changes to the output for readability purposes or to fix bugs. In fact, many outstanding issues for prototext have been ignored for a very long time out of fear for breaking such users. With the new v2 implementation, we are finally fixing these long standing issues with textproto. Since we anticipate possibly more textproto changes in the future, we deliberately added instability to the output in the new implementation to prevent users from unknowingly depending on stability.

[vadimsht]

Ok, so if I want the output of my tool to not depend on random unrelated modifications to the compiled binary, my only option is to "fork" existing text encoder (assuming it is forkable, I see parts of it live in internal/...)?

[cybrcodr]

How important is stability for your use cases?

(1) You mentioned about using textproto format for configuration and checking these into a repository. I take it that these are configurations consumed by a program, in which case the formatting shouldn't matter much.

I can understand that a user may not want to see too much diffs when reviewing configuration changes. I think having a format tool for textproto files would solve this issue, like what gofmt does for Go source code.

(2) You also mentioned about unit tests. Can you elaborate on this? One should not compare 2 proto messages by comparing their textproto representation.

[dsnet]

There are several possibilities:

1. Modify your use-case to not depend on output stability. That is, instead of relying on the output to match byte-for-byte, make sure the semantic information represented by the textproto is equal by parsing the textprotos and then using proto.Equal. This approach is appropriate for most use-cases that think they want stability (but really don't).
2. Pass the output of prototext.Marshal to some formatter (similar to go fmt) that constrains the the prototext format to a more limited grammar to ensure some reasonable degree of stability. Inside Google, this is increasingly the approach taken when checking in prototext files as source code. Unfortunately, the formatter tool is not maintained by us (i.e., the maintainers of Go protobufs) and I do not know if it is open source.
3. Petition the protobuf team for the creation of a canonical specification of text marshaling. Such a specification specifies a much stricter grammar than what parsing would normally allow and would have to specify everything from whitespace, to field ordering, to integer representation, string representation, etc. This format would enable various language implementations to have a single output that is suitable for your use case. Perhaps the most challenging part of this is specifying a normalized output for all possible floating point values.
4. (as you suggest) Fork encoding/prototext to ensure you have an absolutely frozen version of Marshal for all time. The main pieces of code you would need to copy are encoding/prototext and internal/encoding/text. Since you only care about the marshaler, you can delete all the unmarshal logic (which is most of the complexity, to be honest). Also, since it's frozen code, you could just delete all the tests.

[vadimsht]

Thank you for your answers.

(2) You also mentioned about unit tests. Can you elaborate on this? One should not compare 2 proto messages by comparing their textproto representation.

tl;dr I was confused by undocumented non-deterministic behavior of text marshaller that resulted in apparently flaky tests. Documenting that it is non-deterministic (as in the CL above) is enough to remove this confusion.

I have an existing tool that generates text-encoded proto files (using APIv1 proto library). To be concrete: this one. It executes Starlark code that eventually generates configs (which are text-encoded proto files).

Its unit tests relied heavily on comparing text output to known "golden" files, because a crucial property of the tool is that given same inputs (to same version of the tool), it produces same outputs. Because these outputs go into source control, and we don't want useless noop diffs. Thus unit tests compare text output as raw text, not semantically.

I'm porting this tool to use APIv2 (for ability to load descriptors on the fly and use dynamicpb). I'm doing it before the official release of APIv2 because there's a lot of pain associated with embedding various random protos used by various Starlark scripts from multiple unrelated repos into the tools' binary itself, so it can serialize them (because that's how APIv1 works). I can't handle this pain anymore, so I'm switching to unreleased APIv2, hoping that it will be less painful (and it actually was and I enjoyed using it).

Everything was great until I started porting unit tests that compare text outputs, and discovered that they are "flaky", in a really weird way. That led me to discovery of internal/detrand and how it hashes body of os.Executable, making results of the tests depend on unrelated modifications to the rest of test executable code. This was confusing because most flakes I encounter are usually reproducible by e.g. go test . -count=100. But not in this case... Again, just documenting this behavior in text marshaller docs would have been sufficient to reduce the confusion.

[puellanivis]

we deliberate added instability to the output in the new implementation to prevent users from unknowingly depending on stability.

This reminds me of the release notes for go 1.3: “Programmers were improperly depending upon stable iteration order of small maps. We have fixed the problem.” 😂

[dsnet]

https://github.com/protocolbuffers/txtpbfmt was open-sourced. This makes option 2 in my earlier comment #920 (comment) now a viable option.