kidok / protobuf

Automatically exported from code.google.com/p/protobuf
0 stars 0 forks source link

Make SingleFieldBuilder truly extendable. #451

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
Though SingleFieldBuilder allowed to be subclassed, it's really locked due to:
1. all fields are private w/o accessor methods.   
2. there are no protected methods to alter its behavior.
3. many its methods access protobuf package-private classes/fields, so without 
proper hooks (via protected methods), even the complete source-reuse is not 
possible.

What is the expected output? What do you see instead?
Here are the alternatives in order of the increased openness:
1. (Best and Safest): Inside SingleFieldBuilder.getBuilder() call the protected 
BType getBuilder(MType Message, , BuilderParent parent) which returns a Builder 
for the passing message (the private message field). This way, a subclass can 
control how and Builder is returned. Then there is no need to open up any 
private fields. This case is very important/crucial for any kind of "universal" 
GeneratedMessage Builders, and also is safe. Here is the proposed code:      

public BType getBuilder() {
  if (builder == null) {
    // builder.mergeFrom() on a fresh builder
    // does not create any sub-objects with independent clean/dirty states,
    // therefore setting the builder itself to clean without actually calling
    // build() cannot break any invariants.
    builder = getBuilder(this.message, this);
    builder.markClean();
  }
  return builder;
}

protected BType getBuilder(MType message, BuilderParent parent) {
  builder = (BType) message.newBuilderForType(parent);
  builder.mergeFrom(message); // no-op if message is the default message
}

Thus, subclass can override getBuilder(MType message), for example, by using 
its own concrete BType Builder factory, allowing for any kind of MType message 
and the accommodating concrete BType builder. It is hugely important for 
performance reasons, providing for caching any broadly defined MType message 
and the more or less universal BType Builder which knows how to get itself 
based on on the message. Currently, it is required for the Message to know its 
builder, and can be extended only by making the unnecessary Message wrapper.    

2. (Complete openness, less safe): Make all fields protected, or create and use 
throughout protected accessors methods. 

What version of the product are you using? On what operating system?
protobuf 2.4.1 and 2.5rc1.

Please provide any additional information below.

Original issue reported on code.google.com by dmtes...@gmail.com on 7 Jan 2013 at 4:48

GoogleCodeExporter commented 9 years ago
SingleFieldBuilder and almost all other classes in protobuf package are not 
supposed to be subclassed by users. For this issue we'd better make 
SingleFieldBuilder final to prevent subclassing.
I am curious about why you want to subclass SingleFieldBuilder. What are you 
trying to achieve here?

Original comment by xiaof...@google.com on 7 Jan 2013 at 5:20

GoogleCodeExporter commented 9 years ago
Well, I've implemented a DynamicMessage and DynamicMessage.Builder like
classes to provide the same functionality plus all sub-builders might have
parents as is the case with GeneratedMessage.Builder(-s).
This is a major use-case not covered by DynamicMessage.Builder. It can be
done as efficiently as with the concrete GeneratedMessage.Builder, but I
felt I've been going through the minefield trying to extends anything.
My implementation is based on GeneratedMessage/Builder and
SingleField/RepeatedFieldBuilder because it is already providing this
functionality. It involves a lot of dancing around final, package-private
and the likes.
If I knew that this would not be rejected, I would gladly provide the code
within the protobuf package, and so would avoid a lot of nastiness and
workarounds in it. This code looks like more efficient than DynamicMessage
one, and also would be very similar to the generated codebase.

Last year, during the last week of my contract @Google, I tried to
implement Protobuf OGNL extension; almost done that, and then realized that
DynamicMessage doesn't support BuilderParent. So this is my comeback to the
issue; tried hard many different pathways. The implementation is completed,
but I am trying to make it as efficient as possible, and want to avoid any
workaround wrappers and such. Basically, I've merge DynamicMessage code and
ideas with the generated GeneratedMessage.

Again, if I knew that this idea and contribution would be welcome, I'd
change a bit my existing implementation and commit this code. Otherwise, my
plan is to go on with less efficient one on top of GeneratedMessage and use
it as part of my ProtoBuf/OGNL extension.

Please let me know your thoughts.

Original comment by dmtes...@gmail.com on 7 Jan 2013 at 6:02

GoogleCodeExporter commented 9 years ago
So what you need is the sub-builder support for DynamicMessage.Builder, right? 
I think this is a nice feature to have. Actually we have a TODO there saying we 
need to implement getFieldBuilder() for DynamicMessage.Builder.

Original comment by xiaof...@google.com on 7 Jan 2013 at 8:35