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

Consider adding Nullable wrapper for non-required nullable properties #1039

Closed
emarcotte opened this issue May 1, 2023 · 13 comments
Closed

Comments

@emarcotte
Copy link

When using the "strict" server types for a simple object definition like:

TestCase:
  type: object
  properties:
    hello:
      type: string

It renders to something like:

type TestCase struct {
	Hello *string `json:"hello,omitempty"`
}

This construction does not naturally let you determine if the field hello was included in a payload or if it was explicitly set to null.

The current best work around I can see for dealing with this is to set x-go-type to a json.RawMessage and define my own wrapper struct containing an Optional[string] as described here:
https://stackoverflow.com/questions/36601367/json-field-set-to-null-vs-field-not-there

Would it be reasonable to add a configurable alternative to generate something like this automatically so that both null and absent can be detected in a strict server?

@jamietanna
Copy link
Member

Similar to #1061

If the field is set to null surely that's the same as not being sent at all?

@emarcotte
Copy link
Author

Consider for example a resource that maybe looks like:

{
  "a": "some nullable string value",
  "b": "some other nullable string value"
}

When designing a PATCH endpoint you may wish to be able to send a partial update, for example to express "Change a to hello", the patch payload might be something like:

{"a": "hello"}

In this model knowing that B was unspecified allow us to avoid updating it while procesing the request. If we do want to set B to null, we could send:

{"a": "hello", "b": null}

Basically, there is difference in JSON between null and undefined (its also why go's map lookup has the v, ok variant). The messages are literally different but there is no way to capture that easily with the existing code generation.

@jamietanna
Copy link
Member

Is this addressed by #1102 being completed? 👀

@emarcotte
Copy link
Author

emarcotte commented Sep 29, 2023

I might be mis-understanding, but I don't think it changes the problem.

Consider this field:

        something:
          type: string
          nullable: true
          x-go-type-skip-optional-pointer: true

Which turned into this member in the input struct with 1.15.0:

Something        string  `json:"something"`

Passing null in the json body results in an empty string in the Something attribute, so I've changed the problem from not being able to distinguish between undefined and null to now being unable to distinguish between null and "".

EDIT: Actually, its slightly more than that, I cant distinguish between null, undefined or empty string.

@xobotyi
Copy link

xobotyi commented Nov 6, 2023

@emarcotte as for me your definition should yield a pointer to string.

I've been trying to solve similar issue and unable to come up with solution in current state of generator (models and client).

In case nullable have priority over x-go-type-skip-optional-pointer problem solves by itself - such combination will allow to distinguish null (which becomes a marker of absense), zero-value and some value.

As go lacks of a null-undefined distinction (thank goodness), I think it's justified to combine these two if it is configured so.

@emarcotte
Copy link
Author

As go lacks of a null-undefined distinction (thank goodness), I think it's justified to combine these two if it is configured so.

This is true when considering a struct with attributes, but it is not true if you consider that jsonschema is describing objects and tools like this are trying to project those onto some a native structure.

Go itself does understand the difference between undefined and nil when it comes to maps. Consider:

number := 3
things := map[string]*int{
  "defined": &number,
  "nil": nil,
}

for _, key := range []string{"defined", "nil", "undefined"} {
  if v, ok  := things[key]; ok {
    fmt.Printf("key: %s It's %v\n", key, v)
  } else {
    fmt.Printf("key: %s - It's not defined\n", key)
  }
}

Which will output the 3 different states rather than aliasing nil and undefined together, something like:

❯ go run main.go
key: defined It's 0xc0000120a8
key: nil It's <nil>
key: undefined - It's not defined

To me, this is exactly describing the semantics described with an object with not-required-but-nullable fields.

Coming back to openapi-codegen, my basic idea here is to come up with a standardized way to document an object structure that has attributes that may not be provided. The approach I describe with a generic Optional type works, but it's in my own repo and has to be copied into any other projects.

I'm willing to provide PRs if there is interest, if there's not I'll just leave it on my own repos.

@xobotyi
Copy link

xobotyi commented Nov 8, 2023

With maps - yes, it is true, but not true with structures, and we usually operate with structures.

My team actually sitting on own fork of this library but v2 forced to perform migration and I wanted to try to do as less changes to generator as possible this time by slightly changing spec generation (we generate spec from code).

Basically we have pretty simple logic -

  • requred controls omitempty
  • nullable controlls pointer, and this also applied to arrays and maps

Thats it =)

@sebgl
Copy link

sebgl commented Dec 6, 2023

An example of the use of wrapper types in ogen (another Go OpenAPI code generator tool): https://github.com/ogen-go/ogen#generics.
This wraps optional types into a generated OptNilString type (for a string). This allows the handler code to distinguish between a non-provided field vs. an explicitly null field.

@jamietanna
Copy link
Member

jamietanna commented Dec 6, 2023

Hey folks, apologies for missing the last few messages.

Current state

If we have:

Obj:
  properties:
    field_name:
      type: string
  required: [field_name]

Would generate:

type Obj struct {
  FieldName string // JSON tags omitted
}

And if we make it optional:

Obj:
  properties:
    field_name:
      type: string

Would generate:

type Obj struct {
  FieldName *string // JSON tags omitted
}

Proposed

Right now, I don't see us handling nullable properties differently, so we could add:

Right now, nullable implies a pointer to the field. Instead, we'd want to take:

Obj:
  properties:
    field_name:
      # NOTE: this would be supported via OpenAPI 3.1
      # type: [string, 'null']
      # NOTE: this only works for OpenAPI 3.0.x
      type: string
      nullable: true

And generate:

type Nullable[T any] struct {
  Value T
  Set   bool
  Null  bool
}

// TODO: Implement custom `MarshalJSON` and `UnmarshalJSON`

type Obj struct {
  FieldName Nullable[string] // JSON tags omitted
}

Would that be a suitable solution?

This must be opt-in via a flag in OutputOptions that would allow configuring this, to reduce the risk of this breaking behaviour of folks who are expecting the pointer type.

@emarcotte
Copy link
Author

Seems viable on a quick glance.

@jamietanna jamietanna changed the title Consider adding Optional wrapper for non-required nullable properties Consider adding Nullable wrapper for non-required nullable properties Dec 11, 2023
@sonasingh46
Copy link
Contributor

sonasingh46 commented Dec 21, 2023

Looks like we can do with

type Nullable[T any] struct {
  Value *T
  Set   bool
}

If Set is true -- this means a value was provided in JSON ( even an explicit null )
If Set is false -- this means a value was not provided in JSON

@jamietanna
Copy link
Member

Worth confirming if this is something that may be made easier by golang/go#63397

jamietanna added a commit to sonasingh46/runtime that referenced this issue Jan 9, 2024
We have spun out a separate package, `oapi-codegen/nullable` as a step
towards oapi-codegen/oapi-codegen#1039.

Until we have implemented oapi-codegen#27, we cannot add an explicit type alias in
this package, so we can at least add some tests to cover additional
functionality and expectations that the package should have when
interplaying with `oapi-codegen`.

Co-authored-by: Sebastien Guilloux <sebastien.guilloux@elastic.co>
Co-authored-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
jamietanna added a commit to sonasingh46/runtime that referenced this issue Jan 9, 2024
We have spun out a separate package, `oapi-codegen/nullable` as a step
towards oapi-codegen/oapi-codegen#1039.

Until we have implemented oapi-codegen#27, we cannot add an explicit type alias in
this package, so we can at least add some tests to cover additional
functionality and expectations that the package should have when
interplaying with `oapi-codegen`.

Co-authored-by: Sebastien Guilloux <sebastien.guilloux@elastic.co>
Co-authored-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
jamietanna pushed a commit to sonasingh46/oapi-codegen that referenced this issue Jan 11, 2024
As part of oapi-codegen#1039, we've created a new library `oapi-codegen/nullable`,
which allows tracking whether:

- a field is not sent
- a field is sent with an explicit `null`
- a field is sent with an explicit value

This introduces an opt-in `output-options` flag, `nullable-type`, which
can generate the `nullable.Nullable` types.

This is opt-in, as existing code will break due to the signature change,
as well as a behaviour change.

Closes oapi-codegen#1039.

Co-authored-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
jamietanna pushed a commit to sonasingh46/oapi-codegen that referenced this issue Jan 11, 2024
As part of oapi-codegen#1039, we've created a new library `oapi-codegen/nullable`,
which allows tracking whether:

- a field is not sent
- a field is sent with an explicit `null`
- a field is sent with an explicit value

This introduces an opt-in `output-options` flag, `nullable-type`, which
can generate the `nullable.Nullable` types.

This is opt-in, as existing code will break due to the signature change,
as well as a behaviour change.

Closes oapi-codegen#1039.

Co-authored-by: Ashutosh Kumar <ashutosh.kumar@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants