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

Add option to make message type fields always nullable #9

Open
mscheong01 opened this issue Jun 8, 2023 · 5 comments
Open

Add option to make message type fields always nullable #9

mscheong01 opened this issue Jun 8, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@mscheong01
Copy link
Owner

In proto3, message type fields are always optional; adding the optional keyword does not change the compiled java code
i.e)

message Message {
    Field field = 1;
}
message Field {
    string name = 1;
}

is equal to

message Message {
    optional Field field = 1;
}
message Field {
    string name = 1;
}

However, krotoDC currently generates different code for these two cases

data class Message(
    val field: Field = Field()
)
data class Message(
    val field: Field? = null
)

Because of this, when a krotoDC message type field is not set, while message type fields with the optional keyword are "unset", fields without the keyword is set with an empty message (with all fields unset).
Strictly speaking, this contradicts the proto3 syntax's basic rule; that all message fields are optional. But it is true that this difference could be preferred for certain cases.
The best way to go would be adding an option for the krotoDC protoc plugin that when enabled, creates all message type fields as nullable. then, if this is preferred by the majority of users, we could adpot it as the default behavior

@mscheong01 mscheong01 added the enhancement New feature or request label Jun 8, 2023
@Dogacel
Copy link
Contributor

Dogacel commented Jun 28, 2023

I prefer the current implementation. We have a convention of adding optional to every field explicitly. But generally, It makes more sense to generate all of them nullable by default, because this convention is just a thing we made up.

Also, if field is unset in the non-null implementation, are we seeing a NullPointerException in runtime?

We use https://github.com/bufbuild/protovalidate to ensure field nullability for example. I had a long conversation in pbandk issues. Ability to generate code as nullable or not nullable based on certain options. What do you think about this?

@mscheong01
Copy link
Owner Author

Also, if field is unset in the non-null implementation, are we seeing a NullPointerException in runtime?

No, these fields are set with default instances

@mscheong01
Copy link
Owner Author

I also agree that having an option for both is the best way to go.

@Dogacel
Copy link
Contributor

Dogacel commented Jun 29, 2023

Also, if field is unset in the non-null implementation, are we seeing a NullPointerException in runtime?

No, these fields are set with default instances

One reason why Proto uses unset as the default instance rather than null is recursion

message LinkedList {
  string value = 1;
  LinkedList next = 2;
}

In this case, the generated code shouldn't really work, right? I assume Kotlin code looks like this:

data class LinkedList(
  val value: String,
  val next: LinkedList,
)

Which is basically impossible to initialize. As long as code throws an appropriate error, I like it.

@Dogacel
Copy link
Contributor

Dogacel commented Aug 3, 2023

Sorry for late response, I am reproducing the bug here: #19

Because every message is optional by default, I expect to see this behavior time to time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants