Skip to content

fix(admin): allow bidirectional format conversion for upstream.nodes in PATCH requests#13065

Open
janiussyafiq wants to merge 3 commits intoapache:masterfrom
janiussyafiq:fix/patch-nodes
Open

fix(admin): allow bidirectional format conversion for upstream.nodes in PATCH requests#13065
janiussyafiq wants to merge 3 commits intoapache:masterfrom
janiussyafiq:fix/patch-nodes

Conversation

@janiussyafiq
Copy link
Contributor

Description

Fixes asymmetric PATCH behavior when converting upstream.nodes between array and hash table formats.

Problem Statement

The Admin API exhibits inconsistent behavior when patching a route's upstream.nodes field:

  • Converting from array → hash table works
  • Converting from hash table → array fails with validation error:
    {"error_msg":"invalid configuration: property \"upstream\" validation failed: property \"nodes\" validation failed: object matches none of the required"}

This happens despite both formats being officially supported by the schema.

Root Cause

The merge() function in apisix/core/table.lua only checks if the origin value is an array before deciding whether to replace or recursively merge:

if _M.nkeys(origin[k]) ~= #origin[k] then
  merge(origin[k] or {}, extend[k] or {})  -- Merge if origin is hash
else
  origin[k] = v  -- Replace if origin is array
end

When patching from hash table → array:

  • Origin is hash table, so it tries to merge
  • This creates an invalid mixed structure with both string keys (from hash) and numeric indexes (from array)
  • Validation fails because it's neither a valid hash nor a valid array

Solution

Modified the merge logic to check both the origin and incoming values:

  • If either is an array → replace (prevents invalid mixed structures)
  • If both are hash tables → merge (safe to combine)

Which issue(s) this PR fixes:

Fixes #13045

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Mar 4, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes asymmetric Admin API PATCH behavior when upstream.nodes changes between array and hash-table formats by adjusting the deep-merge logic to avoid creating mixed array/hash tables during patch merges.

Changes:

  • Update core.table.merge() to replace (not recursively merge) when either the existing value or incoming value is an array.
  • Add an Admin API test that exercises PATCH conversions between array and hash-table upstream.nodes formats.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
apisix/core/table.lua Adjusts merge behavior to prevent invalid mixed table structures when patching between array/hash formats.
t/admin/routes-array-nodes.t Adds a regression test for PATCHing upstream.nodes across both supported representations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +125 to +163
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"uri": "/index.html",
"upstream": {
"nodes": [{
"host": "127.0.0.1",
"port": 8080,
"weight": 1
}],
"type": "roundrobin"
}
}]]
)

code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PATCH,
[[{
"upstream": {
"nodes": {
"127.0.0.1:9200": 1
}
}
}]]
)

code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PATCH,
[[{
"upstream": {
"nodes": [{
"host": "127.0.0.1",
"port": 8080,
"weight": 1
}],
"type": "roundrobin"
}
}]]
)
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

This test performs multiple admin calls (PUT, then PATCH array→hash, then PATCH hash→array) but only asserts the final call’s result. If the intermediate PATCH fails, the test can still pass because later calls overwrite code/body. Add checks after each call (e.g., if code >= 300 then ... return end) so the test fails as soon as any step fails, and so it actually verifies both conversions.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

merge(origin[k] or {}, extend[k] or {})
else
local origin_is_array = _M.nkeys(origin[k]) == #origin[k]
local extend_is_array = _M.nkeys(v) == #v
Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to use table.isarray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Inconsistent behavior when PATCHing a route's upstream.nodes: cannot change from hash table format back to array format

4 participants