From 236557c9e7bc2e455614fc6c73ea508ac3df3857 Mon Sep 17 00:00:00 2001 From: Tim Klemm Date: Tue, 7 Jan 2025 09:02:15 -0500 Subject: [PATCH] Additional review comments. --- esp/bindings/http/platform/httpservice.cpp | 157 +++++++++++---------- 1 file changed, 82 insertions(+), 75 deletions(-) diff --git a/esp/bindings/http/platform/httpservice.cpp b/esp/bindings/http/platform/httpservice.cpp index 98e6920cd6a..73d400f6cd4 100644 --- a/esp/bindings/http/platform/httpservice.cpp +++ b/esp/bindings/http/platform/httpservice.cpp @@ -299,10 +299,12 @@ void CEspHttpServer::traceRequest(IEspContext* ctx, const char* normalizeMethod) span->setSpanAttribute("url.full", full); } -// Enumeration of "esp" service methods that are never traced. Add new values as additional -// requests become relevant. -enum class UntracedGetMethod +// Enumeration of "esp" service methods. These are generally requests for files or form markup, in +// other words, they are web service overhead not specific to any ESP instance. Add new values as +// additional requests become relevant. +enum class EspGetMethod { + None, // empty name Files, Xslt, Body, @@ -312,35 +314,42 @@ enum class UntracedGetMethod NavData, NavMenuEvent, SoapReq, + // DO NOT MAP NAMES TO THE FOLLOWING VALUES: + Unhandled, // catch-all for any method name not explicitly handled by processRequest + NotApplicable, // request not associated with the "esp" service }; -struct UntracedGetMethodNameComparator +struct EspGetMethodNameComparator { bool operator()(const char* lhs, const char* rhs) const { return stricmp(lhs, rhs) < 0; } }; -using UntracedGetMethodMap = std::map; -// Association of method name to a specific "esp" request that will not be traced. Multiple names -// may map to the same request, and all redundant names (e.g., "files" and "files_") must be in the -// map. Tracing will be suppressed for all method names in the map. -static const UntracedGetMethodMap getRequests{ - {"files", UntracedGetMethod::Files}, - {"xslt", UntracedGetMethod::Xslt}, - {"body", UntracedGetMethod::Body}, - {"frame", UntracedGetMethod::Frame}, - {"titlebar", UntracedGetMethod::TitleBar}, - {"nav", UntracedGetMethod::Nav}, - {"navdata", UntracedGetMethod::NavData}, - {"navmenuevent", UntracedGetMethod::NavMenuEvent}, - {"soapreq", UntracedGetMethod::SoapReq}, +using EspGetMethodMap = std::map; +// Association of method names to specific "esp" service requests. This mapping allows +// pprocessRequest to decide if the request should be traced before processing the request, without +// repeating the method name string comparisons. Multiple names may map to the same request, and +// all redundant names (e.g., "files" and "files_") must be included in the map. +static const EspGetMethodMap getRequests{ + // esp + {"", EspGetMethod::None}, + // esp/ + {"files", EspGetMethod::Files}, + {"xslt", EspGetMethod::Xslt}, + {"body", EspGetMethod::Body}, + {"frame", EspGetMethod::Frame}, + {"titlebar", EspGetMethod::TitleBar}, + {"nav", EspGetMethod::Nav}, + {"navdata", EspGetMethod::NavData}, + {"navmenuevent", EspGetMethod::NavMenuEvent}, + {"soapreq", EspGetMethod::SoapReq}, // same as above but with trailing underscore - {"files_", UntracedGetMethod::Files}, - {"xslt_", UntracedGetMethod::Xslt}, - {"body_", UntracedGetMethod::Body}, - {"frame_", UntracedGetMethod::Frame}, - {"titlebar_", UntracedGetMethod::TitleBar}, - {"nav_", UntracedGetMethod::Nav}, - {"navdata_", UntracedGetMethod::NavData}, - {"navmenuevent_", UntracedGetMethod::NavMenuEvent}, - {"soapreq_", UntracedGetMethod::SoapReq}, + {"files_", EspGetMethod::Files}, + {"xslt_", EspGetMethod::Xslt}, + {"body_", EspGetMethod::Body}, + {"frame_", EspGetMethod::Frame}, + {"titlebar_", EspGetMethod::TitleBar}, + {"nav_", EspGetMethod::Nav}, + {"navdata_", EspGetMethod::NavData}, + {"navmenuevent_", EspGetMethod::NavMenuEvent}, + {"soapreq_", EspGetMethod::SoapReq}, }; int CEspHttpServer::processRequest() @@ -402,29 +411,30 @@ int CEspHttpServer::processRequest() // so maximize the amount of request processing that can be traced. Specifically, user // authentication and authorization may generate trace output. bool wantTracing = true; - UntracedGetMethodMap::const_iterator untracedRequestIt = getRequests.end(); - if (!queryTraceManager().isTracingEnabled()) - wantTracing = false; - else if (streq(method, GET_METHOD)) + EspGetMethod espGetMethod = EspGetMethod::NotApplicable; + if (streq(method, GET_METHOD)) { if (sub_serv_root == stype) wantTracing = false; else if (strieq(serviceName, "esp")) { - if (methodName.isEmpty()) - wantTracing = false; - else + // At this time, the presence of a method name in the get request map is sufficient + // to suppress trace output. The mapped value will be used later. + EspGetMethodMap::const_iterator it = getRequests.find(methodName); + if (it != getRequests.end()) { - // The presence of a method name in the get request map is sufficient to - // suppress trace output. The enumerated value will be used later. - untracedRequestIt = getRequests.find(methodName); - if (untracedRequestIt != getRequests.end()) - wantTracing = false; + espGetMethod = it->second; + wantTracing = false; } + else + espGetMethod = EspGetMethod::Unhandled; } } else if (!m_apport) wantTracing = false; + // Check last to prevent necessary side effects of other checks from occurring. + else if (!queryTraceManager().isTracingEnabled()) + wantTracing = false; Owned serverSpan; if (wantTracing) { @@ -481,43 +491,40 @@ int CEspHttpServer::processRequest() return onGetApplicationFrame(m_request.get(), m_response.get(), ctx); } - if (!stricmp(serviceName.str(), "esp")) + // Use the previously identified method selector to dispatch the request. + switch (espGetMethod) { - if (!methodName.length()) - return 0; - - if (untracedRequestIt != getRequests.end()) - { - // Use the previously identified method selector to dispatch the request. - switch (untracedRequestIt->second) - { - case UntracedGetMethod::Files: - if (!getTxSummaryResourceReq()) - ctx->cancelTxSummary(); - checkInitEclIdeResponse(m_request, m_response); - return onGetFile(m_request.get(), m_response.get(), pathEx.str()); - case UntracedGetMethod::Xslt: - if (!getTxSummaryResourceReq()) - ctx->cancelTxSummary(); - return onGetXslt(m_request.get(), m_response.get(), pathEx.str()); - case UntracedGetMethod::Body: - return onGetMainWindow(m_request.get(), m_response.get()); - case UntracedGetMethod::Frame: - return onGetApplicationFrame(m_request.get(), m_response.get(), ctx); - case UntracedGetMethod::TitleBar: - return onGetTitleBar(m_request.get(), m_response.get()); - case UntracedGetMethod::Nav: - return onGetNavWindow(m_request.get(), m_response.get()); - case UntracedGetMethod::NavData: - return onGetDynNavData(m_request.get(), m_response.get()); - case UntracedGetMethod::NavMenuEvent: - return onGetNavEvent(m_request.get(), m_response.get()); - case UntracedGetMethod::SoapReq: - return onGetBuildSoapRequest(m_request.get(), m_response.get()); - default: - break; - } - } + case EspGetMethod::None: + return 0; + case EspGetMethod::Files: + if (!getTxSummaryResourceReq()) + ctx->cancelTxSummary(); + checkInitEclIdeResponse(m_request, m_response); + return onGetFile(m_request.get(), m_response.get(), pathEx.str()); + case EspGetMethod::Xslt: + if (!getTxSummaryResourceReq()) + ctx->cancelTxSummary(); + return onGetXslt(m_request.get(), m_response.get(), pathEx.str()); + case EspGetMethod::Body: + return onGetMainWindow(m_request.get(), m_response.get()); + case EspGetMethod::Frame: + return onGetApplicationFrame(m_request.get(), m_response.get(), ctx); + case EspGetMethod::TitleBar: + return onGetTitleBar(m_request.get(), m_response.get()); + case EspGetMethod::Nav: + return onGetNavWindow(m_request.get(), m_response.get()); + case EspGetMethod::NavData: + return onGetDynNavData(m_request.get(), m_response.get()); + case EspGetMethod::NavMenuEvent: + return onGetNavEvent(m_request.get(), m_response.get()); + case EspGetMethod::SoapReq: + return onGetBuildSoapRequest(m_request.get(), m_response.get()); + case EspGetMethod::Unhandled: + case EspGetMethod::NotApplicable: + break; + default: + IERRLOG("unexpected EspGetMethod value: %d", (int)espGetMethod); + break; } }