Replace use of JsonObject with JsonNode for CruiseControlApi #10860
Conversation
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
tinaselenge
force-pushed
the
cc-remove-vertx-json
branch
from
c1f27c5
to
a386ddd
Compare
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
...n/java/io/strimzi/operator/cluster/operator/resource/cruisecontrol/CruiseControlApiImpl.java
Outdated
Show resolved
Hide resolved
| userTaskID = response.headers().firstValue(USER_TASK_ID_HEADER).orElse(""); | ||
|
|
||
| if (json.has(CC_REST_API_ERROR_KEY)) errorMessage = json.get(CC_REST_API_ERROR_KEY).asText(); | ||
| } |
There was a problem hiding this comment.
I understood what you are trying to achieve with the above code. You are "centralising" where the JSON is extracted from the response for the several status codes. The drawbacks I see is that if we need to handle an additional statusCode, we always need to add the new code here as well (and not just later on where we have the if branch for handling its logic).
I was wondering if it's better factoring out a private method to contain the code which extract the JSON from the response or returning the CruiseControlRestException exception if any error.
Using this method in all the places where we actually have JsonObject json = new JsonObject(response.body()); and leaving the rest of the code the same as it's currently. Would it work in your opinion?
There was a problem hiding this comment.
That is a good point. I took your suggestion made an update. Thank you Paolo.
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
Type of change
Select the type of your PR
Description
This PR replaces Vertx's JsonObject used for parsing CC API responses with Jackson's JsonNode. So that Vertx is completely removed from CC API.
Checklist
Please go through this checklist and make sure all applicable tasks have been done