Skip to content
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.

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

Fix: resolve remote relative refs correctly #327

Merged
merged 14 commits into from
Oct 29, 2024

Conversation

felixjung
Copy link
Contributor

@felixjung felixjung commented Sep 3, 2024

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.

Copy link

codecov bot commented Sep 3, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

felixjung commented Sep 13, 2024

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 felixjung marked this pull request as ready for review October 28, 2024 11:36
@felixjung
Copy link
Contributor Author

felixjung commented Oct 28, 2024

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

@felixjung
Copy link
Contributor Author

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

Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

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!

@daveshanley daveshanley merged commit 27d7834 into pb33f:main Oct 29, 2024
4 checks passed
@felixjung felixjung deleted the authed-remote branch November 11, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants