Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: implement/add isEmpty() on AbstractMessage, DynamicMessage, and GeneratedMessage #55

Closed
protobufel opened this issue Oct 16, 2014 · 3 comments

Comments

@protobufel
Copy link

At present, to find whether the MessageOrBuilder is empty one needs to call the following:

  messageOrBuilder.getUnknownFields().asMap().isEmpty() 
          && messageOrBuilder.getAllFields().isEmpty();

It is cumbersome and expensive ( getAllFields() in many cases would recreate the fields' map!).
However, to implement it from within the MessageOrBuilder, all needed is to just call internal map.isEmpty() or such.

There are so many valid and common cases for emptiness check, the are done often and in many places, so this API and performance optimization is crucial, IMHO.

There are two implementation alternatives:

  1. with minimal/zero impact on compatibility - implement isEmpty() on basic classes AbstractMessage, DynamicMessage, and GeneratedMessage and their Builders.
  2. more complete and consistent API, but could be impact on pure Message/Builder implementations, is implement option 1 plus add isEmpty() to (Lite)MessageOrBuilder interface.

Any implementation alternative will also solve the "is MessageOrBuilder default?" question, which currently resolved differently for GeneratedMessage and DynamicMessage.

The alternative 1 will require casting to these basic classes, so it is not w/o cons.

This request is not extremely urgent, so it could be implemented anytime after 2.6.1.

@xfxyjwf
Copy link
Contributor

xfxyjwf commented Oct 16, 2014

Besides the approach you mentioned, you can also check whether a message is empty by "message.getSerializedSize() == 0". It's recommended and works for lite runtime as well.

Regarding the default instance of DynamicMessage, I think DynamicMessage.getDefaultInstance() not returning a singleton is a bug.

@protobufel
Copy link
Author

  1. message.getSerializedSize() is expensive for non-empty messages; and it is awkward as well.
  2. In order for DynamicMessage to have a singleton defaultInstance that can be compared with == you'll have to maintain internal thread-safe static Map<Descriptor, DynamicMessage> cache of defaults, which is relatively expensive.
  3. In all cases, the most universal, easy to implement, and easiest and most conventional to use is isEmpty(); or if to go a bit further, size() in terms of the total number of fields. It is as conventional, as the Collection API, well understood and widely used.

@acozzette acozzette added the P3 label Jun 8, 2018
TeBoring pushed a commit to TeBoring/protobuf that referenced this issue Jan 19, 2019
Added migration flag for users using old JSON format.
@elharo
Copy link
Contributor

elharo commented Oct 1, 2021

decided not to do this to avoid complexifying the API and adding extra surface to test and support

@elharo elharo closed this as completed Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants