[BUG][go-experimental] Explicit nulls support is broken

[bkabrda]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• What's the version of OpenAPI Generator used?
• Have you search for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Bounty to sponsor the fix (example)

Description

The current support for explicit nulls, as modified in PR #3989 by @shaxbee (and approved by myself) is broken. I'm opening this issue to discuss the best approach to fix it - I'm ok with implementing the solution for it.

For any given nullable and not required field, we currently generate code like:

type V struct {
    A *NullableInt32 `json:"a,omitempty"`
}

The problem right now is that when an API sends back an explicit-null field, it's not deserialized properly as an explicit null. This is because for a pointer field, Go unmarshalling machinery doesn't even invoke the custom unmarshall method, it basically considers explicit null to be the same as the field not being present at all.

So there are 2 options:

• Create custom unmarshaller for V
• Don't make A a pointer

Following the reasoning in #3989 about not overloading marshal/unmarshal for these potentially large structures, let's try going with not making A a pointer, hence getting:

type V struct {
    A NullableInt32 `json:"a,omitempty"`
}

This would make deserialization work, as the custom unmarshaller for A would always run, but there's another problem - the custom marshaller for A would always run when serializing. IOW, now there's no way to not serialize this field. But the field is supposed to be optional, so we'll have to create a custom marshaller for V.

So based on the above (if my thinking is correct), we have two options:

• Leave the field to be a pointer and provide custom unmarshaller
• Make the field non-pointer and provide custom marshaller

I'm leaning towards the latter, as I think it's easier. I've been playing with how this could work and came up with [1] - this solution changes how the Nullable* structures work a bit, but it also brings it closer to how this is implemented for Java/jackson clients, using [2].

If everyone agrees, I'll send a PR.

Also CCing members of go language committee: @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

[1] https://play.golang.org/p/O4AOkyVTikT
[2] https://github.com/OpenAPITools/jackson-databind-nullable/

openapi-generator version

4.3.x branch

OpenAPI declaration file content or url

Use anything that has a field marked with nullable: true and for which the API actually sends an explicit null back.

Command line used for generation

Steps to reproduce

Related issues/PRs

#3989

Suggest a fix

See above

[sebastien-rosset]

One consideration is that to address #4881, #4606, #4559, I think it will be required to write a customer unmarshaler.

[shaxbee]

Pointer to nullable field was a bit weird and changing it to non pointer with custom marshaler sounds sensible.

…

On Tue, Feb 25, 2020, 9:32 PM Sebastien Rosset ***@***.***> wrote: One consideration is that to address #4881 <#4881>, #4606 <#4606>, #4559 <#4559>, I think it will be required to write a customer unmarshaler. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5278?email_source=notifications&email_token=ABH6M7O47XJBLQQNUBMIS6LREUTXVA5CNFSM4KS35DV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM4FRXI#issuecomment-590895325>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABH6M7IKHWTM4ASHVTRDBWTREUTXVANCNFSM4KS35DVQ> .

[spacether]

Looks like this was closed by the above PR
#5414