[BUG] [Go-Server] Required validation too strict

[Fatcat560]

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

Hello! I'm currently trying to use the go-server generator, however I am running into an issue with the required field validation.
If a schema is present which has an integer field marked as required, it will not pass validation if the field is present but has a value of 0.

This causes a problem when a value of 0 is considered to be a valid input for schemas, as the generated code will reject such values.

openapi-generator version

Tested both with 7.10.0 and 7.11.0-SNAPSHOT.

OpenAPI declaration file content or url

openapi: 3.0.3
info:
  title: Test - OpenAPI 3.0
  version: 1.0.0
servers:
  - url: https://localhost:8080/api/v1
paths:
  /config:
    put:
      summary: Update something
      operationId: update
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/TestObject'
        required: true
      responses:
        '204':
          description: Successful operation
components:
  schemas:
   TestObject:
    type: object
    properties:
      n:
        type: integer
        minimum: 0
    required:
      - n

Generation Details

Steps to reproduce

• Get the latest release (in my case version 7.10.0)
• Run java -jar openapi-generator-cli.jar generate -g go-server -i openapi.yaml -o out --additional-properties=router=chi
• cd ./out
• go mod tidy && go mod run ./main.go

Expected results:
Sending a HTTP PUT request to http://localhost:8080/api/v1/config with a body of {"n" : 1} or a body of {"n" : 0} both cause a 501 Not Implemented response

Actual results:
Sending the request with n=1 results in the expected 501 status code, however the n=0 case causes a 422 Unprocessable Entity. The error message is: "required field 'n' is zero value."

Related issues/PRs

Suggest a fix

I believe the issue is in the generated code for the model:

// In model_test_object.go

// AssertTestObjectRequired checks if the required fields are not zero-ed
func AssertTestObjectRequired(obj TestObject) error {
	elements := map[string]interface{}{
		"n": obj.N,
	}
	for name, el := range elements {
		if isZero := IsZeroValue(el); isZero {
			return &RequiredError{Field: name}
		}
	}

	return nil
}

//In helper.go

// IsZeroValue checks if the val is the zero-ed value.
func IsZeroValue(val interface{}) bool {
	return val == nil || reflect.DeepEqual(val, reflect.Zero(reflect.TypeOf(val)).Interface())
}

I think the required validator should not check if the value is some zero of its type, but only check the existence of the key in the property.

As a workaround, the AssertTestObjectRequired function can be changed to only return nil, however this change must then be made "permanent" by adding the model to the ignore list, otherwise it will get overwritten again.

If someone knows the specifics on how to approach this issue I'd be happy to try and help :)

[vingarzan]

I think I'm hitting a very closely related issue to this, which probably exposes the same underlying problem of the go-server generated code. But let's try to generalize.

Your issue

I don't think it's a constraints problem, but a required one. I'm guessing that even if you remove the minimum it will fail just the same.

My issue

In my case I have some properties which are not really required. But they are also not-nullable. Hence they are not pointers in the generated data model structures.

When parsing the JSON, I think the unmarshaler doesn't really leave any bread-crumbs or something like that behind, to tell if a property wasn't present. Hence both the Assert{{baseType}}Required({{paramName}}Param) and the Assert{{baseType}}Constraints({{paramName}}Param) functions are both called and go crazy on empty attributes and sub-structures. My guess is that this is the same as in your case, since those are zero-initialized.

My structures are somewhat complex and... inflexible (because 5G APIs are not done by developers, but by a weird committee 🤮). So I can't change them easily and my only temporary solution was to modify the controller-api.mustache and comment/remove those calls. It's ugly, of course, to remove all these checks, but @Fatcat560 that might work for you too, temporarily, until a proper fix would be in place.

The root cause

The JSON decoder is not saving anywhere some sort of wasPresent metadata for all the properties that it decodes. The Assert{{type}}[Required|Constraints]() function, to function properly, would require for all non-nullable data structures, this metadata. Only if that's present the checks should be done, else the property is zero and shouldn't need checking.

(For nullable properties, of course, that check is simply Something.property != nil. But turning all properties into pointers seems like a bad idea.)

Other generated code (e.g. C++, I guess, which uses cJSON) does the validation during the JSON unmarshaling. But it also generates code for that, I think, while for Go we don't really have to bother (so far). In such a case, it's somewhat solved, as only if a parameter is present it is parsed and immediately checked.

Possible solutions

1. Add to all data model structures some sort of metadata property, which would be a bunch of bool values indicating wasPresent in JSON. Check this before asserting required/constraints. Unfortunately, filling this will require generating UnmarshalJSON() methods for all data models, which is anyway proposed below.
2. Generate UnmarshalJSON() functions and move the checks there, away from the controller-api. Assumes that we'll figure out a way to tell if a property wasPresent.
3. Generate a separate metadata structure somehow from the JSON and use it during checking for that wasPresent indication.
4. Add a circuit breaker in Assert{{type}}[Required|Constraints]() functions to return and skip checks if IsZeroValue(obj) (feels wrong though).

The second seems best to me, but would love the opinion of original authors and/or more seasoned maintainers here, since I just started using this last week 😉. But it might require a 2nd set of data models, where everything is nullable, don't know yet...

@antihax @grokify @kemokemo @jirikuncar @ph4r5h4d @lwj5 @wing328