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: windows support for path loading #71

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

fredbi
Copy link
Member

@fredbi fredbi commented Dec 5, 2023

No description provided.

@fredbi fredbi force-pushed the fix/windows-invalid-url branch from cdba2d9 to d1d6372 Compare December 5, 2023 18:52
@fredbi
Copy link
Member Author

fredbi commented Dec 5, 2023

need to run an integration test with go-swagger's test suite

go-swagger/go-swagger#2995

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (f28dd7a) 85.59% compared to head (6c3422a) 86.38%.

Files Patch % Lines
loading.go 88.09% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   85.59%   86.38%   +0.79%     
==========================================
  Files          13       13              
  Lines        1569     1587      +18     
==========================================
+ Hits         1343     1371      +28     
+ Misses        193      183      -10     
  Partials       33       33              

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

@fredbi fredbi force-pushed the fix/windows-invalid-url branch from 29362e3 to 86392de Compare December 5, 2023 19:09
On windows, there are quite a few specifics when it comes to resolving
a local file. This complexity stemmed from the desire to support
common, albeit invalid URI constructs for windows.

Overall, the original sin of this function has been to ignore the host
part of an URI, but now we can't really break this behavior.

* fixes go-openapi#72
* refactored the local path pre-processing, so the windows-specifics
  appear more clearly
* fixed a few inconsistencies on windows file URIs and guarded against a panic

* test: refactored tests for the local file strategy, now table-driven and
  more systematic
* doc: added detailed documentation to LoadStrategy()
* ci: re-enacted tests on windows
* ci: temporarily muted past linting issues: relinting is deferred to another PR

Signed-off-by: Frederic BIDON <[email protected]>
@fredbi fredbi force-pushed the fix/windows-invalid-url branch from 86392de to 6c3422a Compare December 5, 2023 19:13
fredbi added a commit to fredbi/go-swagger that referenced this pull request Dec 5, 2023
@fredbi fredbi marked this pull request as ready for review December 5, 2023 21:00
@fredbi fredbi changed the title WIP: fix: windows support for paths fix: windows support for path loading Dec 5, 2023
@fredbi
Copy link
Member Author

fredbi commented Dec 5, 2023

Fixing this, I now realize the original sin of the LoadStrategy() func when dealing with file:// URI...
The shortcut with a simple strings.TrimPrefix(path,"file://"), removing the host semantics has deep implications
and makes it now very difficult to expose a consistent behavior on both unix and windows...

Well now we have to keep it this way, and make sure that this also works on windows.

In hindsight, I have some regrets not to impose earlier a strict approach to file URIs...

At least it works, runs tests, doesn't break go-swagger CI...

@fredbi fredbi requested review from casualjim and youyuanwu December 5, 2023 21:06
@fredbi fredbi mentioned this pull request Dec 5, 2023
@fredbi fredbi merged commit 4de0676 into go-openapi:master Dec 6, 2023
9 checks passed
@fredbi fredbi deleted the fix/windows-invalid-url branch December 6, 2023 13:03
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.

3 participants