-
Notifications
You must be signed in to change notification settings - Fork 55
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(#71): naive fix excluding absolute path #72
Conversation
internal/application/config/Load.go
Outdated
@@ -70,7 +70,9 @@ func autocorrectValues(filename string, fileConfig *fileConfiguration) { | |||
} | |||
|
|||
if specificationURLIsRelativeFilename(filename, fileConfig) { | |||
fileConfig.OpenAPI.SpecificationURL = filepath.Dir(filename) + "/" + fileConfig.OpenAPI.SpecificationURL | |||
if !strings.HasPrefix(fileConfig.OpenAPI.SpecificationURL, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thanks for PR. I think this check should be inside specificationURLIsRelativeFilename
function.
func specificationURLIsRelativeFilename(filename string, fileConfig *fileConfiguration) bool {
return filename != "" &&
fileConfig.OpenAPI.SpecificationURL != "" &&
!fileConfig.OpenAPI.urlFromEnv &&
!strings.HasPrefix(fileConfig.OpenAPI.SpecificationURL, "/") &&
!strings.HasPrefix(fileConfig.OpenAPI.SpecificationURL, "http")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
indeed it is easier to read and corresponds well to the need of the function.
Finaly the test is strings.HasPrefix(fileConfig.OpenAPI.SpecificationURL, "./")
Testing http and / is not useful just ./
func specificationURLIsRelativeFilename(filename string, fileConfig *fileConfiguration) bool {
return filename != "" &&
fileConfig.OpenAPI.SpecificationURL != "" &&
!fileConfig.OpenAPI.urlFromEnv &&
strings.HasPrefix(fileConfig.OpenAPI.SpecificationURL, "./")
}
I have a question : in this case is prefixing with the directory a good solution? (filepath.Dir(filename) + "/"
) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing http and / is not useful just ./
I agree.
I have a question : in this case is prefixing with the directory a good solution? (filepath.Dir(filename) + "/") ?
I think if the path is relative, then it is expected to be relative to the configuration file, but not relative to the working directory. For example, if you run docker compose from another directory, all paths will be relative to the configuration file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I hadn't thought of that scenario.
So perhaps the information on the documentation should be added to clarify this. I will create a ticket for this.
I updated the code to test if the path starts with ./ .
Thanks
No description provided.