[BUG] [Go] Discriminator unmarshal error & why DisallowUnknownFields() should be removed

[dahu33]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• Have you tested with the latest master to confirm the issue still exists?
• Have you searched for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

The change introduced in #17267 added a call to DisallowUnknownFields() on the Go JSON decoder to reject unknown fields. This unintentionally broke JSON unmarshalling in generated code that uses Go struct embedding.

Why? When using struct embedding, the embedded struct's UnmarshalJSON(data []byte) error method receives the full JSON object, including fields from its parent. Due to DisallowUnknownFields(), any unknown parent fields will cause unmarshalling to fail.

In the case of the Go generator, I found that the issue occurs when using discriminators. See the steps to reproduce below.

I'm very surprised that this critical bug has remained unnoticed for over a year.

openapi-generator version

v7.12.0

Steps to reproduce

Use the spec file:
modules/openapi-generator/src/test/resources/3_0/go/allof_multiple_ref_and_discriminator.yaml

and the corresponding generated code:
samples/client/others/go/allof_multiple_ref_and_discriminator.

To reproduce the issue:

1. Add a new test file issue_test.go in samples/client/others/go/allof_multiple_ref_and_discriminator with the following content:

package openapi

import (
	"encoding/json"
	"fmt"
	"testing"
)

func TestUnmarshalFinalItem(t *testing.T) {
	jsonStr := `{
		"prop1": "example",
		"quantity": 10,
		"unitPrice": 5.5,
		"totalPrice": 55.0,
		"title": "example",
		"type": "FINAL"
	}`

	var item FinalItem
	err := json.Unmarshal([]byte(jsonStr), &item)
	if err != nil {
		t.Fatalf("Unmarshal error: %v", err)
	}
	fmt.Printf("Unmarshalled FinalItem: %+v\n", item)
}

2. Run the tests with:

go test

You will observe the following error:

--- FAIL: TestUnmarshalFinalItem (0.00s)
    issue_test.go:22: Unmarshal error: json: unknown field "prop1"

The error is caused by the DisallowUnknownFields() inside the embedded struct UnmarshalJSON(data []byte) (err error) function:
https://github.com/curvegrid/openapi-generator/blob/afc27ef4bc969f07613807aba51ec1a9f46cf605/samples/client/others/go/allof_multiple_ref_and_discriminator/model_base_item.go#L138

Suggest a fix

It is not because additionalProperties is set to false in the OpenAPI spec that a generated client should completely fail if the server returns extra fields. In real-world APIs, it is common for servers to evolve and add new properties over time.

The use of DisallowUnknownFields() in generated client code is extremely dangerous because:

• It prevents clients from tolerating new fields added to the server response, making clients extremely fragile.
• It breaks backward compatibility: older clients interacting with newer servers will crash when they encounter unexpected fields.
• It fundamentally contradicts robust client design principles, where clients should ignore unknown fields rather than fail.

I would propose to remove DisallowUnknownFields() from the Go client templates as soon as possible to restore proper resilience and compatibility when dealing with evolving APIs (and embedded structs).

[ctreatma]

There is a generateUnmarshalJSON option for the Go generator that will enable you to bypass the additionalProperties validation. If you run the generator with generateUnmarshalJSON=false it will skip generating the custom, spec-compliant UnmarshalJSON method. It looks like there's also support for an x-go-generate-marshal-json vendor extension so that the method generation can be skipped on a more granular basis, although that requires making changes to the input spec.

One thing to note is that openapi-generator does not generate fully spec-compliant code by default. In the example you linked, there is no additionalProperties on the FinalItem schema in the input spec. Per OpenAPI standards, that is supposed to mean that the FinalItem model can have additional properties, but in order to get openapi-generator to produce a FinalItem model that allows additional properties, you have to run it with disallowAdditionalPropertiesIfNotPresent=false.

Ref: https://openapi-generator.tech/docs/generators/go#config-options

[dahu33]

@ctreatma Thank you very much for your prompt reply! While setting generateUnmarshalJSON=false will help until the issue is resolved, I still think it’s quite extreme to disallow extra fields by default on the client as it creates more problems than it solves. So far, I haven’t seen any other generator that enforces such strictness on the client side.

[ctreatma]

@dahu33 if you are running the generator with disallowAdditionalPropertiesIfNotPresent=false, then the model will only block additional properties for schemas/specs that explicitly say additionalProperties: false. In that case, if the API cannot guarantee there will be no additional fields, the additionalProperties setting should be removed from the spec (it defaults to true according to OAS3 rules).

If you aren't setting disallowAdditionalPropertiesIfNotPresent, it defaults to true and that means that this generator does the opposite of what OAS3 rules say: it assumes that additionalProperties is false by default. If your spec does not explicitly say additionalProperties: false, then running the generator with disallowAdditionalPropertiesIfNotPresent=false should address the problem and you won't need to use generateUnmarshalJSON=false.

EDIT: You mentioned struct embedding, so perhaps the issue you're running into is that something that uses your generated client code is attempting to extend a model that has additionalProperties: false. Struct embedding isn't really in the scope of OpenAPI standards, and issues with inherited embedded struct methods aren't specific to this generator. Those sorts of issues can be addressed directly through type aliasing, as demonstrated here: https://go.dev/play/p/RLxp7aKijly

[dahu33]

I’m well aware of the disallowAdditionalPropertiesIfNotPresent setting. My main point is that there’s a difference between not expecting additional properties and failing hard when they are present. In my opinion, disallowing extra fields by default is a bit extreme.

Regarding struct embedding, it’s the generator that creates them, not me. Please refer to the detailed bug report above, it contains all the necessary information, including the exact steps to reproduce the original issue (which is separate from my opinion on whether disallowing extra fields by default is a good idea).

[ctreatma]

What value are you using for disallowAdditionalPropertiesIfNotPresent? You have to set it to false if you want to generate code that matches the input spec and aligns with OAS standards. If one were to regenerate the example code you pointed to in your initial issue using disallowAdditionalPropertiesIfNotPresent=false, the new code would allow arbitrary properties. Setting disallowAdditionalPropertiesIfNotPresent=true (or not specifying that flag at all) is explicitly telling the generator "block additional properties by default," which is exactly what the generated code is doing.

[dahu33]

Let’s focus strictly on the bug and set aside our disagreement regarding client behavior when additional fields are present.

The bug described above occurs only when disallowAdditionalPropertiesIfNotPresent=true is used, or when additionalProperties: false is explicitly set on the discriminator object. In those cases, the generator produces code that compiles but results in a runtime error when unmarshalling data that is 100% compliant.

[ctreatma]

The client code behavior is entirely driven by your spec and your config options.

If your generator config options say "generate models that reject additional fields," which is what you are telling the generator by passing in disallowAdditionalPropertiesIfNotPresent=true, and then the generator produces models that reject additional fields, the generator is doing exactly what you told it to do. I don't see how that can be considered a bug.

If your spec says "this schema will never have additional fields," then additional fields are invalid and should be handled as such; if your spec has an allOf that includes a closed schema, that is not valid and should be fixed in the spec.

You've adopted the generateMarshalJson=false and generateUnmarshalJson=false workaround and it has unblocked you for now. If, instead of adopting those, you switched to disallowAdditionalPropertiesIfNotPresent=false, you would produce code that matches your spec and have overall a better experience and an easier time determining what the code will look like based on the input spec.