Fix: resolve remote relative refs correctly

[felixjung]

Thanks for working on all these tools for OpenAPI. 👏

I've run into issues with vacuum/libopenapi supporting resolving of relative remote refs. I'm trying to fix this, but I have to admit I get completely lost in the code of this project.

I've added a test case to debug the issue. One thing I notice and that I think it should never happen, is that libopenapi tries to load a file that's not referenced anywhere. The log output says the following:

{"time":"2024-09-03T22:46:30.36193+02:00","level":"ERROR","msg":"unable to fetch remote document","file":"/felixjung/libopenapi/authed-remote/test_specs/schemas/components.openapi.yaml","status":404,"resp":"404: Not Found"}
{"time":"2024-09-03T22:46:30.362202+02:00","level":"ERROR","msg":"unable to locate reference anywhere in the rolodex","reference":"https://raw.githubusercontent.com/felixjung/libopenapi/authed-remote/test_specs/schemas/components.openapi.yaml#/components/responses/ListAccounts"}
{"time":"2024-09-03T22:46:30.362272+02:00","level":"ERROR","msg":"error building path item: map build failed: reference cannot be found: reference 'https://raw.githubusercontent.com/felixjung/libopenapi/authed-remote/test_specs/schemas/components.openapi.yaml#/components/responses/ListAccounts' at line 31, column 11 was not found"}

Instead of

https://raw.githubusercontent.com/felixjung/libopenapi/authed-remote/test_specs/schemas/components.openapi.yaml#/components/responses/ListAccounts

it should only ever try to resolve

https://raw.githubusercontent.com/felixjung/libopenapi/authed-remote/test_specs/minimal_remote_refs/schemas/components.openapi.yaml#/components/responses/ListAccounts

When I call a Render method on the document's model, components are missing and refs have not been resolved (maybe I'm using it incorrectly).

To run the test do

go test -count 1 -v -run TestDocument_MinimalRemoteRefs .

I'd be very appreciative for any hints as to what might be going wrong.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (5ac8657) to head (b22da5d).
Report is 21 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #327   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files         165      165           
  Lines       21050    21081   +31     
=======================================
+ Hits        20971    21002   +31     
  Misses         74       74           
  Partials        5        5           

Flag	Coverage Δ
unittests	99.62% <100.00%> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[daveshanley]

This one is simple. it's a bug, but can be worked around, and the bugfix is simple. Add a '/' to the end of your URL, i.e :

baseURL, err := url.Parse("https://raw.githubusercontent.com/felixjung/libopenapi/authed-remote/test_specs/minimal_remote_refs/")

https://github.com/pb33f/libopenapi/blob/main/datamodel/low/extraction_functions.go#L150 <-- this line is ripping off minimal_remote_refs because it's trying to create a filepath. the last segment of the path looks like a file (minimal_remote_refs)

To patch the code:

if u.Path != "" {
    if u.Path[len(u.Path)-1:] == "/" {
        p = filepath.Dir(u.Path)
    } else {
        p = u.Path
    }
}

[daveshanley]

Thanks for working on all these tools for OpenAPI. 👏

I've run into issues with vacuum/libopenapi supporting resolving of relative remote refs. I'm trying to fix this, but I have to admit I get completely lost in the code of this project.

It's a little over-engineered to cater to a ton of feature work I need in the future, but not today, so it's heavy duty - but because it will need to be down the line. :)

[felixjung]

I was able to fix the bug, I guess. What I'm struggling with now is how to write a test that actually fails before committing my fix. Why would building the document or bundling the spec not fail, if a reference cannot be resolved? In this case the request to GitHub to download the file will fail without the fix and that should fail the bundling or document building process. 🤔

[felixjung]

I've added a bundler test case, which revealed another issue in that the local reference in the file referenced from the root is not included in the bundle result.

[felixjung]

Interestingly, the following test passes just fine:

func TestBundleDocument_MinimalRefs(t *testing.T) {
	newRemoteHandlerFunc := func() utils.RemoteURLHandler {
		c := &http.Client{
			Timeout: time.Second * 120,
		}

		return func(url string) (*http.Response, error) {
			resp, err := c.Get(url)
			if err != nil {
				return nil, fmt.Errorf("fetch remote ref: %v", err)
			}

			return resp, nil
		}
	}

	specBytes, err := os.ReadFile("../test_specs/minimal_remote_refs/openapi.yaml")
	require.NoError(t, err)

	require.NoError(t, err)

	config := &datamodel.DocumentConfiguration{
		AllowFileReferences:   true,
		AllowRemoteReferences: false,
		BundleInlineRefs:      false,
		BasePath:              "../test_specs/minimal_remote_refs",
		BaseURL:               nil,
		RemoteURLHandler:      newRemoteHandlerFunc(),
	}
	require.NoError(t, err)

	bytes, e := BundleBytes(specBytes, config)
	assert.NoError(t, e)
	assert.Contains(t, string(bytes), "Name of the account", "should contain all reference targets")
}

What I've found, trying to debug this, is that there is only one index for the root file. There is no index for the referenced file, despite it containing references to components.

[felixjung]

Cleaned up the commit history and this should now be ready to go.

[felixjung]

Of course I didnn't see the failing test locally. Damn you, Windows.

[daveshanley]

I love all the new comments in extraction_functions That block of code is so hard to understand because it handles so many variations and edge cases. (there are others like it in the resolver and the index btw).

Thank you for digging into this code, it's over-engineered for a reason and getting down and dirty with it, can leave you fried.

Thank you for your contribution!