Re: [protobuf] optional vs required
On Tuesday, 9 June 2015 01:27:24 UTC+1, Feng Xiao wrote: On Mon, Jun 8, 2015 at 7:43 AM, Colin Deasy cde...@demonware.net javascript: wrote: Hey, When reading https://developers.google.com/protocol-buffers/docs/proto#simple I see a stark warning indicating that Required is Forever advocating the use of optional with additional application level validation routines. This is because if at some point a required field is no longer written, the readers will break. However IMO, there are common cases where 'required' is a good thing - given that it's enforced only during encoding/decoding. For example there may be some field that is 'required' (right now) to both the reader and writer. Even if that changes at some point in the future to become optional, the reader would likely have to be updated regardless of the protocol decoding routine as it may make assumptions (reasonable considering it was required in the first place) on the presence of the field (e.g. the field being a key to a certain bit of data). In this case the approach would be to update the .proto of all readers to make that field optional, followed by updating all writers to remove the field. In simpler scenarios, yes, it's possible to migrate a required field to optional even though it's an incompatible change, but in a more complicated system, where you have many different binaries using the same proto file running on thousands of machines, it's hard to tell whether all readers of a proto has been updated or not. You have to be very careful with such changes, and if you miss one, bad things can happen. My point is that regardless of the size of the cluster, you will need to update every reader - it doesn't matter whether the 'required' constraint is within the protobuf deserialization logic or within the application logic itself. Similar to missing an update to a certain application instance's proto file, you could miss an update the to application's binary itself. This is precisely why I don't understand the logic behind deprecating the 'required' constraint. Given this, I feel that the current language of the linked document gives the impression that the 'required' attribute is a Bad Thing and should be avoided. I hope you can clarify if I'm missing some crucial bit of information regarding it's usage. Thanks Colin -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+u...@googlegroups.com javascript:. To post to this group, send email to prot...@googlegroups.com javascript:. Visit this group at http://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscr...@googlegroups.com. To post to this group, send email to protobuf@googlegroups.com. Visit this group at http://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout.
Re: [protobuf] Re: Issue 491 in protobuf: Optional field throws NullPointerException when not set values.
As a work-around, you can implement a simple protoc plugin that will generate additional null-friendly methods into the classes already created by the Java codegen built into protoc. That's actually what we've done at Square. As you point out, the null-unfriendliness is particularly burdensome with builders, for cases where you want to use method-chaining but also conditionally set or clear a field. Our generated messages include getOrNull*() accessor fields (so they return null if unset instead of the field's default value) and the builders have setOrClear*() setter methods (that clear the field when null is passed in). *Josh Humphries* Manager, Shared Systems | Platform Engineering Atlanta, GA | 678-400-4867 *Square* (www.squareup.com) On Mon, Jun 8, 2015 at 4:04 AM, Andreas V andreas.v...@gmail.com wrote: I know its an old thread but i use protobuf3 with Java and it has the same NPE behavior with .set(null-reference). Is their any chance to change this? an option to generating java classes would be nice. It is absolutly annoying to null-check on every .set method. This will lead to more errors in code and expand it inadequate due to chained-setters are not possible. :( Am Mittwoch, 29. Mai 2013 23:50:59 UTC+2 schrieb prot...@googlecode.com: Comment #6 on issue 491 by xiaof...@google.com: Optional field throws NullPointerException when not set values. http://code.google.com/p/protobuf/issues/detail?id=491 We don't have the intention to change to the current behavior. Passing null is likely to be a programming error. Allowing it might do more harm than good. -- You received this message because this project is configured to send all issue notifications to this address. You may adjust your notification preferences at: https://code.google.com/hosting/settings -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscr...@googlegroups.com. To post to this group, send email to protobuf@googlegroups.com. Visit this group at http://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscr...@googlegroups.com. To post to this group, send email to protobuf@googlegroups.com. Visit this group at http://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout.
Re: [protobuf] protobuf 3 without threadlocal
On Mon, Jun 8, 2015 at 6:57 PM, Austin Schuh austin.li...@gmail.com wrote:
Pull request sent. I felt a lot better putting the logic in
google/protobuf/stubs/platform_macros.h, so I put it there.
The NO_THREADLOCAL macro does not belong to platform_macros.h. It will not
work either because common.h does not include platform_macros.h so we have
to have a copy in common.h anyway.
My platform is a very old version of Linux on an embedded box. I'm not
sure what to use for PLATFORM, but since I'm already re-creating the build
commands using bazel to integrate the compiler into my project, I am now
able to define GOOGLE_PROTOBUF_NO_THREADLOCAL myself and everything works.
Is there any interest or plans for a set of bazel rules for the protobuf
library?
Protobuf will be supported by bazel natively. See
https://github.com/google/bazel/issues/52
Thanks!
Austin
On Mon, Jun 8, 2015 at 5:42 PM Feng Xiao xiaof...@google.com wrote:
On Fri, Jun 5, 2015 at 7:02 PM, Austin Schuh austin.li...@gmail.com
wrote:
I have a target which is old enough that it doesn't support thread local
variables. I'd like to build the protobuf3 libraries for it.
It looks like all I need to do is trigger arena.h and arena.cc to use
the version of thread_cache that is used by Android which
uses pthread_key_create. One way to do that would be to add
a GOOGLE_PROTOBUF_NO_THREADLOCAL variable and use that. I've got that
working locally as a proof of concept. I'd like to get a change upstreamed
to support this use case. How would you approach solving that? I'll
happily write the patch if it isn't too tricky.
I think we can change the definition in google/protobuf/stubs/common.h to
something like:
#if defined(ANDROID) || defined(IPHONE) ||
defined(SOME_OTHER_PLATFORM_WITHOUT_THREADLOCAL)
#define GOOGLE_PROTOBUF_NO_THREADLOCAL
#endif
#ifdef GOOGLE_PROTOBUF_NO_THREADLOCAL
class ThreadLocalStorage {
...
}
#endif
And also use this macro in arena.h/arena.cc as well.
Thanks!
Austin
--
You received this message because you are subscribed to the Google
Groups Protocol Buffers group.
To unsubscribe from this group and stop receiving emails from it, send
an email to protobuf+unsubscr...@googlegroups.com.
To post to this group, send email to protobuf@googlegroups.com.
Visit this group at http://groups.google.com/group/protobuf.
For more options, visit https://groups.google.com/d/optout.
--
You received this message because you are subscribed to the Google Groups
Protocol Buffers group.
To unsubscribe from this group and stop receiving emails from it, send an email
to protobuf+unsubscr...@googlegroups.com.
To post to this group, send email to protobuf@googlegroups.com.
Visit this group at http://groups.google.com/group/protobuf.
For more options, visit https://groups.google.com/d/optout.
Re: [protobuf] Dynamic/run-time decoding
On Mon, Jun 8, 2015 at 10:23 PM, Jan Kyjovský jan.kyjov...@tieto.com
wrote:
I have remade code so I now add into descriptor database all protofiles
specified by directory. There is but little problem with error handling.
From nature how I am handling errors it seems that message is possible to
decode but there is one error in error collector which is bothering me:
1:1: ERROR: Multiple package definitions.
*My protos are defined as follows:*
*addressbook_fragmented.proto:*
import phone.proto;
package tutorial;
option java_package = com.example.tutorial;
option java_outer_classname = AddressBookProtos;
message Person {
required string name = 1;
required int32 id = 2;// Unique ID number for this person.
optional string email = 3;
// enum PhoneType {
//MOBILE = 0;
//HOME = 1;
//WORK = 2;
// }
message PhoneNumber {
required string number = 1;
optional PhoneType type = 2 [default = HOME];
}
repeated PhoneNumber phone = 4;
optional int32 age = 5;
}
// Our address book file is just one of these.
message AddressBook {
repeated Person person = 1;
}
*phone.proto:*
package tutorial;
enum PhoneType {
MOBILE = 0;
HOME = 1;
WORK = 2;
}
Even though there is error in error collector I can ignore it and decode
message successfully. Yet if I try to remove this package info from
phone.proto itis not detecting any error but FindMessageTypeByName fails to
find data type. I am suspecting since not all references for person data
type I am asking for are resolved it fails.
So I am confused how exactly should be these package keywords be used?
I don't see a problem in your use of package. You could try to run protoc
on these proto files and see if it reports any error. I suspect it's not
the .proto file's problem.
On Wednesday, 3 June 2015 19:05:59 UTC+2, Feng Xiao wrote:
On Wed, Jun 3, 2015 at 12:40 AM, Jan Kyjovský jan.ky...@tieto.com
wrote:
HI,
I somehow understand what you mean but still I lack in experience with
it. I tried to switch to descriptor database but still I got some problems.
C_ProtoDecoder::C_ProtoDecoder(std::string strFile)
{
m_strProtoFile = strFile;
// Build the descriptor pool
m_bConstructed = ParseProtoFile(m_ProtoFileDescr);
if (m_bConstructed)
{
m_ProtoDescrDatabase.Add(m_ProtoFileDescr);
m_Pool = new DescriptorPool(m_ProtoDescrDatabase);
}
}
C_ProtoDecoder::~C_ProtoDecoder()
{
delete m_Pool;
}
bool C_ProtoDecoder::DecodeDataAsTypeFromWireFormat(std::string
strWireFormat, std::string strType)
{
Descriptor *descriptor = (Descriptor
*)m_Pool-FindMessageTypeByName(strType);
Message *message = m_MessageFactory.GetPrototype(descriptor)-New();
if (descriptor == NULL)
{
char szError[256];
sprintf(szError, Unknown data type %s!, strType.c_str());
m_InternalPrinter.AddErrorMessage(szError);
return false;
}
bool bFlag = message-ParseFromString(strWireFormat);
if (!bFlag)
{
m_InternalPrinter.AddErrorMessage(Encoding error!);
}
else
{
m_Transformator.MorphToStructs(*message);
}
return true;
}
bool C_ProtoDecoder::ParseProtoFile(FileDescriptorProto *result)
{
int file_descriptor = open(m_strProtoFile.c_str(), O_RDONLY);
if (file_descriptor == -1)
{
char szError[256];
sprintf(szError, Invalid proto file \%s\!,
m_strProtoFile.c_str());
m_InternalPrinter.AddErrorMessage(szError);
return false;
}
C_ParserErrorCollector errCollector;
io::FileInputStream stream(file_descriptor);
stream.SetCloseOnDelete(true);
io::Tokenizer tokenizer(stream, errCollector);
result-set_name(m_strProtoFile);
compiler::Parser parser;
return parser.Parse(tokenizer, result);
}
I have used SimpleDescriptorDatabase since it looked like it has already
implemented and its covering what I will be requiring from it (or at least
I think it does)
If you use SimpleDescriptorDatabase, you need to add all .proto files
into the database explicitly. For example, if you have 2 .proto files
foo.proto and bar.proto, where bar.proto imports foo.proto, you need to add
both foo.proto and bar.proto to SimpleDescriptorDatabase. If you already
have a list of all proto files, using SimpleDescriptorDatabase should be
fine.
Everything is fine until I try to get descriptor by
FindMessageTypeByName from pool. It returns null. Am I committing some
steps?
I don't see anything obviously wrong in your code.
On Tuesday, 2 June 2015 19:49:53 UTC+2, Feng Xiao wrote:
On Mon, Jun 1, 2015 at 9:55 PM, Jan Kyjovský jan.ky...@tieto.com
wrote:
Hi again,
As I have stated before I am done with decoding, but now I am solving
different type of problem. As I have mentioned before imports may prove
problematic to our implementation.
Let me
Re: [protobuf] optional vs required
On Tue, Jun 9, 2015 at 2:29 AM, Colin Deasy cde...@demonware.net wrote: On Tuesday, 9 June 2015 01:27:24 UTC+1, Feng Xiao wrote: On Mon, Jun 8, 2015 at 7:43 AM, Colin Deasy cde...@demonware.net wrote: Hey, When reading https://developers.google.com/protocol-buffers/docs/proto#simple I see a stark warning indicating that Required is Forever advocating the use of optional with additional application level validation routines. This is because if at some point a required field is no longer written, the readers will break. However IMO, there are common cases where 'required' is a good thing - given that it's enforced only during encoding/decoding. For example there may be some field that is 'required' (right now) to both the reader and writer. Even if that changes at some point in the future to become optional, the reader would likely have to be updated regardless of the protocol decoding routine as it may make assumptions (reasonable considering it was required in the first place) on the presence of the field (e.g. the field being a key to a certain bit of data). In this case the approach would be to update the .proto of all readers to make that field optional, followed by updating all writers to remove the field. In simpler scenarios, yes, it's possible to migrate a required field to optional even though it's an incompatible change, but in a more complicated system, where you have many different binaries using the same proto file running on thousands of machines, it's hard to tell whether all readers of a proto has been updated or not. You have to be very careful with such changes, and if you miss one, bad things can happen. My point is that regardless of the size of the cluster, you will need to update every reader - it doesn't matter whether the 'required' constraint is within the protobuf deserialization logic or within the application logic itself. Similar to missing an update to a certain application instance's proto file, you could miss an update the to application's binary itself. This is precisely why I don't understand the logic behind deprecating the 'required' constraint. If you update your proto files in a backward compatible way, you do not need to update every reader when you change your protos. I think this is one of the main reasons why protobuf is created in the first place. You are assuming we have to update all readers any way for every proto change, but in some cases that's not even possible. And whether the required constraint is with in the protobuf deserialization logic or in the application logic has a crucial difference: if it's in protobuf deserialization logic, it's enforced for all applications no matter whether the application cares about the content of this proto. For example, a log service may receive log messages from clients. It parses the message but only inspect a few fields of it to determine how and where the message will be stored persistently. Clients can define their own log entries and add these entries to the log message. The log service may very likely have a different release schedule from its clients. If a client decides to remove a required field, the log service will start to fail on parsing such messages even though it does not access these required fields by itself. Basically besides the direct producer and consumer of a proto, there may be some other binaries that need to parse/serialize this proto. Changing a proto in an incompatible way is risking breaking these binaries because it's hard to figure out what binaries are parsing your protos in a complex environment. Given this, I feel that the current language of the linked document gives the impression that the 'required' attribute is a Bad Thing and should be avoided. I hope you can clarify if I'm missing some crucial bit of information regarding it's usage. Thanks Colin -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+u...@googlegroups.com. To post to this group, send email to prot...@googlegroups.com. Visit this group at http://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscr...@googlegroups.com. To post to this group, send email to protobuf@googlegroups.com. Visit this group at http://groups.google.com/group/protobuf. For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups Protocol Buffers group. To unsubscribe from this group and stop receiving emails from it, send an email to protobuf+unsubscr...@googlegroups.com. To post to this group, send email to
