Support nullable types #790

ghost · 2023-01-05T17:49:24Z

Hello everyone! 👋

The purpose of this PR is to support nullable types (#523). This allows to use generated messages in this way:

final user = User();
user.firstName = null;
print(user.firstName?.isNotEmpty());

The main advantage of this solution over hazzers is to take advantage of null safety.

Currently, this PR generates nullable types if the field is a message, or if the field is optional in the proto3 sense. Hazzers are still generated. To generate nullable types, you must use the nullable option, which is set to false by default.

I'm sure this PR is not mergeable in its current state (no test added), but I'm opening it to be helped and to ask questions. Don't hesitate to tell me if I'm going in the wrong direction!

Why is ArgumentError.checkNotNull(value, 'value') used in $_setFloat for example, in generated_message.dart?
Is it a good idea to modify generated_message.dart and field_set.dart?
Is it a good idea to add an option to the generator?
For tests, is it better to copy the current tests, modifying them to check for nullable types, or is it better to add new tests just for that?
Is this feature desired in this package?

Thank you very much in advance!

google-cla · 2023-01-05T17:49:27Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

yingshaoxo · 2023-01-17T05:53:07Z

@johynpapin

Hello? Any updates on this?

grpc/grpc-dart#599

ghost · 2023-01-17T10:39:46Z

@yingshaoxo Hello ! I'm waiting for answers from a maintainer, we have to be patient. 😄

thanhdatvo · 2023-01-26T23:05:30Z

@johynpapin It looks like the CI failed. Could you help to update your code so that it could pass the CI checking?
By the way, I did not go through you code yet but I want to ask if there are any signification code update for user?

Thank you,

P/s I'm not a maintainer

osa1 · 2023-01-27T07:25:22Z

The CI failure is an existing issue. We fixed it in master branch, if you could merge master or rebase the branch it should be fixed.

(I will be reviewing this soon, sorry for the delay.)

ghost · 2023-01-27T09:45:02Z

Hello, I performed a rebase from master 😄
(No need to apologize, it's normal!)

ghost · 2023-01-27T09:48:22Z

@thanhdatvo I made sure that no change is visible if the nullable option added to the generator is not used. (Except adding some methods to the GeneratedMessage class.)
If this option is used, then the fields for which it makes sense (messages and optional) are represented with nullable types. The hazzers are kept.
As I asked in my questions in the introduction of this PR, all these choices seem debatable to me and I would love to have feedback on them.

osa1

This looks great, thanks @johynpapin.

Answering your questions:

Why is ArgumentError.checkNotNull(value, 'value') used in $_setFloat for example, in generated_message.dart?

The library is still used by non-null-safe Dart, so we need to handle the cases where a non-nullable parameter is passed null. We have some tests that run with non-null-safe Dart, see e.g. #791 for some recent work on this.

Is it a good idea to modify generated_message.dart and field_set.dart?

This is fine, but GeneratedMessage members will be usable on messages compiled with or without this feature, so we should make sure they work as expected when called on a message that is compiled without this feature. I added some inline comments on this.

Is it a good idea to add an option to the generator?

Yes!

For tests, is it better to copy the current tests, modifying them to check for nullable types, or is it better to add new tests just for that?

I don't know how to best test this feature yet, I'll need to think more about this. Let me know if you have any ideas.

I think some new tests will be needed as none of the existing tests will be calling the new GeneratedMessage methods.

Is this feature desired in this package?

Yes, I think this is probably the most frequently requested feature.

protoc_plugin/lib/src/protobuf_field.dart

osa1 · 2023-01-31T10:05:29Z

protobuf/lib/src/protobuf/field_set.dart

+          fi, value, 'repeating field (use get + .add())'));
+    }
+    if (value == null) {
+      _clearField(tagNumber);


This will remove required fields from the field set, right? I think that's probably OK (removing required fields is already allowed with clearField), but the documentation says "sets a non-repeated nullable field", and my understanding is only optional fields are nullable.

I think we need two documentation, one for this member with implementation details, one for the GeneratedMessage.setFieldNullable with how the member is supposed to be used. This should say it will just clear the field (same as _clearField). GeneratedMessage.setFieldNullable should clarify what happens when you pass null.

Indeed, I hadn't noticed that!
I'll add some documentation.

I just pushed the changes in the documention, I'm not sure if it's right. 😄

ghost · 2023-02-01T14:46:12Z

@osa1 Thank you for all the answers and the review! 🙏 I'll think about the tests.

bjernie · 2023-03-01T13:27:42Z

Any update?

ghost · 2023-03-01T19:22:53Z

@bjernie Not really, I've been pretty busy lately. But I intend to finish this! 😉

craiglabenz · 2023-05-02T17:01:25Z

How's this going, @johynpapin? Any ways we could be of assistance?

ghost · 2023-05-02T18:17:49Z

Hello @craiglabenz! I've finished the small changes mentioned above, but what's mainly missing are tests. I still don't really know how to properly test these changes, I need to read the ones already written. I'll try to do that before the end of this weekend. 😄

daviddomkar · 2023-06-04T14:44:34Z

Hi. Is there any update?

kevmoo · 2023-07-07T22:08:55Z

please rebase this on the latest!

ghost · 2023-07-08T02:10:47Z

Hi @kevmoo! First thing I will do tomorrow.

Arpit1496 · 2024-01-16T15:10:00Z

Someone is going to have to rebase this on master and we'll go from there...

Done: chitochi#1

Since there were no changes, can we expect it to be merged anytime soon ?

ghost · 2024-01-16T15:43:41Z

Hello !
I've just merged the PR, but I've just noticed that it was going towards the master branch and not the nullable branch. 😆
But I just clicked on the GitHub rebase button so I guess it's ok now?

Arpit1496 · 2024-01-17T13:44:17Z

@kevmoo please check if it is good to go. We won't wanna fall behind master again 😅.

kevmoo · 2024-01-17T15:27:48Z

I think @osa1 needs to help here.

Arpit1496 · 2024-01-19T07:52:23Z

@osa1 Please have a look as this is much needed feature for sound null safety.
Also Let us all know how can we help further.

osa1 · 2024-01-19T08:06:38Z

I'm on leave right now and won't be able to review this any time soon.

Please have a look as this is much needed feature for sound null safety.

This PR is for convenience only. The library is already sound-null-safe, otherwise you wouldn't be able to use it with Dart 3.

Arpit1496 · 2024-01-19T16:45:50Z

Agreed. The library is Sound null safe but we have to use checks which is kinda annoying.
Anyway, Let's try to close this whenever you are back. @osa1 If you won't be attending to this issue kindly can you connect to someone who can?

Arpit1496 · 2024-01-25T13:21:48Z

@osa1 Any update on this ?

Arpit1496 · 2024-01-30T13:30:42Z

Any update on this ?

rostopira · 2024-02-04T21:32:09Z

@kevmoo while osa1 on the leave, can't you review and merge it finally?
Ignoring someones time and effort for a year is very rude, in my opinion.

kevmoo · 2024-02-05T00:12:09Z

sorry @rostopira – thanks for your patience

I don't own or maintain this code. I can't add to the maintenance burden without the owner approving!

Arpit1496 · 2024-02-09T17:29:47Z

Hi everyone, any update on this? @osa1, are you back from leave?

Arpit1496 · 2024-02-18T20:20:13Z

Do you have any updates yet?
It has been more than a year since the community worked on this. Please find some time to review at least.

mraleph · 2024-02-20T09:08:23Z

We currently don't have any engineering capacity to do a proper review of this PR. @osa1 is an engineer with most context, but he is occupied by more pressing things. Eventually he will get to it, but we are not going to promise any concrete ETA on when this happens.

I understand that it might be rather frustrating to have PR languishing in limbo for a while, but unfortunately we are stretched very thin.

hiroshihorie · 2024-02-20T09:22:25Z

I gave up waiting for it to get merged, I just depend on johynpapin:nullable and use it in production at my own risk.

daviddomkar · 2024-02-20T11:10:16Z

We currently don't have any engineering capacity to do a proper review of this PR. @osa1 is an engineer with most context, but he is occupied by more pressing things. Eventually he will get to it, but we are not going to promise any concrete ETA on when this happens.

I understand that it might be rather frustrating to have PR languishing in limbo for a while, but unfortunately we are stretched very thin.

I get it but it is sitting here for a year now. Personally, I am waiting for this PR since its inception. Can you at least try to prioritize this please? I don't see anyone relying on this package when there are wait times this long for a simple but much needed feature to get merged.

ghost · 2024-03-20T11:28:34Z

Hello everyone!

I just resolved the conflicts, if a rebase is preferred I can do one. 😄

I just wanted to say that I personally don't mind things taking time. I haven't been very reactive on this PR either. 😅

EDIT: By the way, I am in the process of deleting the @johynpapin github account. I will be using @chitochi instead. I hope it will not break too many things in this PR. 😅
EDIT 2: I changed the merge to a rebase.

…tion to hazzers. Signed-off-by: Chito <chitochi@proton.me>

Signed-off-by: Chito <chitochi@proton.me>

note that right now, this test is failing

Signed-off-by: Chito <chitochi@proton.me>

daviddomkar · 2024-04-10T14:31:02Z

Is there any update?

daviddomkar · 2024-05-12T12:49:42Z

Hello, are there any plans to get this merged?

ghost marked this pull request as draft January 5, 2023 17:50

ghost marked this pull request as ready for review January 5, 2023 19:09

yingshaoxo mentioned this pull request Jan 17, 2023

optional in protobuf not working in grpc-dart grpc/grpc-dart#599

Closed

mraleph requested a review from osa1 January 17, 2023 11:36

ghost force-pushed the nullable branch from f76f18c to 341839c Compare January 27, 2023 09:43

osa1 mentioned this pull request Jan 31, 2023

[Bug][proto3] inconsistent behavior of assigning null value for optional string #794

Closed

osa1 reviewed Jan 31, 2023

View reviewed changes

yingshaoxo mentioned this pull request Feb 2, 2023

optional nullable is not working grpc/grpc-java#9868

Closed

osa1 mentioned this pull request May 4, 2023

Update to latest lints and Dart SDK #828

Merged

ghost force-pushed the nullable branch 5 times, most recently from 8dc3cbd to 1009692 Compare May 7, 2023 14:51

ghost force-pushed the nullable branch from 1009692 to 2024979 Compare July 9, 2023 15:49

ghost force-pushed the nullable branch from dea18ea to 5413542 Compare January 16, 2024 15:42

osa1 added the publish-ignore-warnings label Jan 20, 2024

chitochi and others added 6 commits March 20, 2024 16:15

Adds the nullable option which allows to use nullable types in addi…

aa2c83a

…tion to hazzers. Signed-off-by: Chito <chitochi@proton.me>

Improved documentation of the two new methods.

f6eb0d6

Signed-off-by: Chito <chitochi@proton.me>

Replaces isNullable with isOptional.

f9b9106

Signed-off-by: Chito <chitochi@proton.me>

fixes

889182d

add test for optional int32

d0f4e55

note that right now, this test is failing

Build protos using the nullable option for testing.

aa7210e

Signed-off-by: Chito <chitochi@proton.me>

chitochi force-pushed the nullable branch 2 times, most recently from f8c18bb to aa7210e Compare March 20, 2024 16:05

fischerscode mentioned this pull request May 11, 2024

allow adding custom plugin parameters pikaju/dart-protoc-builder#22

Open

Support nullable types #790

Are you sure you want to change the base?

Support nullable types #790

Conversation

ghost commented Jan 5, 2023 • edited by ghost

google-cla bot commented Jan 5, 2023

yingshaoxo commented Jan 17, 2023

ghost commented Jan 17, 2023

thanhdatvo commented Jan 26, 2023 • edited

osa1 commented Jan 27, 2023

ghost commented Jan 27, 2023 • edited by ghost

ghost commented Jan 27, 2023 • edited by ghost

osa1 left a comment

Choose a reason for hiding this comment

osa1 Jan 31, 2023

Choose a reason for hiding this comment

ghost Feb 1, 2023

Choose a reason for hiding this comment

ghost May 7, 2023

Choose a reason for hiding this comment

ghost commented Feb 1, 2023

bjernie commented Mar 1, 2023

ghost commented Mar 1, 2023

craiglabenz commented May 2, 2023

ghost commented May 2, 2023

daviddomkar commented Jun 4, 2023

kevmoo commented Jul 7, 2023

ghost commented Jul 8, 2023

Arpit1496 commented Jan 16, 2024 • edited

ghost commented Jan 16, 2024

Arpit1496 commented Jan 17, 2024

kevmoo commented Jan 17, 2024

Arpit1496 commented Jan 19, 2024

osa1 commented Jan 19, 2024

Arpit1496 commented Jan 19, 2024

Arpit1496 commented Jan 25, 2024 • edited

Arpit1496 commented Jan 30, 2024

rostopira commented Feb 4, 2024

kevmoo commented Feb 5, 2024

Arpit1496 commented Feb 9, 2024

Arpit1496 commented Feb 18, 2024

mraleph commented Feb 20, 2024

hiroshihorie commented Feb 20, 2024

daviddomkar commented Feb 20, 2024

ghost commented Mar 20, 2024 • edited by ghost

daviddomkar commented Apr 10, 2024

daviddomkar commented May 12, 2024

ghost commented Jan 5, 2023 •

edited by ghost

thanhdatvo commented Jan 26, 2023 •

edited

ghost commented Jan 27, 2023 •

edited by ghost

ghost commented Jan 27, 2023 •

edited by ghost

Arpit1496 commented Jan 16, 2024 •

edited

Arpit1496 commented Jan 25, 2024 •

edited

ghost commented Mar 20, 2024 •

edited by ghost