diff --git a/AGENTS.md b/AGENTS.md index 8eb04b094..8a3e24df2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,6 +23,15 @@ ColdBox is an HMVC (Hierarchical Model-View-Controller) framework designed for t - Do not start a docblock line with `@` in an example, because BoxLang reads it as documentation metadata. - Prefer the elvis operator `?:` and safe navigation `?.` over `structKeyExists()` and `isNull()` checks when they say the same thing. +### Hot Paths +Code that runs on every request must be as fast as possible: event dispatch in `HandlerService` and `Controller`, routing, rendering and view lookups, interceptor announcements, WireBox `getInstance()`, and `RequestContext` getters. +- Look things up in a struct, with `structKeyExists()` or `keyExists()`. Don't search a list or an array (`listFind()`, `listFindNoCase()`, `arrayFind()`, `arrayContains()`). +- Don't use regular expressions. Use `left()`, `right()`, `mid()`, `find()` or `compare()`, or a lookup struct. +- Build lookup structs once, in `init()` or `onConfigurationLoad()`, not on every call. +- Put the cheapest check first, so the common case returns early. +- Remember results that don't change between requests in a struct, gated by the matching setting (`handlerCaching`, `viewDiscoveryCaching`) like the existing caches. +- Don't read metadata (`getMetadata()`), serialize (`serializeJSON()`) or hash on every call. + ## JavaScript Coding Standards ### Spacing and Formatting diff --git a/system/web/services/HandlerService.cfc b/system/web/services/HandlerService.cfc index 1e1d53cc1..b6d6a9513 100644 --- a/system/web/services/HandlerService.cfc +++ b/system/web/services/HandlerService.cfc @@ -63,6 +63,12 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { // Default registries variables.registeredHandlers = {} variables.registeredExternalHandlers = {} + // Handler-level advice methods, a struct for fast existence checks on every event + variables.ADVICE_METHODS = { + "preHandler" : true, + "postHandler" : true, + "aroundHandler" : true + } return this } @@ -175,6 +181,13 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { return processInvalidEvent( arguments.ehBean, oRequestContext ) } + // Are they requesting an advice method (preHandler, preIndex, etc) as an action? + // The Controller runs advice by calling the methods directly, so they never come through here. + if ( isAdviceAction( arguments.ehBean.getMethod(), oEventHandler ) ) { + // Invalid Event processing + return processInvalidEvent( arguments.ehBean, oRequestContext ) + } + /* ::::::::::::::::::::::::::::::::::::::::: EVENT CACHING :::::::::::::::::::::::::::::::::::::::::::: */ // Event Caching Routines, if using caching, NOT a private event and we are executing the main event @@ -755,6 +768,46 @@ component extends="coldbox.system.web.services.BaseService" accessors="true" { return getHandler( getHandlerBean( targetInvalidEvent ), arguments.requestContext ); } + /** + * Verify if an action is one of the handler's advice methods: preHandler(), postHandler(), aroundHandler(), + * or the pre/post/around advice of another action in the same handler. This is the same rule the + * Controller uses to find the advice it runs around an action. Inherited framework methods tagged + * cbMethod, such as view(), can never run as actions, so preview() next to them is a normal action. + * + * This runs for every event, so it only uses struct lookups and plain string checks. An action + * without a pre, post or around prefix, which is almost every action, returns without any metadata lookup. + * + * @action The requested action + * @oEventHandler The event handler instance + */ + private boolean function isAdviceAction( required string action, required oEventHandler ){ + if ( structKeyExists( variables.ADVICE_METHODS, arguments.action ) ) { + return true + } + + // pre{Action}, post{Action} or around{Action}: the action it would advise + var adviceTarget = "" + if ( left( arguments.action, 3 ) == "pre" ) { + adviceTarget = mid( arguments.action, 4, len( arguments.action ) ) + } else if ( left( arguments.action, 4 ) == "post" ) { + adviceTarget = mid( arguments.action, 5, len( arguments.action ) ) + } else if ( left( arguments.action, 6 ) == "around" ) { + adviceTarget = mid( arguments.action, 7, len( arguments.action ) ) + } + if ( !len( adviceTarget ) ) { + return false + } + + // Only for an action with an advice prefix: does the advised action exist? + if ( !arguments.oEventHandler._actionExists( adviceTarget ) ) { + return false + } + // The target must be an action, not an inherited framework method + var targetMetadata = arguments.oEventHandler._actionMetadata( adviceTarget ) + var annotations = targetMetadata.keyExists( "annotations" ) ? targetMetadata.annotations : targetMetadata + return !annotations.keyExists( "cbMethod" ) + } + /** * Verifies setup of base handler classes in WireBox * diff --git a/test-harness/handlers/adviceDispatch.cfc b/test-harness/handlers/adviceDispatch.cfc new file mode 100644 index 000000000..cf4b819af --- /dev/null +++ b/test-harness/handlers/adviceDispatch.cfc @@ -0,0 +1,83 @@ +/** + * Advice methods that must not be requestable as actions, next to normal actions. + * Every method records itself in prc.ran so the specs can tell what executed. + * Tested by tests/specs/integration/AdviceDispatchSpec.cfc + */ +component { + + /************************************** ADVICE *********************************************/ + + function preHandler( + event, + rc, + prc, + action = "", + eventArguments = {} + ){ + // The framework passes the action when it runs this as advice + record( prc, len( arguments.action ) ? "preHandler" : "preHandler:action" ) + } + + function postHandler( + event, + rc, + prc, + action = "", + eventArguments = {} + ){ + record( prc, "postHandler" ) + } + + function aroundHandler( + event, + rc, + prc, + targetAction, + eventArguments = {} + ){ + record( prc, "aroundHandler" ) + return arguments.targetAction( + event = arguments.event, + rc = arguments.rc, + prc = arguments.prc + ) + } + + function preIndex( event, rc, prc ){ + record( prc, "preIndex" ) + } + + // Advice for view(): the framework runs it before every view() request + function preview( event, rc, prc ){ + record( prc, "preview" ) + } + + /************************************** ACTIONS *********************************************/ + + function index( event, rc, prc ){ + record( prc, "index" ) + event.renderData( type = "plain", data = "ADVICE-INDEX-OK" ) + } + + function view( event, rc, prc ){ + record( prc, "view" ) + event.renderData( type = "plain", data = "ADVICE-VIEW-OK" ) + } + + // Starts with "pre", but there is no flight() action, so this is a normal action + function preflight( event, rc, prc ){ + record( prc, "preflight" ) + event.renderData( type = "plain", data = "ADVICE-PREFLIGHT-OK" ) + } + + + /************************************** PRIVATE *********************************************/ + + private function record( required prc, required name ){ + if ( !arguments.prc.keyExists( "ran" ) ) { + arguments.prc.ran = [] + } + arguments.prc.ran.append( arguments.name ) + } + +} diff --git a/test-harness/handlers/adviceInherited.cfc b/test-harness/handlers/adviceInherited.cfc new file mode 100644 index 000000000..479aa9e39 --- /dev/null +++ b/test-harness/handlers/adviceInherited.cfc @@ -0,0 +1,12 @@ +/** + * preview() with no view() action of its own: the inherited framework view() helper is not an action, + * so preview() is a normal action, not advice. + * Tested by tests/specs/integration/AdviceDispatchSpec.cfc + */ +component { + + function preview( event, rc, prc ){ + event.renderData( type = "plain", data = "INHERITED-PREVIEW-OK" ) + } + +} diff --git a/tests/specs/integration/AdviceDispatchSpec.cfc b/tests/specs/integration/AdviceDispatchSpec.cfc new file mode 100644 index 000000000..f62959d56 --- /dev/null +++ b/tests/specs/integration/AdviceDispatchSpec.cfc @@ -0,0 +1,86 @@ +/** + * Advice methods (preHandler, postHandler, aroundHandler, pre/post/around{Action}) can't be requested as the action. + * Fixture: test-harness/handlers/adviceDispatch.cfc + */ +component extends="tests.resources.BaseIntegrationTest" { + + function run(){ + describe( "Advice methods requested as actions", function(){ + beforeEach( function( currentSpec ){ + // Setup as a new ColdBox request + setup() + } ) + + var adviceEvents = [ + "adviceDispatch.preHandler", + "adviceDispatch.postHandler", + "adviceDispatch.aroundHandler", + "adviceDispatch.preIndex", + "adviceDispatch.PREINDEX", + "adviceDispatch.preview" + ] + + for ( var adviceEvent in adviceEvents ) { + it( + title = "rejects #adviceEvent# as an invalid event", + data = { event : adviceEvent }, + body = function( data ){ + var e = execute( event = data.event, renderResults = true ) + var prc = e.getPrivateCollection() + + expect( prc.invalidevent ?: "" ).toBe( data.event ) + expect( e.getStatusCode() ).toBe( 404 ) + expect( e.getRenderedContent() ).toInclude( "Invalid Page" ) + // Nothing in the advice handler ran + expect( prc.keyExists( "ran" ) ).toBeFalse() + } + ) + } + + it( "runs a normal action with its automatic advice", function(){ + var e = execute( event = "adviceDispatch.index", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-INDEX-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,preIndex,aroundHandler,index,postHandler" ) + } ) + + it( "still runs preview() as advice for view()", function(){ + var e = execute( event = "adviceDispatch.view", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-VIEW-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,preview,aroundHandler,view,postHandler" ) + } ) + + it( "runs an action that starts with pre but advises no action", function(){ + var e = execute( event = "adviceDispatch.preflight", renderResults = true ) + + expect( e.getRenderedContent() ).toInclude( "ADVICE-PREFLIGHT-OK" ) + expect( e.getPrivateValue( "ran" ).toList() ).toBe( "preHandler,aroundHandler,preflight,postHandler" ) + } ) + + it( "runs preview() as an action when view() is only the inherited framework helper", function(){ + var e = execute( event = "adviceInherited.preview", renderResults = true ) + + expect( e.getPrivateValue( "invalidevent", "" ) ).toBe( "" ) + expect( e.getRenderedContent() ).toInclude( "INHERITED-PREVIEW-OK" ) + } ) + + it( "rejects an advice method requested with runEvent(), like a missing action", function(){ + // runEvent() of an action that doesn't exist throws, and so does an advice method now + expect( function(){ + getController().runEvent( event = "adviceDispatch.preHandler" ) + } ).toThrow() + expect( getRequestContext().getPrivateValue( "invalidevent", "" ) ).toBe( "adviceDispatch.preHandler" ) + expect( getRequestContext().getPrivateValue( "ran", [] ) ).notToInclude( "preHandler:action" ) + } ) + + it( "rejects an advice method requested as a private event", function(){ + var e = execute( event = "adviceDispatch.preHandler", private = true ) + + expect( e.getPrivateValue( "invalidevent", "" ) ).toBe( "adviceDispatch.preHandler" ) + expect( e.getPrivateValue( "ran", [] ) ).notToInclude( "preHandler:action" ) + } ) + } ) + } + +}