Arazzo Parser v1#1568
Conversation
arcuri82
left a comment
arcuri82
left a comment
There was a problem hiding this comment.
hi @daniellopera15 thx for your first PR! ;)
see my comments.
sorry for delay, i was on a 2-week leave
| <!-- </execution>--> | ||
| <!-- </executions>--> | ||
| <!-- </plugin>--> | ||
| <!-- <plugin>--> |
There was a problem hiding this comment.
why modifying this file as part of this PR?
There was a problem hiding this comment.
This was a mistake due to the IDE's formatting. Reverted.
| <parent> | ||
| <groupId>org.evomaster</groupId> | ||
| <artifactId>evomaster-core-extra</artifactId> | ||
| <version>5.1.1-SNAPSHOT</version> |
There was a problem hiding this comment.
update to 6.0.1-SNAPSHOT
| <dependency> | ||
| <groupId>io.swagger.core.v3</groupId> | ||
| <artifactId>swagger-models</artifactId> | ||
| <version>2.2.34</version> |
There was a problem hiding this comment.
this version should not be here, but in a parent pom, where other swagger dependency are declared, ie., core-parent/pom.xml
| <dependency> | ||
| <groupId>org.mock-server</groupId> | ||
| <artifactId>mockserver-client-java-no-dependencies</artifactId> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
why a compile dependency for this?
There was a problem hiding this comment.
This dependency was removed because it had been added during a previous implementation of the tests that ultimately went unused.
| <dependency> | ||
| <groupId>io.swagger.core.v3</groupId> | ||
| <artifactId>swagger-core</artifactId> | ||
| <version>2.2.34</version> |
There was a problem hiding this comment.
use properties for repeated version numbers in the same dependency family
| } | ||
|
|
||
| private static Pair<ArazzoSpecificationsDTO, JsonNode> parseSchemaText(String schemaText) { | ||
| String schemaTextClean = schemaText.replaceAll("^\\s+", ""); |
There was a problem hiding this comment.
why is this needed? wouldn't Jackson handle it anyway?
There was a problem hiding this comment.
The previous functionality was necessary to prevent errors. It was removed when the solution you suggested was implemented.
| JsonNode arazzoJsonNode; | ||
|
|
||
| try { | ||
| if (schemaTextClean.startsWith("{")) { |
There was a problem hiding this comment.
this check is potntially brittle. maybe use try/catch like:
try {
JSON_MAPPER.readValue(input, Object.class);
} catch (JsonProcessingException e) {
// Not valid JSON, try YAML
YAML_MAPPER.readValue(input, Object.class);
}
| try { | ||
| if (schemaTextClean.startsWith("{")) { | ||
| arazzoSpecificationsDTO = JSON_MAPPER.readValue(schemaTextClean, ArazzoSpecificationsDTO.class); | ||
| arazzoJsonNode = JSON_MAPPER.readTree(schemaTextClean); |
There was a problem hiding this comment.
why is the file parsed twice? why having a POJO representation and a JsonNode at the same time?
There was a problem hiding this comment.
"arazzoSpecificationsDTO" is the parsed document. "arazzoJsonNode" is used to allow referencing via JsonNode when resolving references in ArazzoReferenceResolver.
| String referenceClean = referenceWithoutComponents.substring((expectedPrefix + ".").length()); | ||
| Object resolve; | ||
| switch (expectedPrefix) { | ||
| case "successActions": |
There was a problem hiding this comment.
all these hardcoded values like "successActions" should be put in some common constant
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
| public class ArazzoParserTest { |
There was a problem hiding this comment.
maybe not in this PR, but we will need more tests, eg, especially for all the code in the resolver
There was a problem hiding this comment.
It is a simple test to ensure functionality. The idea is to extend it to better tests.
3 participants
No description provided.