From 5271b37593a3b2f00c7271f30c21b09f35e453ef Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 16 Aug 2026 20:25:17 +0000 Subject: [PATCH] fix: evaluate EVENT_CACHE_SUFFIX per request instead of freezing it at first use A dynamic EVENT_CACHE_SUFFIX closure was evaluated once, the first time an event's caching metadata was memoized, and the resulting value was frozen into the memoized dictionary entry for the lifetime of the app. Every later request for that event - regardless of locale, session, slug, or whatever the closure actually reads - reused that first request's value, so different requests could silently share a cache key and serve each other's cached content. Fix: store the closure itself in the memoized entry (never evaluate it during the once-per-app metadata build), and evaluate it on every read via a new resolveCacheSuffix() helper, on a shallow copy of the entry so the memoized original keeps the closure. This is exercised on both the request-start cache-lookup path (RequestService.eventCachingTest() -> HandlerService.getEventMetadataEntry()) and the cache-write path (HandlerService.getHandler() -> getEventCachingMetadata()), which must produce the same cache key from the same closure or a cached response is built under one key and looked up under another - never served. Two things that guarantee is easy to accidentally break, both addressed here: - The two paths must see the same request context. getEventMetadataEntry() and getEventCachingMetadata() now take requestContext as an explicit parameter, threaded from callers that already have it (RequestService.eventCachingTest()'s own arguments.context, HandlerService.getHandler()'s own oRequestContext), instead of a resolveCacheSuffix() implementation reaching for requestService.getContext() on its own - which reads request scope and can auto-create a context if one isn't already there, an unnecessary hazard when the real one was one call away the whole time. - The two paths must see an event handler bean with the same action metadata loaded, since a suffix closure may read eventHandlerBean.getActionMetadata(...). getHandlerBean() never loads metadata itself - only getHandler() does, by constructing a handler instance and reflecting it. When handlerCaching is on, the bean getHandlerBean() returns on the lookup path happens to be the same instance getHandler() already populated on an earlier request, so this is invisible - but with handlerCaching off, getHandlerBean() hands back a fresh, un-reflected bean on every call, so the lookup path's closure would silently see empty metadata while the write path's closure sees the real thing: two different suffixes, two different keys, cached responses never served. Factored the metadata-loading block out of getHandler() into ensureHandlerMetadata() and call it from both paths - it only constructs a handler instance when the bean doesn't already have one to reuse, so this doesn't add a second construction to the already-cached common case. New test-harness/handlers/eventcachingSuffix.cfc fixture (separate from eventcaching.cfc so its handler-global suffix doesn't change the cache keys of every other spec using that handler) exercises: per-request re-evaluation producing distinct keys, lookup/build key parity, that same parity under handlerCaching=false specifically, the static-suffix fast path, and that the dictionary keeps the closure rather than a frozen value. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016kCmPkBvNZZhU6iuDcG4NR --- system/web/services/HandlerService.cfc | 130 +++++++++++++++---- system/web/services/RequestService.cfc | 2 +- test-harness/handlers/eventcachingSuffix.cfc | 30 +++++ tests/specs/integration/EventCachingSpec.cfc | 84 ++++++++++++ 4 files changed, 222 insertions(+), 24 deletions(-) create mode 100644 test-harness/handlers/eventcachingSuffix.cfc diff --git a/system/web/services/HandlerService.cfc b/system/web/services/HandlerService.cfc index f4ba338eb..2660a235a 100644 --- a/system/web/services/HandlerService.cfc +++ b/system/web/services/HandlerService.cfc @@ -167,12 +167,7 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { // method check finalized. // Store metadata in execution bean - if ( !variables.handlerCaching || !arguments.ehBean.isMetadataLoaded() ) { - var md = getMetadata( oEventHandler ) - arguments.ehBean - .setActionMetadata( oEventHandler._actionMetadata( arguments.ehBean.getMethod() ) ) - .setHandlerMetadata( md.keyExists( "annotations" ) ? md.annotations : md ) - } + ensureHandlerMetadata( arguments.ehBean, oEventHandler ) // Are they trying to execute an internal ColdBox method? if ( arguments.ehBean.actionMetadataExists( "cbMethod" ) ) { @@ -189,7 +184,11 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { arguments.ehBean.getFullEvent() EQ oRequestContext.getCurrentEvent() ) { // Get event action caching metadata - var eventDictionaryEntry = getEventCachingMetadata( arguments.ehBean, oEventHandler ); + var eventDictionaryEntry = getEventCachingMetadata( + arguments.ehBean, + oEventHandler, + oRequestContext + ); // Do we need to cache this event's output after it executes?? if ( eventDictionaryEntry.cacheable ) { @@ -570,14 +569,29 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { /** * Get an event string's metadata entry. If not found, then you will get a new metadata entry using the `getNewMDEntry()` method. * - * @targetEvent The event to match for metadata. + * @targetEvent The event to match for metadata. + * @requestContext The request context for the current request, passed through to a closure suffix untouched. */ - struct function getEventMetadataEntry( required targetEvent ){ + struct function getEventMetadataEntry( required targetEvent, required requestContext ){ if ( NOT structKeyExists( variables.eventCacheDictionary, arguments.targetEvent ) ) { return getNewMDEntry() } - return variables.eventCacheDictionary[ arguments.targetEvent ] + var mdEntry = variables.eventCacheDictionary[ arguments.targetEvent ] + + // Fast path: a static suffix needs no per-request work, hand back the memoized entry + if ( isSimpleValue( mdEntry.suffix ) ) { + return mdEntry + } + + // Closure suffix: resolve it for THIS request. getHandlerBean() gives back a bean whose + // action/handler metadata is only guaranteed loaded when handlerCaching lets it reuse a + // previously-executed instance - ensureHandlerMetadata() closes that gap so a closure + // reading eventHandlerBean.getActionMetadata(...) sees the same data the store-side + // getEventCachingMetadata() call already guarantees, regardless of that setting. + var bean = getHandlerBean( arguments.targetEvent ) + ensureHandlerMetadata( bean ) + return resolveCacheSuffix( mdEntry, bean, arguments.requestContext ) } /** @@ -784,12 +798,17 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { /** * Return the event caching metadata for an action execution context. * - * @ehBean The event handler bean - * @oEventHandler The event handler to execute + * @ehBean The event handler bean + * @oEventHandler The event handler to execute + * @requestContext The request context for the current request, passed through to a closure suffix untouched. * * @return strc */ - private struct function getEventCachingMetadata( required ehBean, required oEventHandler ){ + private struct function getEventCachingMetadata( + required ehBean, + required oEventHandler, + required requestContext + ){ var cacheKey = arguments.ehBean.getFullEvent(); // Double lock for race conditions @@ -828,15 +847,12 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { mdEntry.lastModified = arguments.ehBean.getActionMetadata( "lastModified", false ); mdEntry.cacheControl = arguments.ehBean.getActionMetadata( "cacheControl", "" ); - // Handler Event Cache Key Suffix, this is global to the event - if ( - isClosure( arguments.oEventHandler.EVENT_CACHE_SUFFIX ) || - isCustomFunction( arguments.oEventHandler.EVENT_CACHE_SUFFIX ) - ) { - mdEntry.suffix = oEventHandler.EVENT_CACHE_SUFFIX( arguments.ehBean ); - } else { - mdEntry.suffix = arguments.oEventHandler.EVENT_CACHE_SUFFIX; - } + // Handler Event Cache Key Suffix, this is global to the event. + // Stored AS DECLARED: a closure must NOT be evaluated here because this + // entry is memoized for the life of the app, and a request-time value + // (locale, session, slug) would freeze into every later request's cache + // key. resolveCacheSuffix() evaluates it on every read instead. + mdEntry.suffix = arguments.oEventHandler.EVENT_CACHE_SUFFIX; // if the cacheFilter has a length and is a method, then we need to verify and store the resulting closure if ( len( mdEntry.cacheFilter ) ) { @@ -879,7 +895,75 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { } // end if - return variables.eventCacheDictionary[ cacheKey ]; + return resolveCacheSuffix( + variables.eventCacheDictionary[ cacheKey ], + arguments.ehBean, + arguments.requestContext + ); + } + + /** + * Ensure an event handler bean has its action/handler metadata loaded, building and + * reflecting a handler instance only if it doesn't already have one to reuse. + * + * Factored out of getHandler() so the event-caching suffix lookup path + * (getEventMetadataEntry()) can guarantee the same metadata is present on the bean it hands + * to an EVENT_CACHE_SUFFIX closure, whether or not handlerCaching lets getHandlerBean() reuse + * a bean instance that a prior getHandler() call already populated. + * + * @ehBean The event handler bean to load metadata onto + * @oEventHandler An already-built handler instance to reflect, if the caller has one; built via newHandler() otherwise + */ + private function ensureHandlerMetadata( required ehBean, oEventHandler ){ + if ( arguments.ehBean.isMetadataLoaded() ) { + return arguments.ehBean; + } + + var handler = isNull( arguments.oEventHandler ) ? newHandler( arguments.ehBean ) : arguments.oEventHandler; + var md = getMetadata( handler ); + + arguments.ehBean + .setActionMetadata( handler._actionMetadata( arguments.ehBean.getMethod() ) ) + .setHandlerMetadata( md.keyExists( "annotations" ) ? md.annotations : md ); + + return arguments.ehBean; + } + + /** + * Resolve a metadata entry's cache-key suffix for the CURRENT request. + * + * A static string suffix passes the entry through untouched. A closure suffix is + * evaluated now, on a shallow COPY of the entry - the memoized entry keeps the + * closure so every request re-evaluates it (locale, session, slug use cases). + * + * The closure receives ( eventHandlerBean, event ) and runs twice per request + * (serve-side key lookup + store-side key build), so it must be deterministic + * within a request: read request-stable inputs only, never time or randomness, + * and mutate nothing - the same contract the hashed rc and the stored + * cacheFilter closure already have. + * + * @mdEntry The memoized event caching metadata entry + * @ehBean The event handler bean, passed to the closure as its first argument + * @requestContext The request context for the current request, passed to the closure as its second argument + */ + private struct function resolveCacheSuffix( + required struct mdEntry, + required ehBean, + required requestContext + ){ + // We check for isClosure and isCustomFunction for ACF/Lucee/BoxLang compatibility + if ( + !isClosure( arguments.mdEntry.suffix ) && + !isCustomFunction( arguments.mdEntry.suffix ) + ) { + return arguments.mdEntry; + } + + var resolved = structCopy( arguments.mdEntry ); + var suffixUDF = arguments.mdEntry.suffix; + resolved.suffix = suffixUDF( arguments.ehBean, arguments.requestContext ); + + return resolved; } } diff --git a/system/web/services/RequestService.cfc b/system/web/services/RequestService.cfc index d8037a0dc..ae8284278 100644 --- a/system/web/services/RequestService.cfc +++ b/system/web/services/RequestService.cfc @@ -156,7 +156,7 @@ component extends="coldbox.system.web.services.BaseService" { arguments.context.removeEventCacheableEntry() // Get metadata entry for event that's fired. - var eventDictionary = variables.handlerService.getEventMetaDataEntry( currentEvent ) + var eventDictionary = variables.handlerService.getEventMetaDataEntry( currentEvent, arguments.context ) // Verify that it is cacheable, else quit, no need for testing anymore. if ( NOT eventDictionary.cacheable ) { diff --git a/test-harness/handlers/eventcachingSuffix.cfc b/test-harness/handlers/eventcachingSuffix.cfc new file mode 100644 index 000000000..0efcaf842 --- /dev/null +++ b/test-harness/handlers/eventcachingSuffix.cfc @@ -0,0 +1,30 @@ +/** + * Fixture for EVENT_CACHE_SUFFIX closure specs. It lives apart from `eventcaching.cfc` + * because the suffix is handler-global and would change the cache keys of every spec + * that uses that handler. + */ +component output="false" { + + // Evaluated per request: the suffix carries the incoming slug (proves per-request + // re-evaluation) and a custom action annotation read off the bean (proves the bean it + // receives has its action metadata loaded - reading it falls back to "missing" instead + // of "present" if metadata isn't loaded, which is silent otherwise). + this.EVENT_CACHE_SUFFIX = function( eventHandlerBean, event ){ + return arguments.event.getValue( "slug", "none" ) & "-" & arguments.eventHandlerBean.getActionMetadata( + "suffixTag", + "missing" + ) + } + + // cacheInclude="" keeps the rc hash constant, so only the suffix varies the cache key + function index( event, rc, prc ) + cache ="true" + cacheTimeout="10" + cacheInclude="" + suffixTag ="present" + { + prc.data = { when : now() } + return prc.data + } + +} diff --git a/tests/specs/integration/EventCachingSpec.cfc b/tests/specs/integration/EventCachingSpec.cfc index 50a80d2d6..057ec0b6b 100755 --- a/tests/specs/integration/EventCachingSpec.cfc +++ b/tests/specs/integration/EventCachingSpec.cfc @@ -495,6 +495,90 @@ expect( data2 ).notToBe( data ); } ); } ); + + describe( "EVENT_CACHE_SUFFIX", function(){ + it( "evaluates a closure suffix on every request producing distinct cache keys", function(){ + getRequestContext().setValue( "slug", "alpha" ) + var event1 = execute( event = "eventcachingSuffix.index", renderResults = true ) + var key1 = event1.getPrivateCollection().cbox_eventCacheableEntry.cacheKey + + expect( key1 ).toInclude( "alpha-present" ) + + // reset to simulate another request with a different slug + setup() + getRequestContext().setValue( "slug", "beta" ) + var event2 = execute( event = "eventcachingSuffix.index", renderResults = true ) + var key2 = event2.getPrivateCollection().cbox_eventCacheableEntry.cacheKey + + // the closure must re-evaluate per request, not freeze on the first request's value + expect( key2 ).toInclude( "beta-present" ) + expect( key2 ).notToBe( key1 ) + } ); + + it( "produces the same key on the serve-side lookup and the store-side build", function(){ + getRequestContext().setValue( "slug", "gamma" ) + var event = execute( event = "eventcachingSuffix.index", renderResults = true ) + var storeKey = event.getPrivateCollection().cbox_eventCacheableEntry.cacheKey + + // re-run the real serve-side path: getEventMetadataEntry() -> buildEventKey() + controller.getRequestService().eventCachingTest( event ) + var serveKey = event.getPrivateCollection().cbox_eventCacheableEntry.cacheKey + + // if lookup and storage keys disagree, cached responses are never served + expect( serveKey ).toBe( storeKey ) + } ); + + it( "keeps the serve-side and store-side keys in sync even with handlerCaching off", function(){ + var handlerService = controller.getHandlerService() + handlerService.setHandlerCaching( false ) + + try { + getRequestContext().setValue( "slug", "delta" ) + var event = execute( event = "eventcachingSuffix.index", renderResults = true ) + var storeKey = event.getPrivateCollection().cbox_eventCacheableEntry.cacheKey + + controller.getRequestService().eventCachingTest( event ) + var serveKey = event.getPrivateCollection().cbox_eventCacheableEntry.cacheKey + + // Both keys must resolve the "present" tag, not just agree with each other - + // otherwise a bean with unloaded action metadata on BOTH sides would still + // produce two equal-but-wrong ("delta-missing") keys and this test would miss it. + expect( storeKey ).toInclude( "delta-present" ) + expect( serveKey ).toBe( storeKey ) + } finally { + handlerService.setHandlerCaching( true ) + } + } ); + + it( "leaves static string suffixes untouched when resolving", function(){ + var handlerService = controller.getHandlerService() + makePublic( handlerService, "resolveCacheSuffix" ) + + var mdEntry = { "cacheable" : true, "suffix" : "static" } + var resolved = handlerService.resolveCacheSuffix( + mdEntry, + handlerService.getHandlerBean( "eventcachingSuffix.index" ), + getRequestContext() + ) + + expect( isSimpleValue( resolved.suffix ) ).toBeTrue() + expect( resolved.suffix ).toBe( "static" ) + } ); + + it( "keeps the closure in the memoized dictionary entry after requests", function(){ + getRequestContext().setValue( "slug", "epsilon" ) + execute( event = "eventcachingSuffix.index", renderResults = true ) + + var dictionary = prepareMock( controller.getHandlerService() ).$getProperty( + "eventCacheDictionary", + "variables" + ) + var suffix = dictionary[ "eventcachingSuffix.index" ].suffix + + // the dictionary must keep the closure so later requests can re-evaluate it + expect( isClosure( suffix ) || isCustomFunction( suffix ) ).toBeTrue() + } ); + } ); } ); }