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

Feature 1436 load api defintion from url #3059

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

winzj
Copy link
Member

@winzj winzj commented Apr 4, 2024

@winzj winzj self-assigned this Apr 16, 2024
@winzj winzj requested a review from de-jcup April 16, 2024 08:02
Copy link
Member

@de-jcup de-jcup left a 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);
}

/**
*
Copy link
Member

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

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());
Copy link
Member

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 ?

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

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

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?

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.

webscan config support loading api definitions from URL
2 participants