From 01cefd12e09c44a329dd39cc5eba8b979a93fd79 Mon Sep 17 00:00:00 2001 From: David Rogers Date: Mon, 11 Sep 2023 13:53:42 -0500 Subject: [PATCH 1/2] support a per-function `@noCbSwagger` attribute the presence of this attribute prevents the inclusion of the function's associated documentation from ending up in swagger docs --- models/RoutesParser.cfc | 41 +++++++++++++++++++ test-harness/config/Router.cfc | 18 ++++++++ test-harness/handlers/api/v1/Users.cfc | 11 +++++ test-harness/tests/specs/RoutesParserTest.cfc | 13 +++++- 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/models/RoutesParser.cfc b/models/RoutesParser.cfc index cc1c6a5..ececd0d 100644 --- a/models/RoutesParser.cfc +++ b/models/RoutesParser.cfc @@ -284,6 +284,21 @@ component accessors="true" threadsafe singleton { for ( var methodList in actions ) { // handle any delimited method lists for ( var methodName in listToArray( methodList ) ) { + // + // If we have handler component metadata (when do we expect it to be null?), + // and we have function metadata (when do we expect it to be null?), + // and there is an appropriate annotation on the function from that function's metadata, + // then do not expose this function to swagger docs. + // + if ( !isNull( arguments.handlerMetadata ) ) { + var maybeNull_metaData = getFunctionMetadata( handlerMetadata = arguments.handlerMetadata, functionName = lCase( actions[ methodName ] ) ); + if ( !isNull( maybeNull_metaData ) ) { + if ( isAnnotatedWithTruthyNoCbSwaggerAttribute( functionMetadata = maybeNull_metaData ) ) { + continue; + } + } + } + // method not in error methods if ( !arrayFindNoCase( errorMethods, actions[ methodList ] ) ) { // Create new path template @@ -324,6 +339,11 @@ component accessors="true" threadsafe singleton { } } + if ( path.count() == 0 ) { + // if we skipped over every possible verb, there's nothing to place into swagger docs for this route + return; + } + // Strip out any typing placeholders in routes var pathSegments = listToArray( arguments.pathKey, "/" ); var typingParams = [ "numeric", "alpha", "regex:" ]; @@ -592,6 +612,27 @@ component accessors="true" threadsafe singleton { } } + /** + * return true if the metadata represents a function marked "noCbSwagger" + */ + private boolean function isAnnotatedWithTruthyNoCbSwaggerAttribute( required struct functionMetadata ) { + if ( !structKeyExists( functionMetadata, "noCbSwagger" ) ) { + return false; + } + + if ( trim( functionMetadata.noCbSwagger ) == "" ) { + // e.g. `function handler() noCbSwagger { ... }` + return true; + } + + if ( isValid( "boolean", functionMetadata.noCbSwagger ) && functionMetadata.noCbSwagger ) { + // e.g. `function handler() noCbSwagger=true { ... }` + return true; + } + + return false; + } + private void function appendConventionSamples( required string type, required any methodName, diff --git a/test-harness/config/Router.cfc b/test-harness/config/Router.cfc index 2fe2d46..379b1ae 100755 --- a/test-harness/config/Router.cfc +++ b/test-harness/config/Router.cfc @@ -54,6 +54,24 @@ component{ action=defaultAPIActions ); + // Would be included in docs, but function def has @noCbSwagger attribute + addRoute( + pattern='/api/v1/noCbSwagger', + handler='api.v1.Users', + action={ + "GET": "sharedRouteDifferentHTTPMethods_get_shouldNotBeExposed", + "POST": "sharedRouteDifferentHTTPMethods_post_shouldBeExposed" + } + ); + + addRoute( + pattern='/api/v1/noCbSwagger2', + handler='api.v1.Users', + action = { + "GET": "loneRoute_get_shouldNotBeExposed" + } + ); + // @app_routes@ // Conventions-Based Routing diff --git a/test-harness/handlers/api/v1/Users.cfc b/test-harness/handlers/api/v1/Users.cfc index 164225b..2966ad1 100755 --- a/test-harness/handlers/api/v1/Users.cfc +++ b/test-harness/handlers/api/v1/Users.cfc @@ -78,4 +78,15 @@ component displayname="API.v1.Users" { }; } + /** + * @noCbSwagger + */ + function sharedRouteDifferentHTTPMethods_get_shouldNotBeExposed() {} + function sharedRouteDifferentHTTPMethods_post_shouldBeExposed() {} + + /** + * @noCbSwagger + */ + function loneRoute_get_shouldNotBeExposed() {} + } diff --git a/test-harness/tests/specs/RoutesParserTest.cfc b/test-harness/tests/specs/RoutesParserTest.cfc index 2c1e3cd..5672406 100644 --- a/test-harness/tests/specs/RoutesParserTest.cfc +++ b/test-harness/tests/specs/RoutesParserTest.cfc @@ -80,6 +80,10 @@ component expect( isJSON( APIDoc.asJSON() ) ).toBeTrue(); + expect( NormalizedDoc.paths["/api/v1/noCbSwagger"] ).toHaveKey( "post", "post function NOT marked noCbSwagger" ); + expect( NormalizedDoc.paths["/api/v1/noCbSwagger"] ).notToHaveKey( "get", "get function was marked noCbSwagger" ); + expect( NormalizedDoc.paths ).notToHaveKey( "/api/v1/noCbSwagger2", "the lone function handling this was marked noCbSwagger" ); + variables.APIDoc = APIDoc; } ); @@ -134,7 +138,14 @@ component if ( left( route.pattern, len( routePrefix ) ) == routePrefix ) { var translatedPath = swaggerUtil.translatePath( route.pattern ); if ( !len( route.moduleRouting ) ) { - expect( normalizedDoc[ "paths" ] ).toHaveKey( translatedPath ); + if ( translatedPath == "/api/v1/noCbSwagger2" ) { + // nothing + // this could handled more programmatically, but what we are saying here, + // is that the "magic route configured to be discarded in the swagger docs is indeed discarded" + } + else { + expect( normalizedDoc[ "paths" ] ).toHaveKey( translatedPath ); + } } } } From bed7cad7b20518811ea5dd8f7a38b36f74d111ba Mon Sep 17 00:00:00 2001 From: David Rogers Date: Tue, 12 Sep 2023 14:17:27 -0500 Subject: [PATCH 2/2] fixups per code review --- models/RoutesParser.cfc | 38 +++++++++---------- test-harness/handlers/api/v1/Users.cfc | 4 +- test-harness/tests/specs/RoutesParserTest.cfc | 20 ++++++---- 3 files changed, 32 insertions(+), 30 deletions(-) diff --git a/models/RoutesParser.cfc b/models/RoutesParser.cfc index ececd0d..6d6dc01 100644 --- a/models/RoutesParser.cfc +++ b/models/RoutesParser.cfc @@ -291,9 +291,9 @@ component accessors="true" threadsafe singleton { // then do not expose this function to swagger docs. // if ( !isNull( arguments.handlerMetadata ) ) { - var maybeNull_metaData = getFunctionMetadata( handlerMetadata = arguments.handlerMetadata, functionName = lCase( actions[ methodName ] ) ); - if ( !isNull( maybeNull_metaData ) ) { - if ( isAnnotatedWithTruthyNoCbSwaggerAttribute( functionMetadata = maybeNull_metaData ) ) { + var functionMetadata = getFunctionMetadata( handlerMetadata = arguments.handlerMetadata, functionName = lCase( actions[ methodName ] ) ); + if ( !isNull( functionMetadata ) ) { + if ( !isDocumentationEnabled( functionMetadata = functionMetadata ) ) { continue; } } @@ -339,7 +339,7 @@ component accessors="true" threadsafe singleton { } } - if ( path.count() == 0 ) { + if ( structIsEmpty( path ) ) { // if we skipped over every possible verb, there's nothing to place into swagger docs for this route return; } @@ -613,24 +613,20 @@ component accessors="true" threadsafe singleton { } /** - * return true if the metadata represents a function marked "noCbSwagger" + * return true if the supplied metadata represents a function that has NOT been explicitly marked as disabled */ - private boolean function isAnnotatedWithTruthyNoCbSwaggerAttribute( required struct functionMetadata ) { - if ( !structKeyExists( functionMetadata, "noCbSwagger" ) ) { - return false; - } - - if ( trim( functionMetadata.noCbSwagger ) == "" ) { - // e.g. `function handler() noCbSwagger { ... }` - return true; - } - - if ( isValid( "boolean", functionMetadata.noCbSwagger ) && functionMetadata.noCbSwagger ) { - // e.g. `function handler() noCbSwagger=true { ... }` - return true; - } - - return false; + private boolean function isDocumentationEnabled( required struct functionMetadata ) { + // + // function foo() {} <-- no attr, documentation is enabled (depending on module config) + // function foo() cbSwagger {} <-- has attr with no value, documentation is enabled (depending on module config) + // function foo() cbSwagger=true {} <-- has attr, with truthy value, documentation is enabled (depending on module config) + // function foo() cbSwagger=false {} <-- has attr, with falsy value, documentation is disabled (overrides module config) + // function foo() cbSwagger=xyz {} <-- has attr, with non-booleanish value, which we consider falsy, documentation is disabled (overrides module config) + // + return !structKeyExists( functionMetadata, "cbSwagger" ) + ? true // there is no `cbSwagger` attribute, so the default is to assume true. + // booleanish test first, to handle docbloc attrs that have no associated value being assigned `true` values + : (isValid("boolean", functionMetadata.cbSwagger) && functionMetadata.cbSwagger) || len( functionMetadata.cbSwagger ) == 0; } private void function appendConventionSamples( diff --git a/test-harness/handlers/api/v1/Users.cfc b/test-harness/handlers/api/v1/Users.cfc index 2966ad1..3792290 100755 --- a/test-harness/handlers/api/v1/Users.cfc +++ b/test-harness/handlers/api/v1/Users.cfc @@ -79,13 +79,13 @@ component displayname="API.v1.Users" { } /** - * @noCbSwagger + * @cbSwagger false */ function sharedRouteDifferentHTTPMethods_get_shouldNotBeExposed() {} function sharedRouteDifferentHTTPMethods_post_shouldBeExposed() {} /** - * @noCbSwagger + * @cbSwagger false */ function loneRoute_get_shouldNotBeExposed() {} diff --git a/test-harness/tests/specs/RoutesParserTest.cfc b/test-harness/tests/specs/RoutesParserTest.cfc index 5672406..aea56d5 100644 --- a/test-harness/tests/specs/RoutesParserTest.cfc +++ b/test-harness/tests/specs/RoutesParserTest.cfc @@ -80,9 +80,9 @@ component expect( isJSON( APIDoc.asJSON() ) ).toBeTrue(); - expect( NormalizedDoc.paths["/api/v1/noCbSwagger"] ).toHaveKey( "post", "post function NOT marked noCbSwagger" ); - expect( NormalizedDoc.paths["/api/v1/noCbSwagger"] ).notToHaveKey( "get", "get function was marked noCbSwagger" ); - expect( NormalizedDoc.paths ).notToHaveKey( "/api/v1/noCbSwagger2", "the lone function handling this was marked noCbSwagger" ); + expect( NormalizedDoc.paths["/api/v1/noCbSwagger"] ).toHaveKey( "post", "post function marked cbSwagger=false" ); + expect( NormalizedDoc.paths["/api/v1/noCbSwagger"] ).notToHaveKey( "get", "get function was not marked cbSwagger=false" ); + expect( NormalizedDoc.paths ).notToHaveKey( "/api/v1/noCbSwagger2", "the lone function handling this was marked cbSwagger=false" ); variables.APIDoc = APIDoc; } ); @@ -132,16 +132,20 @@ component expect( arrayLen( CBRoutes ) ).toBeGT( 0 ); + // in cases where we are testing that some route does not end up in the docs, + // we want to ensure we DO see that the route exists, and consequently that its absence from the docs + // is due to an intentional exclusion of a route that positively exists. + var requireTheseRoutesExistWithoutDocs = {"/api/v1/noCbSwagger2": 1} + // Tests that all of our configured paths exist for ( var routePrefix in apiPrefixes ) { for ( var route in cbRoutes ) { if ( left( route.pattern, len( routePrefix ) ) == routePrefix ) { var translatedPath = swaggerUtil.translatePath( route.pattern ); if ( !len( route.moduleRouting ) ) { - if ( translatedPath == "/api/v1/noCbSwagger2" ) { - // nothing - // this could handled more programmatically, but what we are saying here, - // is that the "magic route configured to be discarded in the swagger docs is indeed discarded" + if ( structKeyExists(requireTheseRoutesExistWithoutDocs, translatedPath) ) { + expect( normalizedDoc[ "paths" ] ).notToHaveKey( translatedPath, "expected to have discarded this route from the docs" ); + structDelete( requireTheseRoutesExistWithoutDocs, translatedPath ) } else { expect( normalizedDoc[ "paths" ] ).toHaveKey( translatedPath ); @@ -150,6 +154,8 @@ component } } } + + expect( requireTheseRoutesExistWithoutDocs ).toBeEmpty( "all routes expected to not have docs were seen" ); } ); it( "Tests the API Document for module introspection", function(){