Use KeyNamerWithOriginalFieldName function in jsonschema config generation

[karimkhaleel]

• PR Description
  This PR switches from using the OriginalPropertiesMapping fork of https://github.com/invopop/jsonschema to using the KeyNamerWithOriginalFieldName reflector field of this fork.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[karimkhaleel]

Discussion related to this: #4419

[stefanhaller]

Makes sense to me. Just to make sure I understand the motivation: we think a KeyNamerWithOriginalFieldName PR would be more likely to be accepted upstream than the OriginalPropertiesMapping PR, is that right?

[stefanhaller]

I find it extremely confusing that the same map is used for both directions. It works for us, but only because we never have something like this in our config:

type UserConfig struct {
	Foo string `yaml:"Bar"`
	Bar string `yaml:"Foo"`

That's a super contrived example, I know, but why not model this properly if we can.

I suggest a patch something like this:

diff --git i/pkg/jsonschema/generate.go w/pkg/jsonschema/generate.go
index ddb230207..54c711edb 100644
--- i/pkg/jsonschema/generate.go
+++ w/pkg/jsonschema/generate.go
@@ -53,9 +53,10 @@ func getSubSchema(rootSchema, parentSchema *jsonschema.Schema, key string) *json
 
 func customReflect(v *config.UserConfig) *jsonschema.Schema {
 	yamlToFieldNames := make(map[string]string)
+	fieldToYamlNames := make(map[string]string)
 	keyNamer := func(yamlName string, originalFieldName string) string {
 		yamlToFieldNames[yamlName] = originalFieldName
-		yamlToFieldNames[originalFieldName] = yamlName
+		fieldToYamlNames[originalFieldName] = yamlName
 		return yamlName
 	}
 
@@ -76,7 +77,7 @@ func customReflect(v *config.UserConfig) *jsonschema.Schema {
 
 		subSchema := getSubSchema(schema, userConfigSchema, yamlName)
 
-		setDefaultVals(schema, subSchema, defaultValue.FieldByName(fieldName).Interface(), yamlToFieldNames)
+		setDefaultVals(schema, subSchema, defaultValue.FieldByName(fieldName).Interface(), fieldToYamlNames)
 	}
 
 	return schema
@@ -92,7 +93,7 @@ func filterOutDevComments(r *jsonschema.Reflector) {
 	}
 }
 
-func setDefaultVals(rootSchema, schema *jsonschema.Schema, defaults any, yamlToFieldNames map[string]string) {
+func setDefaultVals(rootSchema, schema *jsonschema.Schema, defaults any, fieldToYamlNames map[string]string) {
 	t := reflect.TypeOf(defaults)
 	v := reflect.ValueOf(defaults)
 
@@ -123,7 +124,7 @@ func setDefaultVals(rootSchema, schema *jsonschema.Schema, defaults any, yamlToF
 		value := v.Field(i).Interface()
 		parentKey := t.Field(i).Name
 
-		key, ok := yamlToFieldNames[parentKey]
+		key, ok := fieldToYamlNames[parentKey]
 		if !ok {
 			fmt.Println(key)
 			continue
@@ -132,7 +133,7 @@ func setDefaultVals(rootSchema, schema *jsonschema.Schema, defaults any, yamlToF
 		subSchema := getSubSchema(rootSchema, schema, key)
 
 		if isStruct(value) {
-			setDefaultVals(rootSchema, subSchema, value, yamlToFieldNames)
+			setDefaultVals(rootSchema, subSchema, value, fieldToYamlNames)
 		} else if !isZeroValue(value) {
 			subSchema.Default = value
 		}

[karimkhaleel]

I pushed a fixup where I used lo.Invert for this.

[stefanhaller]

Even better, nice.

[karimkhaleel]

We can still run into a bit of a naming issue here if we have something like this:

type ConfigA struct {
    Color string `yaml:"color"`
}
type ConfigB struct {
    Color string `yaml:"shade"`
}

or

type ConfigA struct {
    Shade string `yaml:"color"`
}
type ConfigB struct {
    Color string `yaml:"color"`
}

We could get away with something like this in the OriginalPropertiesMapping approach since we maintained the relationship hierarchy, but since we are storing a flat map here, this relationship is lost.

[karimkhaleel]

Makes sense to me. Just to make sure I understand the motivation: we think a KeyNamerWithOriginalFieldName PR would be more likely to be accepted upstream than the OriginalPropertiesMapping PR, is that right?

Yeah, that's what I'm thinking.

[karimkhaleel]

Do you think I should go ahead and close this old PR and open up an new one for this?

[stefanhaller]

Do you think I should go ahead and close this old PR and open up an new one for this?

Yes, I think this would be a good idea.

[karimkhaleel]

The only qualm I have with this approach is that its still not really a "namer" function. I am not sure how many users would need to see both the field name and the tag name in order to come up with a final name. We are just using it to preserve that mapping of field name to tag name. I wish there was a better way to query this information from the this library, but it doesn't seem like it was designed for it. I am also not sure if this is the best way to export this information out of the reflection process, it's just the least invasive one I could come up with.

[stefanhaller]

Hm, true. You could leave the old PR open and post a new one for the new approach, and leave it to the maintainers to decide which one they like better. But then, there was already little response to the first PR, so I don't have much hope that there will be a lot more response to this one, or to the question which is better.