ogen icon indicating copy to clipboard operation
ogen copied to clipboard

JSON Marshal error on generated empty `OptString` struct

Open exu opened this issue 3 years ago • 11 comments

What version of ogen are you using?

v0.54.1

Can this issue be reproduced with the latest version?

Yes just try to run this test in directory where API is generated with OptString

package api

import (
	"testing"

	"encoding/json"
)

func TestOpString(t *testing.T) {

	s := OptString{}

	_, err := json.Marshal(s)

	if err != nil {
		t.Fatalf("error should be nil, got %v", err)
	}

}

So for most use cases with fields set to not required when trying to marshal it (e.g. send through event bus) it's not possible to use ogen.

Getting output:


--- FAIL: TestOpString (0.00s)
    /Users/exu/code/kube/testkube-cloud-api/pkg/api/optstring_test.go:16: 
   error should be nil, got json: error calling MarshalJSON for type api.OptString: 
   unexpected end of JSON input

exu avatar Nov 07 '22 16:11 exu

Encoding and decoding with encoding/json is not supported for OptString because following limitations: https://github.com/golang/go/issues/11939

So it is impossible to represent OptString with encoding/json (pointers are used instead), that's why we are using go-faster/jx and custom code-generated json marshaling and unmarshaling.

ernado avatar Nov 07 '22 16:11 ernado

@ernado thanks for the quick response, so how should I deal with it, can I omit OptString and use pointers as most other generators do? Or any other ideas on how to workaround this without changing the generator? (we have quite big API definition)

exu avatar Nov 08 '22 07:11 exu

Looks like just handling zeroed value works

Would it be a case to just do PR to the generator for Opt* structs?

// MarshalJSON implements stdjson.Marshaler.
func (s OptString) MarshalJSON() ([]byte, error) {
	if !s.Set {
		return []byte(`{}`), nil
	}
	e := jx.Encoder{}
	s.Encode(&e)
	return e.Bytes(), nil
}


Just is there a way to check if Type is optional in template?

exu avatar Nov 08 '22 08:11 exu

What are you trying to do?

Using MarshalJSON on requests, responses or most generated types should work.

Returning {} is invalid, probably we should not generate MarshalJSON for OptString at all, it is impossible to express omitempty struct with encoding/json.

ernado avatar Nov 08 '22 09:11 ernado

I try to send generated struct through external library (NATS) and don't have control over marshaller. Additional info: Fields are not initialized.

exu avatar Nov 08 '22 09:11 exu

It's more about generating valid JSON - as looks like currently, we're ending with something like field: when the struct is uninitialized. It would be ok for me to just store pass it (without handling omitempty) I just need valid JSON and currently it's not.

exu avatar Nov 08 '22 09:11 exu

I see there several ways to fix it:

  1. Do not generate MarshalJSON/UnmarshalJSON for the Opt/NilOpt types.

    • Likely breaks existing code.
    • These types would be marshaled/unmarshaled by encoding/json, as JSON objects.
  2. Return []byte("null").

  3. ~~Return an error in case if value is not set, add omitempty tag to optional fields.~~

    • ~~Forces to add omitempty tag. Some people may use generated types in their own structs.~~
    • ~~Would not work properly in some cases. For example: Set is false, but value is not zero.~~

ghost avatar Nov 08 '22 23:11 ghost

@tdakkota omitempty does not work for structures, even if they are zero value.

ernado avatar Nov 09 '22 12:11 ernado

The only valid solution is (1). Let's remove MarshalJSON/UnmarshalJSON for Opt* types, it should be only for full components (e.g. requests, responses).

ernado avatar Nov 09 '22 12:11 ernado

Maybe just make no 1 optional? To not break possible existing implementations

exu avatar Nov 09 '22 14:11 exu

Hi,

for objects workaround with overhead

...
// use generated method MarshalJSON.
tempBts, err := s.MarshalJSON()
if err != nil {
    return fmt.Errorf("marshal: %w", err)
}

var m map[string]interface{}

// unmarshal to map[string]interface{}
if er := json.Unmarshal(tempBts, &m); er != nil {
    return fmt.Errorf("umarshal to map: %w", er)
}

// use  std MarshalIndent for get ident json.
bts, err := json.MarshalIndent(m, "", "    ")
if err != nil {
    return fmt.Errorf("marshal: %w", err)
}

P.S. If all of the Opt fields filled, error not happen! P.P.S. ogen --version // ogen version v0.72.1 (built with go1.21.0) darwin/amd64

art-frela avatar Aug 09 '23 18:08 art-frela

Just checking, is this still being worked on?

@tdakkota omitempty does not work for structures, even if they are zero value.

But they did recently add omitzero in https://github.com/golang/go/issues/45669

The only valid solution is (1). Let's remove MarshalJSON/UnmarshalJSON for Opt* types

Wouldn't this alter data if the structs do have values?

Is this all because of avoiding pointers?

fiendish avatar Dec 02 '24 22:12 fiendish

Yes, we avoid pointers for better semantics and removing NPE problem.

Will use omitzero when released tho. Finally.

ernado avatar Dec 03 '24 06:12 ernado

We've got Go 1.24 and omitzero can we look at having this patched.

koblas avatar Feb 25 '25 11:02 koblas

Any chance this is being looked at? Otherwise I'd be happy to try help out on #1427 if that's the way to go?

We've had production bug recently due to incorrect usage of NewOpt[Type](...):

// nils.Unwrap(...) returns the zero value of the passed in pointer type for nil values
- out.Details = ogen.NewOptOrderDetails(nils.Unwrap(orderDetails))
+ if orderDetails != nil {
+ 	out.Details = ogen.NewOptOrderDetails(*orderDetails)
+ }

The old code would end up returning something like this (prettified) invalid json:

{
  "id": "9ad94a36-e350-464a-b46c-3e2577dd1193",
  "status": "APPROVED",
  "customerId": "26b6c9e0-454d-4146-a8bd-469121ec0382",
  "details":
  "createdAt": "2025-05-06T07:44:00Z",
  "updatedAt": "2025-05-06T07:44:00Z",
  "fulfilledAt": "2025-05-06T07:44:00Z"
}

While the error definitely lies on our side and was an easy fix, it feels like an easy trap to fall in to by accident. In our case it was early on misunderstanding of what the NewOpt[Type] actually did that was found only after the stars aligned in production...

kristofferremback avatar May 06 '25 10:05 kristofferremback

The problem occurs not only in Marshal but also in Unmarshal.

nonchan7720 avatar May 27 '25 09:05 nonchan7720