From 67f673e6c64cd8ff079945b75c705e638e9022c8 Mon Sep 17 00:00:00 2001 From: Andreas Fankhauser <23085769+hiddenalpha@users.noreply.github.com> Date: Wed, 28 Jun 2023 17:22:52 +0200 Subject: [PATCH] Cleanup random findings - Add error handling. - Prevent millions of nonsense string comparions where a simple int compare is enough. - Shorten useless complicated messages. Cache result of toString(). - Replace ugly imports. References (www): - https://m.youtube.com/watch?v=x2EOOJg8FkA - https://m.youtube.com/watch?v=EpYr3T5VP6w - https://medium.com/swlh/yagni-and-dry-the-kiss-of-death-for-your-software-project-cfd44b0654b6#fc82 References (I'm not allowed to make those publicly available): - https://wikit.post.ch/x/_Bv6Rw - https://wikit.post.ch/x/iRepPQ - https://jira.post.ch/browse/SDCISA-10871 (wip @ 284d263027f32af8980e83eaf01042aa4ebb483e) --- .../gateleen/expansion/ExpansionHandler.java | 32 ++++++-- .../swisspush/gateleen/hook/HookHandler.java | 77 +++++-------------- 2 files changed, 45 insertions(+), 64 deletions(-) diff --git a/gateleen-expansion/src/main/java/org/swisspush/gateleen/expansion/ExpansionHandler.java b/gateleen-expansion/src/main/java/org/swisspush/gateleen/expansion/ExpansionHandler.java index a75f5c39..53546d9f 100755 --- a/gateleen-expansion/src/main/java/org/swisspush/gateleen/expansion/ExpansionHandler.java +++ b/gateleen-expansion/src/main/java/org/swisspush/gateleen/expansion/ExpansionHandler.java @@ -5,16 +5,24 @@ import io.vertx.core.MultiMap; import io.vertx.core.Vertx; import io.vertx.core.buffer.Buffer; -import io.vertx.core.http.*; +import io.vertx.core.http.HttpClient; +import io.vertx.core.http.HttpClientRequest; +import io.vertx.core.http.HttpClientResponse; +import io.vertx.core.http.HttpMethod; +import io.vertx.core.http.HttpServerRequest; import io.vertx.core.json.JsonArray; import io.vertx.core.json.JsonObject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.swisspush.gateleen.core.http.RequestLoggerFactory; import org.swisspush.gateleen.core.storage.ResourceStorage; -import org.swisspush.gateleen.core.util.*; +import org.swisspush.gateleen.core.util.ExpansionDeltaUtil; import org.swisspush.gateleen.core.util.ExpansionDeltaUtil.CollectionResourceContainer; import org.swisspush.gateleen.core.util.ExpansionDeltaUtil.SlashHandling; +import org.swisspush.gateleen.core.util.HttpServerRequestUtil; +import org.swisspush.gateleen.core.util.ResourceCollectionException; +import org.swisspush.gateleen.core.util.ResponseStatusCodeLogUtil; +import org.swisspush.gateleen.core.util.StatusCode; import org.swisspush.gateleen.routing.Rule; import org.swisspush.gateleen.routing.RuleFeaturesProvider; import org.swisspush.gateleen.routing.RuleProvider; @@ -25,6 +33,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; +import static org.swisspush.gateleen.core.util.StatusCode.INTERNAL_SERVER_ERROR; import static org.swisspush.gateleen.routing.RuleFeatures.Feature.EXPAND_ON_BACKEND; import static org.swisspush.gateleen.routing.RuleFeatures.Feature.STORAGE_EXPAND; import static org.swisspush.gateleen.routing.RuleProvider.RuleChangesObserver; @@ -491,7 +500,9 @@ private void removeZipParameter(final HttpServerRequest request) { private void makeStorageExpandRequest(final String targetUri, final List subResourceNames, final HttpServerRequest req, final DeltaHandler handler) { Logger log = RequestLoggerFactory.getLogger(ExpansionHandler.class, req); - httpClient.request(HttpMethod.POST, targetUri + "?storageExpand=true").onComplete(asyncResult -> { + HttpMethod reqMethod = HttpMethod.POST; + String reqUri = targetUri + "?storageExpand=true"; + httpClient.request(reqMethod, reqUri, asyncResult -> { if (asyncResult.failed()) { log.warn("Failed request to {}: {}", targetUri + "?storageExpand=true", asyncResult.cause()); return; @@ -512,16 +523,23 @@ private void makeStorageExpandRequest(final String targetUri, final List subReso cReq.write(payload); cReq.send(event -> { + if (event.failed()) { + Throwable ex = event.cause(); + log.debug("{} {}", reqMethod, reqUri, ex); + var exWrappr = new ResourceCollectionException(ex.getMessage(), INTERNAL_SERVER_ERROR); + handler.handle(new ResourceNode(SERIOUS_EXCEPTION, exWrappr)); + } HttpClientResponse cRes = event.result(); cRes.bodyHandler(data -> { if (StatusCode.NOT_FOUND.getStatusCode() == cRes.statusCode()) { - log.debug("requested resource could not be found: {}", targetUri); + log.debug("NotFound: {}", targetUri); handler.handle(new ResourceNode(SERIOUS_EXCEPTION, new ResourceCollectionException(cRes.statusMessage(), StatusCode.NOT_FOUND))); } else if (StatusCode.INTERNAL_SERVER_ERROR.getStatusCode() == cRes.statusCode()) { - log.error("error in request resource : {} message : {}", targetUri, data.toString()); - handler.handle(new ResourceNode(SERIOUS_EXCEPTION, new ResourceCollectionException(data.toString(), StatusCode.INTERNAL_SERVER_ERROR))); + String fullResponseBody = data.toString(); + log.error("{}: {}: {}", INTERNAL_SERVER_ERROR, targetUri, fullResponseBody); + handler.handle(new ResourceNode(SERIOUS_EXCEPTION, new ResourceCollectionException(fullResponseBody, StatusCode.INTERNAL_SERVER_ERROR))); } else if (StatusCode.METHOD_NOT_ALLOWED.getStatusCode() == cRes.statusCode()) { - log.error("POST requests (storageExpand) not allowed for uri: {}", targetUri); + log.error("storageExpand not allowed for: {}", targetUri); handler.handle(new ResourceNode(SERIOUS_EXCEPTION, new ResourceCollectionException(cRes.statusMessage(), StatusCode.METHOD_NOT_ALLOWED))); } else { String eTag = geteTag(cRes.headers()); diff --git a/gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java b/gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java index d38c1c9a..464be82f 100755 --- a/gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java +++ b/gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java @@ -47,6 +47,8 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; +import static io.vertx.core.http.HttpMethod.DELETE; +import static io.vertx.core.http.HttpMethod.PUT; import static org.swisspush.gateleen.core.util.HttpRequestHeader.CONTENT_LENGTH; /** @@ -502,24 +504,25 @@ public boolean handle(final RoutingContext ctx) { /* * 1) Un- / Register Listener / Routes */ - if (isHookListenerRegistration(request)) { - handleListenerRegistration(request); - return true; - } - - if (isHookListenerUnregistration(request)) { - handleListenerUnregistration(request); - return true; - } - - if (isHookRouteRegistration(request)) { - handleRouteRegistration(request); - return true; + if (request.method() == PUT) { + if (request.uri().contains(HOOKS_LISTENERS_URI_PART)) { + handleListenerRegistration(request); + return true; + } + if (request.uri().contains(HOOKS_ROUTE_URI_PART)) { + handleRouteRegistration(request); + return true; + } } - - if (isHookRouteUnregistration(request)) { - handleRouteUnregistration(request); - return true; + if (request.method() == DELETE) { + if (request.uri().contains(HOOKS_LISTENERS_URI_PART)) { + handleListenerUnregistration(request); + return true; + } + if (request.uri().contains(HOOKS_ROUTE_URI_PART)) { + handleRouteUnregistration(request); + return true; + } } /* @@ -1654,46 +1657,6 @@ private String getListenerUrlSegment(String requestUrl) { return requestUrl.substring(pos + HOOKS_LISTENERS_URI_PART.length()); } - /** - * Checks if the given request is a listener unregistration instruction. - * - * @param request request - * @return boolean - */ - private boolean isHookListenerUnregistration(HttpServerRequest request) { - return request.uri().contains(HOOKS_LISTENERS_URI_PART) && HttpMethod.DELETE == request.method(); - } - - /** - * Checks if the given request is a listener registration instruction. - * - * @param request request - * @return boolean - */ - private boolean isHookListenerRegistration(HttpServerRequest request) { - return request.uri().contains(HOOKS_LISTENERS_URI_PART) && HttpMethod.PUT == request.method(); - } - - /** - * Checks if the given request is a route registration instruction. - * - * @param request request - * @return boolean - */ - private boolean isHookRouteRegistration(HttpServerRequest request) { - return request.uri().contains(HOOKS_ROUTE_URI_PART) && HttpMethod.PUT == request.method(); - } - - /** - * Checks if the given request is a route registration instruction. - * - * @param request request - * @return boolean - */ - private boolean isHookRouteUnregistration(HttpServerRequest request) { - return request.uri().contains(HOOKS_ROUTE_URI_PART) && HttpMethod.DELETE == request.method(); - } - /** * @param request Request to extract the value from. This instance gets manipulated * internally during call.