-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
C#: Consider exposing protobuf optional fields as nullable properties #9083
Comments
We get requests in some other languages that have null support to allow optional fields to have nullable properties. The main problem is that we like to keep languages consistent and other languages don't have those same options. Additionally, it's not clear with some primitive fields what a null value would be (is it the default? What if users meant to set it to that option?). For those reasons, we will not support this in the near future. |
Literally the point to optional is to re-enable nullable support that was readily available in v2. (Literally someone in the protobuf team thought they knew better and removed it and got hammered into putting it back) There is no other case. If no value is passed (because it’s optional) it’s ALWAYS a null value in all languages because that’s literally what null means: no entry. In languages without nullable types the property of the object is still set to null they just have to check for null. This is how ALL serializers ever have ALWAYS handled this case. This is the #1 reason we refuse to use GRPC and protobuf. It results in bad coding technique because null in data entry is a baseline requirement. If someone has the option of not entering a value, the value is null. Ie no entry. Not supporting nullable types because other languages are in the dark ages is not a valid argument. As it stands GRPC+protobuf requires bad hacks to deal with a base use case of all CRUD operations that is unacceptable. And no there isn’t some magic value that isn’t null for every data type that can be used in its stead. Only enums have the possibility to overcome the limitation you’re creating and all others (especially bool) have no possible way to represent no entry without using 2 fields in your data model which makes protobuf pointless because it bloats the payload. Please reconsider, and provide baseline functionality that literally ALL serializers in ALL languages supports and is required to provide a good UX without hacks for serializing. And use the full functionality of the language in question. Dart and c# should just work with nullable types as a small example. And this should be ultra high priority for all languages. If the language supports explicit nullability then use it. If not, optional still sets the property to null. If some insane language doesn’t allow a null to be set on a property then it should just throw with optional fields. And honestly its inconceivable that protobuf still doesn’t have basic support required for effective form entry CRUD in 2022 given that Microsoft had this same argument all of the way back in the VB days and created an entire cottage industry of 3rd party controls that charged money to fix Microsoft’s refusal that allow null entry on date and combo controls to name two. It took them until winforms v2 to finally admit they were wrong and fix their mistake. This is a solved problem and a resolved argument that millions of developers voted with their money to prove that the ideology of those that think otherwise is wrong. And I’ve talked to other devs that have evaluated GRPC and protobuf and they’ve have rejected it for this very reason. |
proto2 supports explicitly specifying the default values of fields to be non-zero/empty values. When types get pushed out as language level optional, that fact is immediately lost. Doing what you suggest leads directly to a proliferation of bugs where some callers respect the explicitly specified default from the proto file and others force the question of what value to use instead to every person accessing such a default. The problem is made even worse by languages that provide a very convenient default to zero/empty mapping. |
So you outlined bad behavior by languages and implementations not reasons not to do it right. If there is a default value specified and the field optional then the default value goes over the wire. (Or it should fail to compile the proto because it should be required and have a default. It’s up to you which way you want to go). If there is no default value and optional is specified then null is passed and serialized and deserialized on the other side not some magic default value of the language. Optional and not entered is always, 100% of the time in all cases in all language NULL. Not some default, NULL because null is no entry by all definitions dating back 70 years. It has no other meaning in computer science. Again I’ve used dozens of languages not one serializer I have ever used hasn’t worked exactly as I just described. And that includes Google languages like Dart and Go. Json in Dart for example uses nullable types and works exactly as I outlined above (including default values). All serializers in c# do the same. I’m not aware of any in Java that don’t also. (I’m not as fluent in Java so it’s possible there is). JavaScript either does null or undefined and the language doesn’t support nullable types. So even archaic languages do it correctly. Even VB sets null properly and WSDL and xml in all Languages support null and explicitly define how to serialize/deserialize null values and fields with defaults and the rules are explicitly what I outlined above. And yes, if you do it like 70 years of computer science has solved you eliminate bugs, not create them. We know this because the original case for nullable types when first implemented by c# was explicitly to serialize and deserialize null to relational databases and eliminate all of the bugs caused by failure to check for null. And no it didn’t introduce new bugs because of database default values. Even if the field in the object model was nullable and the database set a default it just meant you’d never get a null, it wouldn’t cause a bug. But the opposite case almost always introduces bugs. (I know because I helped write the specs for nullable types in c# back in the day when no languages had them and the #1 most frequent bug in all languages was “object not set to an instance of an object” or some variation thereof. In nullable type languages that is no longer the case specifically because of this solved problem. ) |
How about this alternative for optional fields that it is again allowed to set the value to null (at least for C# reference types) instead of raising an exception when setting the value to null? This latter behaviour is pretty confusing. As said, clearing the value of an optional sets the underlying field to null already. It's just that it was somehow decided that the property setter needed to be overriden to raise an exception when setting it to null. This is a bit upside down for me. |
Here's my short analysis of the situation, taking into account where we already are (rather than where we might be in an ideal world). The TL;DR is that I can see one change we could make, but I don't propose we actually make it.
The last of these bullets means we can't change the compile-time type of any properties based on whether the field is optional or not. So we can't represent an Message-type fields are already nullable (the value is null by default, and you can set it to null), and that's the way it's always been (in the modern C# implementation starting in 2015, rather than the legacy implementation). So I don't think any work is needed there, for this issue. (There may be further consideration required in terms of what difference, if any, the That leaves just string and bytes fields. These always have a non-null value. In other words, if we have a message of: message TestMessage {
string text1 = 1;
optional string text2 = 2;
} ... then the generated We could allow the property to be set with a null value for optional fields, which would be equivalent to clearing it. So: var message = new TestMessage();
message.Text1 = null; // Throws now and would continue to throw
message.Text2 = null; // Throws now, would not throw after this change
Console.WriteLine(message.Text2 == ""); // Would still print true after this change The fact that after setting the property to null, fetching it still returns an empty string feels really surprising to me. I suspect that the asymmetry would confuse users even more than the current inability to set the property with a null value. Now of course we could consider abandoning the "can't break existing users" axiom above, but that's a much bigger, long-term change. I suspect it's more likely that if we ever create a separate C# generator/support-library for immutable messages (see #9016) we would think about nullity again from scratch for that implementation. |
Wow, that's very helpful and really improves understanding for me.
This makes much sense and I can follow your decision here. String someNullString = null;
var message = new TestMessage();
Console.WriteLine(message.HasText2); // Is false
message.Text2 = someNullString ?? String.Empty; // Ensure we don't assign null
Console.WriteLine(message.HasText2); // Is true, as far as I understand This still breaks projection. In a normal world, I can check HasText2 before assigning, but in a projection, that's not going to work, it is either assign or do not assign. Therefore it is pretty difficult to compose a message right from a database model using projection when the db model contains null strings and the protobuf message has an optional equivalent. |
Glad that helped, @LeonarddeR. Going on:
Please clarify exactly what you mean by "projection". It's the kind of term that's used in a wide range of ways, and I want to make sure we don't talk past each other. From your example, I can imagine either: if (someNullString is string)
{
message.Text2 = someNullString;
} or, if you're not mapping to a new message: if (someNullString is string)
{
message.Text2 = someNullString;
}
else
{
message.ClearText2();
} One option we could explore is introducing a new method: message.SetOrClearText2(someNullString); Using a method rather than a property makes the "I called it with a null value, but the property is still an empty string" significantly less confusing IMO. The downside is that it increases the API surface significantly. I'm on the fence about it personally at the moment, erring on the side of caution (i.e. not making a change we can't undo). I'd be interested to hear your thoughts though. |
I created an example: In short, when using projection, it is not possible to deal with optional values as you either have to call the setter of every property you want to set or leave out the property entirely. |
I don't know what you mean by "do they work with other languages at all" - as far as I'm aware, other languages don't have special handling for the wrapper types in the generator... the types "work" but the generated code just refers to them as normal. (There's special handling in JSON parsing and formatting though.) That was the case when I last looked, but things may have changed since then, of course. It does feel to me like this is one scenario where there's just an impedance mismatch between Protobuf and EF Core, and I don't want to break existing Protobuf users for the sake of removing that. |
It's an impedance mismatch between Protobuf and C#. EF Core is just a single example. But more so, Protobuf has an impedance mismatch with all up-to-date programming languages because it breaks best practices coding (nullable types) that have eliminated the most often seen bug in coding. By not supporting this properly, you're turning back the clock and causing an entire new set of problems, and in specific the Dart and C# version is even worse because you can't even set a field to null ever. It would be a passable hack to allow setting null on a nullable property but not use nullable types, but better would be a property in the csproj file that allows you to opt in to optional fields generating nullable typed fields for ALL types. Other languages then can do whatever other languages want to do and people using C# that want the way it is, which is problematic from the above example but dozens of others, can keep it the way it is. I'd argue that Dart also needs the same flag, as does every other language that supports nullable types. This solves the entire problem, because optional still means optional as defined in the spec, and all languages don't change behavior unless you opt in. Once you get telemetry on the people that use the flag versus those that don't, assuming that you take into account the people that don't know it exists, I think you'll find in very short order you'll be able to phase out the flag and make it the default and invert the flag for the legacy way of doing so. The other alternative is add to proto3 modifier for "nullable" and treat it the way optional should have always been treated: the value is optional, if not entered, it's null, and in all languages that support nullables will generate nullable types, and in all languages that don't, the value will be set to null and you'll have to check to see if it is. This would eliminate the mismatch between protobuf and all current languages that handle nulls properly, AND doesn't break anything in the past AND brings protobuf in line with basically all serializers in all languages which means it becomes far more adoptable. |
I like the first proposal from @JohnGalt1717 Note that Strings still need special handling. Therefore the opt in should only work if nullable reference types are on. If not, there's no way to distinguish between string and optional string. having said that, I guess nullable reference type support in C# isn't at all implemented for the code generator, right? |
I think we've reached the end of fruitful discussion at the moment. Allowing a generator option would effectively pass on a lot of complexity to the generator, and the support library and anyone using a protobuf generated library (as they may well have to deal with different semantics for different libraries). If we start work on a new major version, where backward compatibility with the current generated code isn't an issue, I promise we'll revisit this - but until that point, I don't have the time even to research and plan what I'd consider to be the most appropriate greenfield generated code (which still needs to bear in mind all kinds of constraints for non-C# use cases). |
@jskeet Then Protobuf and GRPC is not viable in any real-world project for even simple crud forms entry and submission to an API because as currently written this adds endless additional code and logic that is ripe for bugs and bloat. Literally every developer that I've talked to that has any exposure to GRPC/Protobuf either complains about all of the noise code because of this or refuses to use it because of this reason. As it stands, I can't create a form using a DTO and submit directly as an example. I have to have a form object and then a translation layer that converts that to the protobuf DTO and works around this for any fields that allow users to not enter a value. That is a MASSIVE waste in productivity, and all of that translation code means bugs. My flag creates a fork in the code. It would be completely separate and would generate nullable typed code in all cases. It would not break any other implementations because as far as I'm aware, only the .NET one enforces never putting nulls in fields in Protobuf. The rest allow you to set null if nullable types is not enabled and the field is optional (Dart has this problem catastrophically and blocking now that nullable types is the default so it will throw errors when assigning null to non-nullable fields just like C#) And for V4, the separate code that was under the flag could be used going forward as it would be the most compatible with the new standard written as it should have always been written (presumably). This is the #1 blocker for any project to use GRPC/Protobuf as soon as anyone dives into it deeply. It was raised in V2, was outright ignored and derided for V3 (and then partially reversed although optional is largely useless as it stands now) and now being delayed to V4 for any substantive fix. Either V4 needs to be accelerated to aggressively fix this, the flag needs to be added to all languages with nullable types, or Protobuf is blocked from serious usage. |
@JohnGalt1717: I'm certainly not trying to persuade you to use Protobuf. As I said before, I think we've reached the end of fruitful discussion. |
It is ridiculous that we need to check for null for strings that otherwise introduce an exception. Why it can't just set HasXXX to false when set null to XXX (optional)? |
@cnSchwarzer: I'm afraid I don't follow the scenario you're talking about at all. Given the range of topics in this issue, I would suggest you create a new issue with a very precise description of what you're seeing. (Ideally, a proto file and sample C# code using it.) (I'm guessing you're setting a property to null, and it's throwing ArgumentNullException when you wouldn't expect it to, but it's not fully clear. Note that I don't expect to change that behavior, as it would be very odd to be able to set a property to null without it then returning null when fetched - and that change would be very, very breaking.) |
Sorry for being rude and not expressing myself well. I know it is very breaking, but consider this situation: array.Select(a => new SomeGrpcObject {
Name = a.Name
}); Which now we have to do I think this behaviour is understandable when this field is not optional: Expect behavior: Moreover, If the |
Because that's never the way it's worked before - when you fetch Name, it will never be null - which means if it starts being null, that's a breaking change. If we'd designed all of the generated code knowing everything we'd end up wanting to do, including the Fundamentally, the answer is the same as I've given in various places in this issue: the change you're describing is a significant breaking change. We may consider that sort of change for a new major version, but not until then. I hope you'll agree now that while you may not like the current behavior, it's not really "ridiculous" after all. |
Yeah, I mentioned ridiculous because it will introduce much more unnessesary code like: array.Select(a => {
var ret = new SomeGrpcObject {
Xyz = a.Xyz
};
if (a.Name != null)
ret.Name = a.Name;
return ret;
}); It is sure not ridiculous, but |
I agree it's inconvenient - but I don't think we can fix that inconvenience without breaking things. Of course, if you don't actually care about the presence bit, you could use |
this is a fundamental architectural design flaw, not expected from a big company. messagepack seems like a good alternative: https://msgpack.org/index.html |
Can you bump a major version to break change, and redesign using modern .NET? If you can't I would plan to write my own lib. |
If we do create a new major version, we'd look at this as one potential change. But we're not going to do it just for this. (The cost of a new major version is going to be very significant to the whole community, due to the number of things that depend on protobuf.) |
Quite some time later, but I find working with protobuf-generated c# objects painful for this reason. I have code like @cnSchwarzer mentioned here all over the place, and I'd argue that it makes code harder to reason about (and certainly more burdensome to write). @cnSchwarzer did you ever write your own lib? Edit: I just realized that the google-provided wrappers do what I'm looking for. Doesn't really solve the underlying issue (in cases where you have to use |
Unforturnately no, this thing does very tight integration with Visual Studio and I found not that worty to spend time implementing another one. Anyway, I still stand by my point, not using optional with C#'s ? is definetly a design flaw and should be fixed ASAP in next major version. |
Is edition 2023 enough |
No. We've implemented edition 2023 in C# without any breaking changes. (To remove any doubt: taking a major version is going to be a huge amount of work, not just for Google but for MS as well, given all the gRPC templates etc. There has to be a really good reason to do so.) |
Nullable types is a really good reason to do so. Like earth shatteringly good reason. |
How about: Bad API design? |
I still struggle to see what breaking change is required for adding an option to C# proto compiler, something like "emit_defaults" in Go protojson formatter. It would turn this
into this:
or this:
into:
You would make many people happy and you can still keep the current "dialect" as default. |
@git-torrent: It would be possible, yes. We've tried to keep the command-line options less semantically-meaningful than that usually. (Reasoning: if two people generate code from the same proto, it's odd if code using the result compiles for one and doesn't for the other.) It would make things harder in the reflection parts of the library, but it would at least be feasible. (I would say that making the library harder to maintain is not appealing - we have few resources for maintaining protobuf on .NET as it is, without making life harder for ourselves.) I'll mention it to folks on the protobuf team and see whether the policy on command-line options is as it was before. |
Another possibility that I've raised with the protobuf folks just now is having a field descriptor extension for this - that would avoid the problem of "two people generating the same proto getting different results", and make the runtime reflection work easier. I'll post a reply with whatever I hear back. |
Thanks for the comments, everyone. I've discussed this with the protobuf team, and in doing so gained new insight into how presence is expected to work. The experience of most C# developers with protobuf is with proto3, which uses implicit presence for primitive fields other than those with the optional keyword (which introduces explicit presence). In proto2, explicit presence was the default - and that's also the case in Edition 2023 (see FeatureSetDescriptor for full defaults). Importantly, the expectation is that even when field presence is explicit, most usage will still not care - which means the generated code should make access to the value (whether present or not) as simple as possible. This also allows for custom default values, which would be hard to achieve in the nullable design. Effectively, this means that "protobuf explicit field presence" and "C# nullable value types" sound the same on paper, but have different use cases, and we don't want to use the latter to model the former. The protobuf team's experience with trying to use a nullable-like model in other languages has been that it doesn't work out in practice. As such, we don't plan to implement this feature request. I understand this is not the result most of the subscribers to this issue are looking for, and I don't expect everyone to be convinced by this line of reasoning, but this is the path we're taking. |
So, why are we throwing exceptions when setting null to non-primitive types? Can you at least remove that null check when optional is present? It is so wierd that we have bunch of _hasBits0 but still throw exceptions when we just don't want to set the value, "implicit presence". |
To avoid inconsistency. (This was something I remember considering a very long time ago. I doubt that I could find the design notes now.) I think it would be odd to have: message.SomeStringField = null;
Console.WriteLine(message.SomeStringField.Length); // No exception, just prints 0 I think that would be odd regardless of whether presence is explicit or implicit. This is basically what I said earlier:
I think this is one of those situations where reasonable people can disagree on the best design. Note that for message types, where setting a property to null is allowed, you get null back for unset fields too - so for any given property, it's consistent. (It means that message type handling is inconsistent with String/ByteString handling, but in a way that I believe ends up giving the desired outcome in most cases.) |
Why not just letting set null results in clearing _hasBitForStringField? and returns null (or StringFieldDefaultValue) when this bit is not present? It is not odd if the default value could be null, and if a default value is set the user should be expected to have a value even it was set null just line above. It is just same as clearing bit method but can simplify some code. It is not even breaking, as setting null, just skip the field being serialized. |
It would be very breaking for unset string values to start returning null instead of the empty string. |
Anyway, I think setting null could be an alias for clearing the bit, it is truly odd to not returning null but it is not breaking, just expect some missing exception while it make no harm. I think it is acceptable to me if well documented, as it can make code much more fluent. |
I disagree. The documented behaviour is that it throws is the value is null - so not throwing an exception can be used to infer that the value is not null, leading to a later change in behaviour. Even if we were to change this in a major version, I definitely wouldn't do so in a minor. (And I don't think I would want to do it even in a major, due to the oddity of the behaviour.) |
The issue is that there is already "optional" which should have always been if not entered or explicitly set to null, then null in every language that is nullable aware. Now you're going to have to add yet another keyword because it was defined badly in a way that no one actually uses to mean something that doesn't even really make sense. And we're all just asking for the people in charge of protobuf to fix the mistake and then every language implementation to align to it because it's just brutally bad as it stands right now making Protobuf useless in virtually all real world cases because it has no concept of the most basic of concepts: (i.e. character 0 in ASCII !!!) null. |
As @jskeet indictated, we know this is not the decision that will make folks happy, but it is the decision that aligns with current design and prioritization/staffing considerations. If there is a major redesign in the future, we will definitely use this discussion as an input. As this issue has been discussed for > 2 years and the conversation seems to be looping back on itself, I'll be locking it here. Thanks Everyone! |
Originally posted as grpc/grpc#27501 as I thought it belonged to @grpc
Is your feature request related to a problem? Please describe.
The way optional fields are implemented in C# is slightly hacky. For Strings, the underlying private field can be null, but the public property raises an exception upon null assignments. For non nullable (value type) fields, the underlying magic is even more scary.
Describe the solution you'd like
Consider implementing optional fields as nullable properties in C#. This has the following benefits:
The text was updated successfully, but these errors were encountered: