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
Feature 1436 load api defintion from url #3059
base: develop
Are you sure you want to change the base?
Conversation
winzj
commented
Apr 4, 2024
- closes webscan config support loading api definitions from URL #1436
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.
Good job.
I have some thoughts and found some minor parts but I think it is good enough to merge directly. But please read my comments and decide if there are small changes necessary or not.
@@ -234,6 +235,18 @@ public ApiResponse importOpenApiFile(String openApiFile, String url, String cont | |||
return clientApi.openapi.importFile(openApiFile, url, contextId); | |||
} | |||
|
|||
/** | |||
* |
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.
Please: Either remove the javadoc part or write a text and describe the parameters.
@@ -48,4 +52,20 @@ void json_attribute_use_is_handled_correctly_by_to_json() { | |||
assertEquals(expected, json); | |||
} | |||
|
|||
@Test | |||
void api_definition_url_is_handled_correctly() throws MalformedURLException { |
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.
Please add a /* prepare */
/* test */ | ||
String expected = "{\"apiDefinitionUrl\":\"https://example.com/api/v1/swagger/\",\"use\":[]}"; | ||
assertEquals(expected, json); | ||
assertEquals(config.getApiDefinitionUrl(), apiConfig.getApiDefinitionUrl()); |
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.
What about a renaming here ?
assertEquals(config.getApiDefinitionUrl(), apiConfig.getApiDefinitionUrl()); | |
assertEquals(originConfig.getApiDefinitionUrl(), loadedConfig.getApiDefinitionUrl()); |
@@ -12,7 +12,8 @@ | |||
"url" : "https://productfailure.demo.example.org", | |||
"api" : { | |||
"type" : "openApi", //<2> | |||
"use" : [ "open-api-file-reference" ] //<3> | |||
"use" : [ "open-api-file-reference" ], //<3> |
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.
Have we ensured that a file without "apiDefintionUrl" inside is also loaded as well - i am not sure here.
"url" : "https://localhost:8443", | ||
"api" : { | ||
"type" : "openApi", | ||
"apiDefinitionUrl" : "https://example.com/api/v1/swagger/?format=openapi", |
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.
Is it correct that we allow "use" + "apiDefinitionUrl" at the same time?