-
Notifications
You must be signed in to change notification settings - Fork 523
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 to SwaggerCompatConverter so it can load inputSpec from classpath. #580
Fix to SwaggerCompatConverter so it can load inputSpec from classpath. #580
Conversation
Copied the code from Swagger20Parser
Thanks for the PR. Can you add a test to it? |
@@ -186,7 +191,22 @@ public ResourceListing readResourceListing(String input, MessageBuilder messages | |||
String json = RemoteUrl.urlToString(input, auths); | |||
jsonNode = Json.mapper().readTree(json); | |||
} else { | |||
jsonNode = Json.mapper().readTree(new File(input)); | |||
final String fileScheme = "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.
Its final string. Should be constant.
jsonNode = Json.mapper().readTree(new File(input)); | ||
final String fileScheme = "file:"; | ||
java.nio.file.Path path; | ||
if (input.toLowerCase().startsWith(fileScheme)) { |
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.
To avoid NPE, to compare with fileSchema, as it is constant
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.
No, it can't throw NPE on line 196, it'll throw it in the previous if(input.startsWith("http")) statement.
} | ||
String json; | ||
if (Files.exists(path)) { | ||
json= FileUtils.readFileToString(path.toFile(), "UTF-8"); |
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.
StandardCharsets already has "UTF-8" constant
@@ -489,7 +509,21 @@ public ApiDeclaration readDeclaration(String input, MessageBuilder messages, Lis | |||
String json = RemoteUrl.urlToString(input, auths); | |||
jsonNode = Json.mapper().readTree(json); | |||
} else { | |||
jsonNode = Json.mapper().readTree(new java.io.File(input)); | |||
final String fileScheme = "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.
Any reason for duplicate code ? Can't we refactor this code into function
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.
I took this exact code from https://github.com/swagger-api/swagger-parser/blob/master/modules/swagger-parser/src/main/java/io/swagger/parser/Swagger20Parser.java#L46
Maybe it makes sense to refactor this code too?
@Dema thanks, sorry for the delay on this. |
Swagger20Parser already can load input spec from classpath, but codegen project loads extensions one them is SwaggerCompatConverter and it fails loading from classpath
This fixes swagger-api/swagger-codegen#2666