Fix double-slash URL path bug across 74 GetRequest methods

[sean-]

Summary

When optional path segments (Indices, Index, Repo, TaskID, etc.) are empty, GetRequest() methods produced a double-slash (e.g. //_alias/myalias) which http.NewRequest misparses per RFC 3986, treating the next segment as the URL authority. After setReqURL overwrites the host, the path loses its API prefix entirely, causing requests to hit the wrong endpoint.

Observed symptoms:

• [BUG] PUT Alias creates index and not an alias #650: AliasPutReq with empty Indices creates an index instead of an alias
• [BUG] Wrong request path computed in AliasGetReq when no indices are specified #617: AliasGetReq with empty Indices returns wrong error format (hits index lookup instead of alias lookup)

Root cause: 74 GetRequest() methods across opensearchapi, plugins/security, and plugins/ism unconditionally wrote "/" + variable into the URL path without guarding against empty values.

Fix: Extract opensearch.BuildPath helper that skips empty segments, preventing the double-slash class of bug. All 74 vulnerable methods converted to use it. Benchmarked at 1 alloc / ~21ns vs path.Join's 3 allocs / ~63ns.

Fixes: #617, #650

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[codecov]

Codecov Report

❌ Patch coverage is 54.49804% with 2443 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.51%. Comparing base (8aef106) to head (5462692).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/osgen/emit/build.go	0.33%	891 Missing ⚠️
cmd/osgen/api_cmd.go	36.42%	236 Missing and 38 partials ⚠️
cmd/osgen/resp_walk.go	64.00%	94 Missing and 23 partials ⚠️
cmd/osgen/paths_testgen.go	48.21%	103 Missing and 13 partials ⚠️
cmd/osgen/emit/frag_resp.go	22.96%	103 Missing and 1 partial ⚠️
cmd/osgen/ir_bridge.go	61.37%	83 Missing and 7 partials ⚠️
cmd/osgen/emit/format.go	59.63%	80 Missing and 8 partials ⚠️
cmd/osgen/resp_union.go	62.27%	69 Missing and 14 partials ⚠️
cmd/osgen/api_extract.go	79.44%	51 Missing and 23 partials ⚠️
cmd/osgen/version_filter.go	69.15%	60 Missing and 2 partials ⚠️
... and 25 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #804       +/-   ##
===========================================
- Coverage   73.45%   52.51%   -20.95%     
===========================================
  Files         431      990      +559     
  Lines       16398    59059    +42661     
===========================================
+ Hits        12045    31014    +18969     
- Misses       2705    24939    +22234     
- Partials     1648     3106     +1458     

Flag	Coverage Δ
integration	33.63% <ø> (-33.66%)	⬇️
unit	46.20% <54.49%> (+0.64%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
cmd/osgen/api_render_union.go	100.00% <100.00%> (ø)
cmd/osgen/emit/imports.go	100.00% <100.00%> (ø)
cmd/osgen/spec.go	100.00% <100.00%> (ø)
internal/apiutil/duration.go	50.00% <ø> (ø)
internal/build/request.go	81.94% <ø> (ø)
internal/params/params.go	21.42% <ø> (ø)
internal/path/builders_gen.go	99.51% <ø> (ø)
internal/test/readiness/cluster.go	66.15% <ø> (ø)
internal/test/readiness/diagnostic.go	84.72% <ø> (ø)
internal/test/readiness/harness.go	61.01% <ø> (ø)
... and 184 more

... and 603 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[VijayanB]

Thanks for the fix! Have you had a chance to verify that this same issue doesn't exist in other APIs as well?

[sean-]

Yes, it's fixed now for every call site, not just the opensearchapi package.

[Jakob3xD]

I mislike that concept of omitting required values that are empty. Doing so can cause a lot of different behaviours. The correct approach would be to validate the user input, to not run into the double slash situation.

[sean-]

Heh, I agree — and almost went with something closer to my conventional approach, which is to define a type for each URL segment and pass a config struct with proper validation. Let me update this PR to match the revised approach, since I'm on the same page but didn't want to overdo it given the ubiquity and surface area of the change (even if it's mechanical). Will push a follow-up shortly.

[sean-]

@Jakob3xD What do you think of this?

New (proposed):

func (r AliasPutReq) GetRequest() (*http.Request, error) {
    path, err := opensearch.AliasPath{
        Indices: r.Indices,
        Alias:   r.Alias,
    }.Build()
    if err != nil {
        return nil, err
    }
    return opensearch.BuildRequest(http.MethodPut, path, nil, r.Params.get(), r.Header)
}

Where AliasPutReq.Indices is opensearch.Indices ([]Index), and Build() validates and handles comma-joining internally.

For call sites where the fields are known to be populated, MustBuild enables inline usage (panics on error, like template.Must*()):

func (r AliasPutReq) GetRequest() (*http.Request, error) {
    return opensearch.BuildRequest(
        http.MethodPut,
        opensearch.MustBuild(opensearch.AliasPath{Indices: r.Indices, Alias: r.Alias}.Build()),
        nil,
        r.Params.get(),
        r.Header,
    )
}

[sean-]

Alright, this has been pushed up. It was a large but very mechanical change. Every GetRequest() method across opensearchapi and the plugins now delegates path construction to validated, typed builder structs in path.go instead of fmt.Sprintf(). The builder structs reject empty required segments with a clear error rather than silently producing double-slash URLs that http.NewRequest misparses per RFC 3986.

The change intentionally keeps all published API types (Req struct fields) as plain string or []string to prevent any breakage. Domain types (Index, Action, DocumentID, etc.) are purely internal to the path builders and the GetRequest() methods handle the cast from public types to domain types.

I think this is a net positive even though the diff is sweeping: the boilerplate to define the path structs eliminates an entire class of bugs (e.g. empty segment -> double slash -> authority confusion) and by making the path shape explicit and testable.

For the future v5 branch, I would consider breaking the published API into domain-specific types for the additional safety, but I don't think that's very high ROI compared to what we did here.

[sean-]

@Jakob3xD Can you give this another shake or bless this?

[sean-]

Run 26578837681 / Job 78305677820 — failure is not caused by this PR.

What hung

TestNodesInfo hit a 6m 30s readiness timeout. Once that test burned through the budget, TestNodesStats had only 3m 13s left before the package-wide -timeout=10m fired and panic: test timed out killed the process.

--- FAIL: TestNodesInfo (390.03s)
    Error: readiness timeout
    Messages: 2/3 nodes satisfy LayerStatsReady (need 3), 391 polls over 6m30.007s
panic: test timed out after 10m0s
    running tests:
        TestNodesStats (3m13s)

Why the gate never advanced

LayerStatsReady requires _cat/nodes to return non-null cpu and heap.percent for every expected node (clusterlens.go:101). Last poll (391st):

node	cpu	heap.percent	cluster_manager
opensearch-node1 (172.18.0.4)	0	51	-
opensearch-node2 (172.18.0.3)	null	null	-
opensearch-node3 (172.18.0.2)	0	14	* (master)

node2 joined the cluster (LayerClusterJoin reached at 13:53:06 — 1m 38s in) but its stats stayed null for the entire 6m 30s window.