Webhook openAPI spec and code disagrees on property casing

[roosmaa]

What happened:

In the api/webhook.yaml openAPI spec, the changes component lists the properties as starting with lower-case.

external-dns/api/webhook.yaml

Lines 250 to 257 in 58ac76e

	create:
	$ref: '#/components/schemas/endpoints'
	updateOld:
	$ref: '#/components/schemas/endpoints'
	updateNew:
	$ref: '#/components/schemas/endpoints'
	delete:
	$ref: '#/components/schemas/endpoints'

However, the Changes struct in Go side doesn't have the JSON tag giving it an eplicit name, so Go will default to the field names (Create, UpdateOld, UpdateNew, Delete) in the generated JSON.

external-dns/plan/plan.go

Lines 55 to 64 in 58ac76e

	type Changes struct {
	// Records that need to be created
	Create []*endpoint.Endpoint
	// Records that need to be updated (current data)
	UpdateOld []*endpoint.Endpoint
	// Records that need to be updated (desired data)
	UpdateNew []*endpoint.Endpoint
	// Records that need to be deleted
	Delete []*endpoint.Endpoint
	}

What you expected to happen:

I would expect the openAPI spec and code to agree. Even though Go doesn't care about casing, other platforms do.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

• External-DNS version (use external-dns --version): v0.15.1
• DNS provider: webhook
• Others:

[ivankatliarchuk]

The API is yaml and not a JSON ;-).

The api/webhook.yaml file is manually generated (see blame). Ideally, automating its generation would have addressed this issue.

[roosmaa]

@ivankatliarchuk The API is yaml and not a JSON ;-).

Not sure what you mean. external-dns sends JSON to the webhook provider APIs. Not Yaml. The openAPI spec may be YAML, but end of the day we're dealing with JSON objects.

All of the other JSON objects sent to the webhook provider follow lowerCamelCase convention for property names. So, I would reason the idea is to use lowerCamelCase and the fact that Changes struct doesn't have the proper lowerCamelCase name overrides in the Go json tags is not how it should be.

[ivankatliarchuk]

I'm not familiar with the webhook api. You pointed to a manually generate yaml file.....
But now I understand it better.

This is the method where Encoding and Decoding is happening

external-dns/provider/webhook/api/httpapi.go

Line 42 in be7f970

func (p *WebhookServer) RecordsHandler(w http.ResponseWriter, req *http.Request) {

While is nice to have json tags on objects, they are not required for Go....

Proof how it works, you could have any case when tags not set, is actually more flexible, thereby promoting language-agnostic compatibility. Open API speck expects that implementations can handle different casing variations of the same logical property name.
https://go.dev/play/p/Nc9aYwqHNg4

Please explain why tagging on that Structs is needed, not sure why it's a bug. OpenAPI specifies that schema element names are case insensitive for schema element names (within the same scope). The OpenAPI spec focuses on the structure and semantics of the API, rather than strict naming conventions for element names. So you could have any case you like, to provide out-of-the box compatibility with other protocols or programming languages out-of-the-box conventions.

crEate: 
   $ref: '#/components/schemas/endpoints' 
 updateold: 
   $ref: '#/components/schemas/endpoints' 
 updaTenew: 
   $ref: '#/components/schemas/endpoints' 
 DELETE: 
   $ref: '#/components/schemas/endpoints' 

I prefer camelCase as well. There probably no harm to add json tags, but not too sure, this may break some compatibility.

[roosmaa]

@ivankatliarchuk Open API speck expects that implementations can handle different casing variations of the same logical property name.

Actually no, that's just one of Go idiosyncrasies, where it tries to be helpful and unmarshal things case-insensitively. OpenAPI spec, JavaScript, and many other environments that deal with JSON are case-sensitive by default.

For example, if you run the following in your browser devconsole (or in a playground):

let data = JSON.parse('{"CreAtE": "foo"}');
console.log('Parsed data', data);
console.log('Value of create', data.create);
console.log('Value of CreAtE', data.CreAtE);

You can see that the properties are case-sensitive.

Why I stumbled upon this was because I was implementing a webhook in Rust and it caused some headbanging.

From my (the users) point of view, one of the two places should be fixed:

1. The Go struct to unify the JSON property naming with other property naming getting POSTed into the webhook APIs
2. The OpenAPI spec, clearly showing that the casing is different for that endpoint.

Since I'm not the maintainer of this project, I'm in no position to decide which one is correct. As you pointed out there may be some compatibility issues; however, if all the existing webhooks are Go based it won't affect them.

[ivankatliarchuk]

Exactly same behaviour in Node

const input = `
{
  "create": [
    {
      "Name": "example.com"
    },
    {
      "Name": "example.org"
    }
  ],
  "updateold": [
    {
      "Name": "old.example.com"
    },
    {
      "naMe": "paradox.example.com"
    }
  ],
  "updatenew": [
    {
      "Name": "new.example.com"
    }
  ],
  "Delete": [
    {
      "Name": "delete.example.com"
    }
  ]
}
`;

class Endpoint {
  constructor(name) {
    this.Name = name;
  }
}

class Changes {
  constructor() {
    this.Create = [];
    this.UpdateOld = [];
    this.UpdateNew = [];
    this.Delete = [];
  }
}

async function main() {
  console.log("Hello, playground");

  const changes = new Changes();
  changes.Create.push(new Endpoint("example.com"));
  changes.Create.push(new Endpoint("example.org"));
  changes.UpdateOld.push(new Endpoint("old.example.com"));
  changes.UpdateNew.push(new Endpoint("new.example.com"));
  changes.Delete.push(new Endpoint("delete.example.com"));


  // Print the changes object to verify
  const data = JSON.stringify(changes, null, "  ");
  console.log(data);

  const ch = new Changes();
  try {
    const parsedInput = JSON.parse(input);

    // Populate the ch object from the parsed JSON.  Handle potential case differences in keys.
    ch.Create = (parsedInput.create || []).map(item => new Endpoint(item.Name));
    ch.UpdateOld = (parsedInput.updateold || []).map(item => new Endpoint(item.Name || item.naMe)); // Handle "naMe"
    ch.UpdateNew = (parsedInput.updatenew || []).map(item => new Endpoint(item.Name));
    ch.Delete = (parsedInput.Delete || []).map(item => new Endpoint(item.Name));

    console.log("DECODED");
    console.log("Create:");
    ch.Create.forEach(e => console.log(e.Name));
    console.log("UpdateOld:");
    ch.UpdateOld.forEach(e => console.log(e.Name));
    console.log("Delete:");
    ch.Delete.forEach(e => console.log(e.Name));

  } catch (error) {
    console.error("Error parsing JSON:", error);
  }
}

main()

You could try here https://playcode.io/new

I'm more then sure, .Net behaves exactly the same, even by convention expected fist letter to be capitilise.

From the specification itself, the response content itself does not have a strict case-sensitivity rule. This feels like is outside of Open API spec, so if we discussing JSON, is most likely one of the JSON specification (RFC XXXX).

I'm easy where or not tags a present on Go. And I agree

• The OpenAPI schema definitions are case-sensitive. If project schema defines userName, the API response must match it exactly, but could support other cases.
• If API response is validated against an OpenAPI schema, case differences in field names may cause validation failures.
• If API consumers expect specific field names, we should be consistent in API definition.

I'll add a label, feel free to open a pull request.

/help

[k8s-ci-robot]

@ivankatliarchuk:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Exactly same behaviour in Node

const input = `
{
 "create": [
   {
     "Name": "example.com"
   },
   {
     "Name": "example.org"
   }
 ],
 "updateold": [
   {
     "Name": "old.example.com"
   },
   {
     "naMe": "paradox.example.com"
   }
 ],
 "updatenew": [
   {
     "Name": "new.example.com"
   }
 ],
 "Delete": [
   {
     "Name": "delete.example.com"
   }
 ]
}
`;

class Endpoint {
 constructor(name) {
   this.Name = name;
 }
}

class Changes {
 constructor() {
   this.Create = [];
   this.UpdateOld = [];
   this.UpdateNew = [];
   this.Delete = [];
 }
}

async function main() {
 console.log("Hello, playground");

 const changes = new Changes();
 changes.Create.push(new Endpoint("example.com"));
 changes.Create.push(new Endpoint("example.org"));
 changes.UpdateOld.push(new Endpoint("old.example.com"));
 changes.UpdateNew.push(new Endpoint("new.example.com"));
 changes.Delete.push(new Endpoint("delete.example.com"));


 // Print the changes object to verify
 const data = JSON.stringify(changes, null, "  ");
 console.log(data);

 const ch = new Changes();
 try {
   const parsedInput = JSON.parse(input);

   // Populate the ch object from the parsed JSON.  Handle potential case differences in keys.
   ch.Create = (parsedInput.create || []).map(item => new Endpoint(item.Name));
   ch.UpdateOld = (parsedInput.updateold || []).map(item => new Endpoint(item.Name || item.naMe)); // Handle "naMe"
   ch.UpdateNew = (parsedInput.updatenew || []).map(item => new Endpoint(item.Name));
   ch.Delete = (parsedInput.Delete || []).map(item => new Endpoint(item.Name));

   console.log("DECODED");
   console.log("Create:");
   ch.Create.forEach(e => console.log(e.Name));
   console.log("UpdateOld:");
   ch.UpdateOld.forEach(e => console.log(e.Name));
   console.log("Delete:");
   ch.Delete.forEach(e => console.log(e.Name));

 } catch (error) {
   console.error("Error parsing JSON:", error);
 }
}

main()

You could try here https://playcode.io/new

I'm more then sure, .Net behaves exactly the same, even by convention expected fist letter to be capitilise.

From the specification itself, the response content itself does not have a strict case-sensitivity rule. I actually this it's outside of Open API spec, so if we discussing JSON, is most likely one of the JSON specification (RFC XXXX).

I'm easy where or not tags a present on Go. And I agree

• The OpenAPI schema definitions are case-sensitive. If project schema defines userName, the API response must match it exactly, but could support other cases.
• If API response is validated against an OpenAPI schema, case differences in field names may cause validation failures.
• If API consumers expect specific field names, we should be consistent in API definition.

I'll add a label, feel free to open a pull request.

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.