[Bug]: ProtobufferCoding doesn't add explicit presence information to optional message properties.

[Supereg]

Description

To my testing, with the current state of ApdoiniGRPC/ProtobufferCoding, the optional label isn't added to message files where the Swift type is declared with an optional scalar value for the respective field (e.g. the message type Foo has a property like var bar: String).

Reproduction

1. Write a minimal WebService using a type as described above as e.g. a response type:

struct FooResponse {
  var bar: String?
}

2. The generated proto file will contain something like the following:

message FooResponse {
  string bar = 1;
}

Expected behavior

I would expect something like the following to be generated:

message FooResponse {
  optional string bar = 1;
}

I know there was a time, with initial versions of proto3 where this wasn't supported. But afaik this syntax is supported with current versions of proto3 syntax (e.g. refer to https://stackoverflow.com/a/62566052/15337360).

Additional context

CC @lukas

Code of Conduct

• I agree to follow this project's Code of Conduct and Contributing Guidelines

[Supereg]

This will also apply to Input types:

e.g. having the Handler defined as follows:

struct AddReviewHandler: Handler {
    @Parameter(.http(.body))
    var review: ReviewForm
    
    @Parameter
    var eventID: Event.ID
    
    @Parameter
    var userID: User.ID
    
    @Parameter
    var displayName = false
    
    func handle() throws -> ReviewForm {
        try .instance(or: notFound)
    }
}

This will yield the following proto file output.
One would expect the displayName to be optional. It isn' though.

service SomeService {
  rpc AddReviewHandler(.QONECTIQ2.AddReviewHandlerInput) returns (.QONECTIQ2.ReviewForm);
}

message AddReviewHandlerInput {
  .QONECTIQ2.ReviewForm review = 1;
  string eventID = 2;
  string userID = 3;
  bool displayName = 4;
}

[lukaskollmer]

1: Had a look at the missing optional attributes, and it'll be fixed in #426

---

2:

IMO in the case of input types the current behaviour (not making the field optional) is the correct one. ProtobufferCoding doesn't really know that the property has a default value (the property's type within Swift's type system is still Bool, after all).
It would probably theoretically be possible to add limited support for this, but that would work only for fields which are declared directly in a Handler, because in these cases we actually have access to the default value.
Something like the following would not work:

struct CustomInputType: Codable, Content {
    var name: String = "Lukas"
}

struct MyHandler: Handler {
    @Parameter var input: CustomInputType
    ...
}

The problem here is that there is no way to access the default value of a property at runtime (at least not that I'm aware of), so ProtobufferCoding would never even know that the property has a default value and could therefore be made optional.
I'm not sure whether having the behaviour this inconsistent (it would work for properties that are declared w/in the handler, but not for properties that are somehow nested w/in the in-handler properties) is necessarily a desirable thing.

Codable's default implementations don't work particularly well with properties w/ default values, in general.
For example, if you have a type like the CustomInputType struct above, and you attempt to decode it from e.g. a JSON object with the name field missing, that will result in an error (instead of using the default value, as one might expect).

[Supereg]

It would probably theoretically be possible to add limited support for this, but that would work only for fields which are declared directly in a Handler, because in these cases we actually have access to the default value.

I'm also only talking about handling @Parameter declarations. Not doing default values in "regular" Codable instances as it is handled right now is fine and expected. For Codable instances there is a clear separation for encoding/decoding and other initializers (this is what the above CustomInputType example is about. Its basically just a synthesized default initializer). I think the expectation that the value "Lukas" won't be set in init(from:) is given.

The semantic für @Parameter declarations is different though. This is not about Codables. This is about explicitly defining a parameters default value (though, using the inlined property assignment syntax). It is a documented feature of @Parameter. And afaik there is no problem of support it as the default value is pretty much injected automatically if the retrieveParameter didn't provide any value.

I'm not sure whether having the behaviour this inconsistent (it would work for properties that are declared w/in the handler, but not for properties that are somehow nested w/in the in-handler properties) is necessarily a desirable thing.

I don't think this is an inconsistency. On the one side we have a Codable implementation and on the other side is a @Parameter declaration. Both are expected behaviors as to their documentation. What is inconsistent though, is how ApodiniGRPC handles this, as it diverges from a documented feature of the DSL and is divergent from the behavior exposed and supported by all other exporters! A user does not expect this!