teragrep / cfe_31

0 stars 0 forks source link

Construct JsonResponse using the ApiResponse class inside Client.send method #23

Closed MoonBow-1 closed 6 months ago

MoonBow-1 commented 7 months ago

Description

As stated above, create a new ctor to remove code from the Client class:

return new JsonResponse(
    jsonResponse.id,
    jsonResponse.message,
    response.statusCode()
);

New version would be:

final JsonResponse jsonResponse = apiResponse.toJsonResponse();
MoonBow-1 commented 7 months ago

This was changed to use the ApiResponse to convert the HttpResponse to the JsonResponse as stated in #22.

MoonBow-1 commented 7 months ago

The ApiResponse class has been implemented and works as expected, except for the response for CaptureMeta, which doesn't return an id if it's successful. This required a simple check if null is present for a successful response:

if (!bodyAsJsonObject.has("id") &&
    (httpResponse.statusCode() == 200 || httpResponse.statusCode() == 201)
) {
    return new JsonResponse(
        0,
        bodyAsJsonObject.get("message").getAsString(),
        httpResponse.statusCode()
    );
}
MoonBow-1 commented 6 months ago

Currently there is a problem with responses for duplicate entries from CFE-18 https://github.com/teragrep/cfe_18/issues/24

MoonBow-1 commented 6 months ago

Error responses have been changed to be parsed as a string body, instead of JsonResponse:

if (response.isSuccessful()) {
    final ApiResponse apiResponse = new ApiResponse(response);
    final JsonResponse jsonResponse = apiResponse.toJsonResponse();
    Typewriter.response(jsonResponse);
    return jsonResponse;
} else {
    //  Non-successful responses return a String body, not JSON -> see ctor
    throw new ResponseException(response);
}

With the ResponseException ctor being:

public ResponseException(final Response response) throws IOException {
    this.statusCode = response.code();
    this.message = response.body() != null ? response.body().string() : null;
}
MoonBow-1 commented 6 months ago

This change has been reverted since CFE-18 now sends all responses with the JSON format

MoonBow-1 commented 6 months ago

Implementation details:

ApiResponse.toJsonResponse() method:

public JsonResponse toJsonResponse() throws ResponseException {
    if (this.httpResponse.body() != null) {
        try {
            final String bodyAsString = httpResponse.body().string();
            final JsonObject bodyAsJsonObject = JsonParser.parseString(bodyAsString)
                .getAsJsonObject();
            if (!bodyAsJsonObject.has("message")) {
                throw new ResponseException(
                    httpResponse.code(),
                    "Message is missing from response"
                );
            } else if (!bodyAsJsonObject.has("id")
                && bodyAsJsonObject.has("message")
                && !httpResponse.isSuccessful()
            ) {
                throw new ResponseException(
                    httpResponse.code(),
                    bodyAsJsonObject.get("message").getAsString()
                );
            }
          //This is for CaptureMeta, where the endpoint doesn't return the id
            else if (!bodyAsJsonObject.has("id")
                && bodyAsJsonObject.has("message")
                && httpResponse.isSuccessful()
            ) {
                return new JsonResponse(
                    0,
                    bodyAsJsonObject.get("message").getAsString(),
                    httpResponse.code()
                );
            } else {
                return new JsonResponse(
                    bodyAsJsonObject.get("id").getAsInt(),
                    bodyAsJsonObject.get("message").getAsString(),
                    httpResponse.code()
                );
            }
        } catch (ResponseException responseException) {
            throw responseException;
        } catch (IllegalStateException | IOException exception) {
            try {
                throw new ResponseException(httpResponse.code(), httpResponse.body().string());
            } catch (IOException e) {
                throw new ResponseException(e);
            }
        }
    } else {
        throw new ResponseException(httpResponse.code(), "Response body is null");
    }
}

JsonResponse class:

public class JsonResponse {
    public final int id;
    public final String message;
    public final int statusCode;
    private final boolean success;

    public JsonResponse(int id, String) {
        this.id = id;
        this.message = message;
        this.statusCode = statusCode;
        this.success = statusCode >= 200 && statusCode <= 299;
    }

    public boolean isSuccessful() {
        return this.success;
    }
    ...
}
kortemik commented 6 months ago

using zero as a placeholder value is a bad practice because one can not distinguish between zero id and no-id-at-all, thus using zero as a magick number should be avoided.

please create separate classes or refactor the use case for the class so that zero or any other magick number is not necessary.

MoonBow-1 commented 6 months ago

Using another method in ApiResponse, "toCaptureMetaJsonResponse()" didn't work:

Solution: Client.sendRequest() method returns ApiResponse and the parsing is left to the send method that called it:

public ApiResponse sendRequest(final JsonRequest request) throws JsonParseException, ResponseException {
    try (final Response response = this.client.newCall(request.request).execute()) {
        final ApiResponseJsonBody responseJsonBody = new ApiResponseJsonBody(response);

        if (response.code() <= 200 || response.code() <= 299) {
            return new ApiResponse(responseJsonBody, response.code());
        } else {
            throw new ResponseException(response.code(), responseJsonBody.getMessage());
        }
    } catch (IOException | IllegalStateException | JsonParseException exceptionWhenSending) {
        throw new ResponseException(exceptionWhenSending);
    }
}

Example parsing from HostGroup send method:

public void send(final Client client) throws ResponseException {
    Typewriter.request("Sending new HostGroup: " + this);

    final JsonRequest jsonRequest = new JsonRequest(
        client.baseUrl,
        "/host/group",
        this.toJson()
    );

    final ApiResponse apiResponse = client.sendRequest(jsonRequest);
    final JsonResponse jsonResponse = apiResponse.toJsonResponse();

    if (jsonResponse.isSuccessful()) {
        Typewriter.response(jsonResponse);
        id = jsonResponse.id;
    }
}

CaptureMeta send method:

public String send(final Client client) throws ResponseException {
    Typewriter.request("Sending new Meta: " + this);
    final JsonRequest jsonRequest = new JsonRequest(
        client.baseUrl,
        "/file/capture/meta/rule",
        this.toJson()
    );

    final ApiResponse apiResponse = client.sendRequest(jsonRequest);
    final CaptureMetaJsonResponse captureMetaJsonResponse = apiResponse.toCaptureMetaJsonResponse();

    Typewriter.response(captureMetaJsonResponse);

    return captureMetaJsonResponse.message.substring(
        captureMetaJsonResponse.message.lastIndexOf("=") + 2
    );
}

CaptureMeta uses a new JsonResponse class, CaptureMetaJsonResponse, which is JsonResponse class without the ID field:

public final class CaptureMetaJsonResponse {
    public final String message;
    public final int statusCode;
    private final boolean success;

    public CaptureMetaJsonResponse(String message, int statusCode) {
        this.message = message;
        this.statusCode = statusCode;
        this.success = statusCode >= 200 && statusCode <= 299;
    }
    ...
}
MoonBow-1 commented 6 months ago

Changed approved and merged changes to main