Skip to content

Fix/no diff request transformer plugin #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix/no diff request transformer plugin #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
be81362
fix: sort nested sets in plugin config, but not arrays
harshadixit12 Sep 17, 2025
f36b9c9
fix: unused elements
harshadixit12 Nov 18, 2025
60ef361
tests: add integration tests
harshadixit12 Nov 18, 2025
7ffa6ef
tests: refactor and add new case
harshadixit12 Nov 18, 2025
23c0114
style: format test file
harshadixit12 Nov 18, 2025
55acf81
tests: run for kong v3 and above
harshadixit12 Nov 19, 2025
b581fb2
tests: run for all gateway versions
harshadixit12 Nov 19, 2025
4b70ff1
refactor: address feedback
harshadixit12 Nov 20, 2025
6d356e4
refactor: move constants back to function scope
harshadixit12 Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
225 changes: 225 additions & 0 deletions pkg/state/fixtures/test-plugin-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,225 @@
{
Copy link
Contributor Author

@harshadixit12 harshadixit12 Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fictional plugin schema - contains complex field structure - nested set / array within record, etc.

"fields": [
{
"config": {
"fields": [
{
"primitive_str": {
"description": "The primitive string field.",
"type": "string"
}
},
{
"record_of_array_of_str": {
"fields": [
{
"headers": {
"default": [],
"elements": {
"type": "string"
},
"required": true,
"type": "array"
}
}
],
"required": true,
"type": "record"
}
},
{
"map_type": {
"description": "The custom query params to be added in the callout HTTP request. Values can contain Lua expressions in the form `$(some_lua_expression)`. The syntax is based on `request-transformer-advanced` templates.",
"keys": {
"type": "string"
},
"required": false,
"type": "map",
"values": {
"referenceable": true,
"required": false,
"type": "string"
}
}
},
{
"array_of_record": {
"description": "Sentinel node addresses to use for Redis connections when the `redis` strategy is defined. Defining this field implies using a Redis Sentinel. The minimum length of the array is 1 element.",
"elements": {
"fields": [
{
"host": {
"default": "127.0.0.1",
"description": "A string representing a host name, such as example.com.",
"required": true,
"type": "string"
}
},
{
"port": {
"default": 6379,
"description": "An integer representing a port number between 0 and 65535, inclusive.",
"type": "integer"
}
}
],
"type": "record"
},
"len_min": 1,
"required": false,
"type": "array"
}
},
{
"nested_array_of_array_of_str": {
"type": "array",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "array",
"elements": {
"type": "string"
}
}
}
},
{
"nested_array_of_set_of_str": {
"type": "array",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "set",
"elements": {
"type": "string"
}
}
}
},
{
"nested_set_of_array_of_str": {
"type": "set",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "array",
"elements": {
"type": "string"
}
}
}
},
{
"nested_set_of_set_of_str": {
"type": "set",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "set",
"elements": {
"type": "string"
}
}
}
},
{
"nested_array_of_record_of_array": {
"type": "array",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "array",
"elements": {
"fields": [
{
"ports": {
"elements": {
"type": "number"
},
"type": "array"
}
}
],
"type": "record"
}
}
}
},
{
"nested_array_of_record_of_set": {
"type": "array",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "array",
"elements": {
"fields": [
{
"ports": {
"elements": {
"type": "number"
},
"type": "set"
}
}
],
"type": "record"
}
}
}
},
{
"nested_set_of_record_of_set": {
"type": "array",
"required": false,
"description": "List of list of strings.",
"elements": {
"type": "set",
"elements": {
"fields": [
{
"ports": {
"elements": {
"type": "number"
},
"type": "set"
}
}
],
"type": "record"
}
}
}
}
],
"required": true,
"type": "record",
"shorthand_fields": [
{
"shorthand_record_of_set_array": {
"fields": [
{
"set_hosts": {
"elements": {
"type": "string"
},
"type": "set"
}
},
{
"array_hosts": {
"elements": {
"type": "string"
},
"type": "array"
}
}
],
"type": "record"
}
}
]
}
}
]
}
72 changes: 57 additions & 15 deletions pkg/state/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"

"github.com/kong/go-kong/kong"
"github.com/tidwall/gjson"
)

// entity abstracts out common fields in a credentials.
Expand Down Expand Up @@ -565,14 +566,14 @@ func (p1 *Plugin) Console() string {
// Equal returns true if r1 and r2 are equal.
// TODO add compare array without position
func (p1 *Plugin) Equal(p2 *Plugin) bool {
Copy link
Contributor Author

@harshadixit12 harshadixit12 Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For plugin, this function didn't seem to be used directly anywhere.
However, it makes sense to expect schema as org here as well - will make that change in next pass.

Copy link
Collaborator

@Prashansa-K Prashansa-K Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as naming convention goes, any "options" should be passed as args in EqualWithOpts function only. So, we should keep this one as it is.

return p1.EqualWithOpts(p2, false, false, false)
return p1.EqualWithOpts(p2, false, false, false, gjson.Result{})
}

// EqualWithOpts returns true if p1 and p2 are equal.
// If ignoreID is set to true, IDs will be ignored while comparison.
// If ignoreTS is set to true, timestamp fields will be ignored.
func (p1 *Plugin) EqualWithOpts(p2 *Plugin, ignoreID,
ignoreTS, ignoreForeign bool,
ignoreTS, ignoreForeign bool, schema gjson.Result,
) bool {
p1Copy := p1.Plugin.DeepCopy()
p2Copy := p2.Plugin.DeepCopy()
Expand All @@ -590,8 +591,10 @@ func (p1 *Plugin) EqualWithOpts(p2 *Plugin, ignoreID,
sort.Slice(p1Copy.Protocols, func(i, j int) bool { return *(p1Copy.Protocols[i]) < *(p1Copy.Protocols[j]) })
sort.Slice(p2Copy.Protocols, func(i, j int) bool { return *(p2Copy.Protocols[i]) < *(p2Copy.Protocols[j]) })

p1Copy.Config = sortNestedArrays(p1Copy.Config)
p2Copy.Config = sortNestedArrays(p2Copy.Config)
const pluginConfigKey = "fields.#(config).config"
configSchema := schema.Get(pluginConfigKey)
p1Copy.Config = sortNestedArraysBasedOnSchema(p1Copy.Config, configSchema)
p2Copy.Config = sortNestedArraysBasedOnSchema(p2Copy.Config, configSchema)

if ignoreID {
p1Copy.ID = nil
Expand Down Expand Up @@ -672,26 +675,61 @@ func (e EmptyInterfaceUsingUnderlyingType) Less(i, j int) bool {
return strings.Compare(objI, objJ) == -1
}

// Helper function to sort nested arrays in a map
func sortNestedArrays(m map[string]interface{}) map[string]interface{} {
sortedMap := make(map[string]interface{})
// Helper function to get schema for a field name
func getSchemaForFieldName(schema gjson.Result, fieldName string) gjson.Result {
const fieldsQueryTemplate = "fields.#(%s).%s"
const shorthandFieldsQueryTemplate = "shorthand_fields.#(%s).%s"
fieldsQuery := fmt.Sprintf(fieldsQueryTemplate, fieldName, fieldName)
result := schema.Get(fieldsQuery)
if !result.Exists() {
// try shorthand fields
shorthandQuery := fmt.Sprintf(shorthandFieldsQueryTemplate, fieldName, fieldName)
result = schema.Get(shorthandQuery)
}
return result
}

// Helper function to determine if we should sort based on schema
func shouldSort(schema gjson.Result) bool {
if !schema.Exists() {
return true
}

typeResult := schema.Get("type")
if !typeResult.Exists() {
return true
}

return typeResult.String() != "array"
}

// Helper function to sort nested arrays in a map referring to schema
func sortNestedArraysBasedOnSchema(m map[string]interface{}, schema gjson.Result) map[string]interface{} {
Copy link
Contributor Author

@harshadixit12 harshadixit12 Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation of sort is slower than previous, by 10x based on benchmark. However, it still takes few microseconds, which will be negligible compared to rest.

Copy link
Collaborator

@Prashansa-K Prashansa-K Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If schema is nil in any case, should we consider any early exit here instead of relying on solely skipSort?

Copy link
Contributor Author

@harshadixit12 harshadixit12 Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to fallback to previous behaviour IMO in case of not being able to get schema - which is to sort and compare. If we do early exit, any set that is in different order in deck file compared to gateway will also show up as diff.
In any case, nil in the gjson result is a empty result object - which leads to skipSort returning false

sortedMap := make(map[string]interface{}, len(m))

for k, v := range m {
switch value := v.(type) {
case []interface{}:
currSchema := getSchemaForFieldName(schema, k)
// For list types like array or set, get the element schema
elementsSchema := currSchema.Get("elements")
// Recursively sort each element if it's a map or array
for i, elem := range value {
switch elemType := elem.(type) {
case map[string]interface{}:
value[i] = sortNestedArrays(elemType)
value[i] = sortNestedArraysBasedOnSchema(elemType, elementsSchema)
case []interface{}:
value[i] = sortArrayElementsRecursively(elemType)
value[i] = sortArrayElementsRecursivelyBasedOnSchema(elemType, elementsSchema)
}
}
sort.Sort(EmptyInterfaceUsingUnderlyingType(value))

if shouldSort(currSchema) {
sort.Sort(EmptyInterfaceUsingUnderlyingType(value))
}
sortedMap[k] = value
case map[string]interface{}:
sortedMap[k] = sortNestedArrays(value)
currSchema := getSchemaForFieldName(schema, k)
sortedMap[k] = sortNestedArraysBasedOnSchema(value, currSchema)
default:
sortedMap[k] = value
}
Expand All @@ -701,17 +739,21 @@ func sortNestedArrays(m map[string]interface{}) map[string]interface{} {
}

// Helper function to sort array elements recursively
func sortArrayElementsRecursively(arr []interface{}) []interface{} {
func sortArrayElementsRecursivelyBasedOnSchema(arr []interface{}, parentSchema gjson.Result) []interface{} {
elementsSchema := parentSchema.Get("elements")

for i, elem := range arr {
switch elemType := elem.(type) {
case map[string]interface{}:
arr[i] = sortNestedArrays(elemType)
arr[i] = sortNestedArraysBasedOnSchema(elemType, elementsSchema)
case []interface{}:
arr[i] = sortArrayElementsRecursively(elemType)
arr[i] = sortArrayElementsRecursivelyBasedOnSchema(elemType, elementsSchema)
}
}

sort.Sort(EmptyInterfaceUsingUnderlyingType(arr))
if shouldSort(parentSchema) {
sort.Sort(EmptyInterfaceUsingUnderlyingType(arr))
}
return arr
}

Expand Down
Loading
Loading