Optional Fields: Should NewOptType(v) take a pointer instead?

[germainc]

Description

Would it make more sense to instantiate OptNilDateTime (or OptDateTime or any other optional type) by passing in a pointer?

func NewOptNilDateTime(v *time.Time) OptNilDateTime {
	return OptNilDateTime{
		Value: *cmp.Or(v, &time.Time{}),
		Set:   true,
		Null:  v == nil,
	}
}

You would still have the flexibility to write more complex logic when you need it but it would simplify most uses cases.

// System document
type DocumentResponse struct {
	UserID      string
	GeneratedAt time.Time
	SignedAt    *time.Time
	ReviewedAt  *time.Time
	ExpiresAt   *time.Time
}

...

// API document response
type DocumentResponse struct {
	UserID      string         `json:"user_id"`
	GeneratedAt time.Time      `json:"generated_at"`
	SignedAt    OptNilDateTime `json:"signed_at"`
	ReviewedAt  OptNilDateTime `json:"reviewed_at"`
	ExpiresAt   OptNilDateTime `json:"expires_at"`
}

...

// Handler
func (h *Handler) GetDocument(ctx context.Context, params api.GetDocParams) (*api.DocumentResponse, error) {
	doc := h.RetrieveDoc(ctx, params.DocID)

	return &api.DocumentResponse{
		UserID:      doc.UserID,
		GeneratedAt: doc.GeneratedAt,
		SignedAt:    api.NewOptNilDateTime(doc.SignedAt),
		ReviewedAt:  api.NewOptNilDateTime(doc.ReviewedAt),
		ExpiresAt:   api.NewOptNilDateTime(doc.ExpiresAt),
	}, nil
}

...

// Output
{
  "user_id": "2bb516da-db63-4606-85be-d531b6badff5",
  "generated_at": "2024-07-02T22:25:52Z",
  "signed_at": "2024-07-02T23:12:19Z",
  "reviewed_at": null,
  "expires_at": null
}

[wildwind123]

I use this approach, for nil values.

func TestNilToVal(t *testing.T) {

	var boolean *bool
	resBool := ogencl.NewOptBool(NilToVal(boolean))
	fmt.Println(resBool)
	var str *string
	resString := ogencl.NewOptString(NilToVal(str))
	fmt.Println(resString)

}

func NilToVal[T any](v *T) T {
	if v == nil {
		v = new(T)
	}
	return *v
}

[germainc]

@wildwind123 That approach makes the assumption that nil and the zero value of a type carry the same meaning in your system.

If you do that with a *time.Time it will render out as:

"generated_at": "0001-01-01T00:00:00Z"

which does not have the same meaning as:

"generated_at": null

[rl-pavel]

Just wanted to bump this up as I have also just ran into a similar issue with NewWhatever methods causing exceptions (in tests, thankfully) because the code-generated methods assume Set: true.

[kushuh]

I don't think it makes sense that NewNil... takes a direct value. I'd use Nilish types, because I expect the underlying data to be a pointer in the actual logic (service layer, or whatever abstraction you may use).

It makes things boilerplate, and error prone if you don't understand well how the ogen schemas work:

// This is returned from another abstracted layer, that doesn't know anything about http/openapi
servideModel := myService.ActionThatReturnsModel(...)

modelField := OptNilModelField{} // Type generated by ogen
// The field is a pointer in the service, and may be nil.
if servideModel.Field == nil {
  modelField.SetToNull()
} else {
  modelField.SetTo(*servideModel.Field)
}

return &Model{
  Field: modelField,
}, nil

Proposal / Idea

I think the code of NewNil... could be easily replaced (or improved with a new method, to preserve backwards compatibility)

// Currently generated by ogen.
//
// func NewOptNilModelField(v Model) OptNilModelField {
//   return OptNilModelField{
//     Value: v,
//     Set:   true,
//   }
// }

// Proposed replacement
func NewOptNilModelField(v *Model) OptNilModelField {
  if v == nil {
    return OptNilModelField{
      Set: true,
      Null: true,
    }
  }

  return OptNilModelField{
    Value: *v,
    Set: true,
  }
}

Result

The above example could be refactored way easier:

// This is returned from another abstracted layer, that doesn't know anything about http/openapi
servideModel := myService.ActionThatReturnsModel(...)

return &Model{
  // Since the NewOptNil method takes a pointer, it automatically takes care of setting the value
  // depending if Field is nil. No more errors, and the api feels more intuitive.
  Field: NewOptNilModelField(servideModel.Field),
}, nil