[v1.12.4 -> v2.3.0] - Order of arguments for runtime.JsonMerge and runtime.JSONMerge are being flipped.

[jamesdillon-pol]

Hi there.

I'm working on a project where the team currently use the [email protected] command line tool, and we were looking into upgrading to the newest version v2.3.0.

The issue I'm having is that when any small change is made to the existing openapi yaml spec and we run the following command with [email protected]:

oapi-codegen --config api/config.yaml ./api/openapi.yaml> ./api/v1/go/ServerConfig.gen.go

It generates hundreds of changes - most of them are benign but some are breaking changes.

The change that is causing issues is to do with converting any instance of the deprecated runtime.JsonMerge to the preferred runtime.JSONMerge. This should be a straightforward drop-in replacement, but for some reason the order of the arguments being supplied is flipped like so:

This change breaks some of our tests.

However, after I do a find and replace in ServerConfig.gen.go to change runtime.JSONMerge(t.union, b) to runtime.JSONMerge(b, t.union) the tests all pass. Obviously modifying the generated file isn't a solution, but it seems to pinpoint the fact that the issue is oapi-codegen reversing the order of the arguments to runtime.JSONMerge.

This is quite surprising to me, as the result of merging two JSON objects shouldn't depend on the order of the arguments.

I'm still actively looking into this, and when I've a bit more time available I'll supply steps to replicate from scratch.

Thanks loads!

[davidsteed1]

The order does matter as the first parameters type is used. The problem is arising when b is an array and t.union is empty. In the old code merged is an array containing b in the second merged is {} as an object cannot be merged with an array and nil is assumed to be an object not an array. Swapping the parameter order back to what it was would fix this

[davidsteed1]

I believe a change to line 56 of pkg/codegen/templates/union.tmpl merged, err := runtime.JSONMerge(b, t.union) will fix the issue

[jamietanna]

This was an intended change as part of #998 - are you seeing any issues related to this, or was it just surprising?

[davidsteed1]

Yes it does not work. The order needs to be changed back

[jamietanna]

Can you please share some examples of the difference in behaviour you're seeing and what issues it's causing?

This will help us work out the impact and whether this needs to be a default change, or if it can be made a "compatibility option" that can be opted in for folks who are affected by this

(as we've not heard of this causing issues until now)

Thanks!

[jamesdillon-pol]

Hey Jamie,

I'll throw together a minimal reproduction example shortly :)

Thanks loads,

[jamesdillon-pol]

Hi there.

Background

I've threw together a bit of a walkthrough that hopefully explains the situation better. Attached is a zip of all files used. oapi-issue-minimal.zip

config.yaml

package: Openapi
generate:
  models: true
  echo-server: true
  embedded-spec: true

openapi.yaml

openapi: "3.0.0"
info:
  version: 1.0.0
  title: Minimal ping API server
paths:
  /ping:
    get:
      responses:
        "200":
          $ref: "#/components/responses/PingResponse"
components:
  schemas:
    PingResponseOne:
      type: object
      properties:
        message:
          type: string
    PingResponseTwo:
      type: object
      properties:
        error:
          type: string
  responses:
    PingResponse:
      description: Example ping union response
      content:
        application/json:
          schema:
            oneOf:
              - $ref: "#/components/schemas/PingResponseOne"
              - $ref: "#/components/schemas/PingResponseTwo"

Using the above config files and [email protected], I run the following command:

oapi-codegen --config config.yaml  openapi.yaml > ./api/Output-1-12-4.gen.go

Similarly, using [email protected]` I run the same command and output it to a different file.

oapi-codegen --config config.yaml  openapi.yaml > ./api/Output-2-4-0.gen.go

If you look at line 57 in both of these output files, you can see that between oapi-codegen versions the function used to merge JSON has changed along with the order that the arguments are supplied.

Output-1-12-4.gen.go

57	merged, err := runtime.JsonMerge(b, t.union)

Output-2-4-0.gen.go

57	merged, err := runtime.JSONMerge(t.union, b)

The issue

The problem this causes (as pointed out by @davidsteed1) is that the order of the arguments is significant, specifically around how it handles nil and empty values. The test function below should illustrate this a bit better.

package main

import (
    "encoding/json"
    "reflect"
    "testing"

    "github.com/oapi-codegen/runtime"
)

func TestJSONMerge(t *testing.T) {
    /*
	    oapi-codegen v1.12.4 generates JSON merges using the deprecated JsonMerge function.
	    The parameters are supplied as `runtime.JsonMerge(b, t.union)` (see Output-1-12-4.gen.go l:57)
    */
    old, _ := runtime.JsonMerge(nil, []byte(`[]`))

    /*
	    oapi-codegen v2.4.0 generates JSON merges using the newer JSONMerge function.
	    The parameters are supplied as `runtime.JSONMerge(t.union, b)` (see Output-2-4-0.gen.go l:57)

	    This test fails, showing that the order of arguments is significant. This is what 
            causes some tests to break in our codebase.
    */
    new, _ := runtime.JSONMerge([]byte(`[]`), nil)

    var oldJson, newJson interface{}

    err1 := json.Unmarshal(old, &oldJson)
    err2 := json.Unmarshal(new, &newJson)

    if err1 != nil || err2 != nil {
	    t.Fatal("Failed to unmarshal JSON data")
    }

    if !reflect.DeepEqual(oldJson, newJson) {
	    t.Errorf("%v and %v are not equal", oldJson, newJson)
    }
}

This produces the error:

--- FAIL: TestJSONMerge (0.00s)
    /Users/username/Documents/oapi-issue-minimal/Example_test.go:37: map[] and [] are not equal
FAIL
FAIL	example	0.290s
FAIL

Hopefully this highlights a little more why changing to the new JSONMerge function, and reversing the arguments causes our test suite to fail.