No empty object#965
Conversation
|
|
||
| # Empty Objects | ||
|
|
||
| Fields of type Message **must not** be empty. Empty messages do not have uniform |
There was a problem hiding this comment.
I think that this sentence is too broad in its guidance. Empty messages are very useful for future proofing APIs, whether it be a response message, Operation metadata, or just a nested message in a resource. I don't think the use of empty messages should be banned completely. It looks like you have a couple of specific scenarios, I think the guidance should focus on those scenarios, and potentially outline when it is OK to use empty messages (i.e. in the scenarios i've mentioned above).
There was a problem hiding this comment.
Hm, ok how about something like:
Messages within the body of resource requests or responses must not be empty.
No problems with empty responses as a concept, or empty operation information. Just empty objects within resources are the target
jskeet
left a comment
jskeet
left a comment
There was a problem hiding this comment.
I'm really, really uncomfortable with this proposed change - which isn't part of the GURP deltas as far as I can see.
| bool enabled = 1; | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I don't remember seeing this as purely enablement, outside a oneof. Often it's "here's a message which might contain more information later" and its presence is indicative of the choice for that feature.
There was a problem hiding this comment.
Agreed, it's usually an object that could contain more information later. I think that's where adding the enabled flag within the object works well, as new fields can be added alongside that field as they become available.
One of the problems I'm trying to solve here is that there are many ways to implement behavior off of an empty message, none of which are clear to the user. To a user that is new to an API seeing an empty object documented in the REST docs is confusing, and it is unclear how or why to use such a field.
Some of this confusion is contributed to by our generated documentation for empty objects like the following. DLP primitive transformations have a field replaceWithInfoTypeConfig that is an empty object. The generated documentation for this field seems broken to a viewer, but is technically correct.
I don't think that the documentation generation is to blame here though. This pattern is nonstandard and nonintuitive to users, which is why generated tools and docs end up with a sub-par experience.
There was a problem hiding this comment.
But enabled won't always be suitable, for multiple reasons:
- Depending on the context, it may be invalid to have a configuration which isn't enabled, because it's always got to be used to retrieve something. Basically you're asserting that there are always more valid scenarios than an empty message implies, and I don't believe that assertion is justified.
- Adding
enabledrequires more work for anyone populating a configuration; it's easy to forget to explicitly enable it, at which point it may be hard to work out what's wrong - If it is valid for
enabledto be false, then a user retrieving a configuration where it's false will still see an empty object - is that really any better than the current situation? (I'd argue it's actually worse, if they've only accidentally left offenabled.)
I agree that we can and should improve documentation - but I believe that's all that we should do here. I don't believe we should be artificially requiring an additional field.
As a side-note, I'm going to do some research on:
- How many (and which) messages are currently empty across all Google APIs
- How many (and which) messages have been empty but then become non-empty
That may provide some additional food for thought.
| message AccessModelQuota { | ||
| int32 requests_per_day = 1; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Except you've then got far more potential for conflicting options having their details specified at the same time.
The oneof pattern is much more sensible IMO... if tools are unable to handle empty messages, I'd far prefer them to be improved.
There was a problem hiding this comment.
I tend to agree that the proposal of sidelined enum configuration over the values in the enum itself has poor usability: it requires the user to look at two different values (the enum and the corresponding configuration message) to reason about which value is correct.
I can see the argument about an empty top-level message, but what's the issue with an empty enum value? it's clear what the type of the object is from the enum itself.
| enum AccessModel { | ||
| UNSPECIFIED = 0; | ||
| FREE = 1; | ||
| QUOTA = 1; |
| For example: | ||
| ```proto | ||
| oneof access_model { | ||
| AccessModelFree free = 1 |
There was a problem hiding this comment.
would it make sense for this oneof to have unspecified? or some default value, as with any other enum value?
IIRC that works around backwards compatibility issues where the returned enum is not a value that is present in older versions of a client.
Just for the GURP context - this isn't a ratified delta yet, but is based on a discussion around difficulties in handling existing empty message objects, and it's usage to enable a feature despite the fact that protobufs will normalize it to the empty object anyway. @slevenick I think it's worth providing more rationale in the AIP itself, or in the description of this PR. |
4 participants
Add AIP guidance on not allowing empty Messages. Number and title TBD, I just chose a placeholder