From 7ace23acb4b1ca6fef5054f1bf0de87d79e67bb6 Mon Sep 17 00:00:00 2001 From: bknutson Date: Tue, 14 Jan 2025 15:37:59 -0700 Subject: [PATCH 01/15] [SPN-1438] Guardrail to add deprecated metric for class methods in the codebase --- src/Checks/ErrorConstants.php | 1 + src/Evaluators/FunctionLike.php | 25 +++++++++++- src/NodeVisitors/StaticAnalyzer.php | 7 +++- src/Scope/ScopeStack.php | 6 +++ tests/TestSuiteSetup.php | 38 ++++++++++++++++++- .../Checks/DeprecatedFunctionMetricTest.php | 32 ++++++++++++++++ .../DeprecatedFunctionMetricTest.1.inc | 16 ++++++++ .../DeprecatedFunctionMetricTest.2.inc | 18 +++++++++ 8 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 tests/units/Checks/DeprecatedFunctionMetricTest.php create mode 100644 tests/units/Checks/TestData/DeprecatedFunctionMetricTest.1.inc create mode 100644 tests/units/Checks/TestData/DeprecatedFunctionMetricTest.2.inc diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 080d2e3..9945299 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -40,6 +40,7 @@ class ErrorConstants { const TYPE_INCORRECT_REGEX = 'Standard.Incorrect.Regex'; const TYPE_METRICS_COMPLEXITY = 'Standard.Metrics.Complexity'; const TYPE_METRICS_LINES_OF_CODE = 'Standard.Metrics.Lines'; + const TYPE_METRICS_DEPRECATED_FUNCTIONS = 'Standard.Metrics.Deprecated'; const TYPE_MISSING_BREAK = 'Standard.Switch.Break'; const TYPE_MISSING_CONSTRUCT = 'Standard.Constructor.MissingCall'; const TYPE_NULL_DEREFERENCE = "Standard.Null.Dereference"; diff --git a/src/Evaluators/FunctionLike.php b/src/Evaluators/FunctionLike.php index 9da50f9..9a487c1 100644 --- a/src/Evaluators/FunctionLike.php +++ b/src/Evaluators/FunctionLike.php @@ -3,6 +3,7 @@ namespace BambooHR\Guardrail\Evaluators; use BambooHR\Guardrail\Checks\ErrorConstants; +use BambooHR\Guardrail\Metrics\Metric; use BambooHR\Guardrail\NodeVisitors\ForEachNode; use BambooHR\Guardrail\Scope; use BambooHR\Guardrail\Scope\ScopeStack; @@ -19,8 +20,30 @@ function getInstanceType(): array { return [Node\Stmt\Function_::class, Node\Stmt\ClassMethod::class]; } + /** + * This picks up for functions and methods, but not closures. Closures are handled in the Expression\FunctionLike class. + * @param Node $node + * @param SymbolTable $table + * @param ScopeStack $scopeStack + * + * @return void + */ function onEnter(Node $node, SymbolTable $table, ScopeStack $scopeStack): void { - // This picks up for functions and methods, but not closures. Closures are handled in the Expression\FunctionLike class. + $func = null; + if ($node instanceof Node\Stmt\Function_) { + $func = $table->getAbstractedFunction($node->name); + } else if ($node instanceof Node\Stmt\ClassMethod) { + $func = $table->getAbstractedMethod($scopeStack->getCurrentClass()->namespacedName, $node->name); + } + if ($func?->isDeprecated() ?? false) { + $scopeStack->getMetricOutput()->emitMetric(new Metric( + $node->name, + $node->getLine(), + ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS, + [] + )); + } + self::handleEnterFunctionLike($node, $scopeStack); } diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index 0d69da2..bbbc5dd 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -75,6 +75,7 @@ class StaticAnalyzer extends NodeVisitorAbstract private $checks = []; private ScopeStack $scopeStack; + private MetricOutputInterface $metricOutput; /** @var array */ private $timings = []; @@ -114,7 +115,7 @@ function getTimingsAndCounts() function __construct(SymbolTable $index, OutputInterface $output, MetricOutputInterface $metricOutput, Config $config) { $this->index = $index; - $this->scopeStack = new ScopeStack($output, $config); + $this->scopeStack = new ScopeStack($output, $metricOutput, $config); $this->scopeStack->pushScope(new Scope(true, true, false)); $this->metricOutput = $metricOutput; @@ -216,7 +217,9 @@ public function getEvaluator(Node $node): ?Ev\EvaluatorInterface public function setFile($name, Config $config) { $this->file = $name; - $this->scopeStack = new ScopeStack($this->scopeStack->getOutput(), $config); + $this->scopeStack = new ScopeStack( + $this->scopeStack->getOutput(), $this->scopeStack->getMetricOutput(), $config + ); $this->scopeStack->pushScope(new Scope(true, true, false)); $this->scopeStack->setCurrentFile($name); } diff --git a/src/Scope/ScopeStack.php b/src/Scope/ScopeStack.php index 3aed4b6..067a55f 100644 --- a/src/Scope/ScopeStack.php +++ b/src/Scope/ScopeStack.php @@ -3,6 +3,7 @@ namespace BambooHR\Guardrail\Scope; use BambooHR\Guardrail\Config; +use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\Output\OutputInterface; use BambooHR\Guardrail\Scope as ScopeInterface; use PhpParser\Node; @@ -21,6 +22,7 @@ class ScopeStack implements ScopeInterface { function __construct( private OutputInterface $output, + private MetricOutputInterface $metricOutput, private Config $config, ) { } @@ -32,6 +34,10 @@ function getOutput(): OutputInterface { return $this->output; } + public function getMetricOutput(): MetricOutputInterface { + return $this->metricOutput; + } + function pushClass(Node\Stmt\ClassLike $class):void { $this->classes[] = $class; diff --git a/tests/TestSuiteSetup.php b/tests/TestSuiteSetup.php index 7324270..3e4b91e 100644 --- a/tests/TestSuiteSetup.php +++ b/tests/TestSuiteSetup.php @@ -2,6 +2,7 @@ use BambooHR\Guardrail\Checks\BaseCheck; use BambooHR\Guardrail\Checks\ErrorConstants; +use BambooHR\Guardrail\Metrics\Metric; use BambooHR\Guardrail\Metrics\MetricInterface; use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\NodeVisitors\StaticAnalyzer; @@ -41,6 +42,37 @@ public function runAnalyzerOnFile($fileName, $emit, array $additionalConfig = [] return array_sum($counts); } + /** + * runAnalyzerOnFile + * + * @param string $fileName + * @param mixed $emit + * @param array $additionalConfig + * + * @return XUnitOutput + */ + public function getOutputFromAnalyzer($fileName, $emit, array $additionalConfig = []) { + return $this->analyzeFileToOutput($fileName, $emit, $additionalConfig); + } + + /** + * @param $output + * @param $metricType + * + * @return int + */ + public function getMetricCountByName($output, $metricType) { + /** @var Metric[] $counts */ + $counts = $output->metrics; + $count = 0; + foreach($counts as $metric) { + if ($metric->getType() == $metricType) { + $count++; + } + } + return $count; + } + /** * @param string $fileName * @param mixed $emit @@ -59,13 +91,17 @@ public function analyzeFileToOutput($fileName, $emit, array $additionalConfig = $config = new TestConfig($fileName, $emit, $additionalConfig); $output = new class($config) extends XUnitOutput implements MetricOutputInterface { + public array $metrics = []; function emitMetric(MetricInterface $metric): void { - return; + $this->metrics[] = $metric; } }; $indexer = new IndexingPhase($config, $output); $indexer->indexFile($fileName); + foreach ($additionalConfig['additionalFilesToIndex'] ?? [] as $file) { + $indexer->indexFile($file); + } $analyzer = new AnalyzingPhase(); $analyzer->initParser($config, $output); diff --git a/tests/units/Checks/DeprecatedFunctionMetricTest.php b/tests/units/Checks/DeprecatedFunctionMetricTest.php new file mode 100644 index 0000000..0e70fab --- /dev/null +++ b/tests/units/Checks/DeprecatedFunctionMetricTest.php @@ -0,0 +1,32 @@ +getOutputFromAnalyzer('.1.inc', ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS); + $this->assertEquals(1, $this->getMetricCountByName($output, ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS)); + } + + /** + * testRunDeprecatedMetricOnClassMethod + * + * @return void + */ + public function testRunDeprecatedMetricOnClassMethod() { + $output = $this->getOutputFromAnalyzer('.2.inc', ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS); + $this->assertEquals(1, $this->getMetricCountByName($output, ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS)); + } +} \ No newline at end of file diff --git a/tests/units/Checks/TestData/DeprecatedFunctionMetricTest.1.inc b/tests/units/Checks/TestData/DeprecatedFunctionMetricTest.1.inc new file mode 100644 index 0000000..b82a352 --- /dev/null +++ b/tests/units/Checks/TestData/DeprecatedFunctionMetricTest.1.inc @@ -0,0 +1,16 @@ + Date: Tue, 14 Jan 2025 16:38:52 -0700 Subject: [PATCH 02/15] simplify enter method in FunctionLike Evaluator --- src/Evaluators/FunctionLike.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Evaluators/FunctionLike.php b/src/Evaluators/FunctionLike.php index 9a487c1..282a58a 100644 --- a/src/Evaluators/FunctionLike.php +++ b/src/Evaluators/FunctionLike.php @@ -29,13 +29,7 @@ function getInstanceType(): array { * @return void */ function onEnter(Node $node, SymbolTable $table, ScopeStack $scopeStack): void { - $func = null; - if ($node instanceof Node\Stmt\Function_) { - $func = $table->getAbstractedFunction($node->name); - } else if ($node instanceof Node\Stmt\ClassMethod) { - $func = $table->getAbstractedMethod($scopeStack->getCurrentClass()->namespacedName, $node->name); - } - if ($func?->isDeprecated() ?? false) { + if (str_contains($node->getDocComment()?->getText(), '@deprecated')) { $scopeStack->getMetricOutput()->emitMetric(new Metric( $node->name, $node->getLine(), From b704af0cbb29a51c1cdd772ca02685348041a6cd Mon Sep 17 00:00:00 2001 From: bknutson Date: Tue, 14 Jan 2025 16:40:58 -0700 Subject: [PATCH 03/15] remove unused class property --- src/NodeVisitors/StaticAnalyzer.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index bbbc5dd..0035542 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -75,7 +75,6 @@ class StaticAnalyzer extends NodeVisitorAbstract private $checks = []; private ScopeStack $scopeStack; - private MetricOutputInterface $metricOutput; /** @var array */ private $timings = []; @@ -117,7 +116,6 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn $this->index = $index; $this->scopeStack = new ScopeStack($output, $metricOutput, $config); $this->scopeStack->pushScope(new Scope(true, true, false)); - $this->metricOutput = $metricOutput; /** @var \BambooHR\Guardrail\Checks\BaseCheck[] $checkers */ $checkers = [ From f4798fc87d82221c6178e631d7b6cefa1aa9974d Mon Sep 17 00:00:00 2001 From: bknutson Date: Wed, 15 Jan 2025 12:01:16 -0700 Subject: [PATCH 04/15] [SPN-1440] New Guardrail Check to ensure documentation is present and report on deprecated web APIs --- src/Checks/ErrorConstants.php | 1 + src/Checks/WebApiDocumentationCheck.php | 66 +++++++++++++++++++ src/NodeVisitors/StaticAnalyzer.php | 2 + .../TestWebApiDocumentationCheck.1.inc | 29 ++++++++ .../TestWebApiDocumentationCheck.2.inc | 17 +++++ .../TestWebApiDocumentationCheck.3.inc | 18 +++++ .../Checks/TestWebApiDocumentationCheck.php | 29 ++++++++ 7 files changed, 162 insertions(+) create mode 100644 src/Checks/WebApiDocumentationCheck.php create mode 100644 tests/units/Checks/TestData/TestWebApiDocumentationCheck.1.inc create mode 100644 tests/units/Checks/TestData/TestWebApiDocumentationCheck.2.inc create mode 100644 tests/units/Checks/TestData/TestWebApiDocumentationCheck.3.inc create mode 100644 tests/units/Checks/TestWebApiDocumentationCheck.php diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 9945299..5f387ee 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -82,6 +82,7 @@ class ErrorConstants { const TYPE_VARIABLE_FUNCTION_NAME = 'Standard.VariableFunctionCall'; const TYPE_VARIABLE_VARIABLE = 'Standard.VariableVariable'; const TYPE_COUNTABLE_EMPTINESS_CHECK = 'Standard.Countable.Emptiness'; + const TYPE_WEB_API_DOCUMENTATION_CHECK = 'Standard.WebApi.Documentation'; /** diff --git a/src/Checks/WebApiDocumentationCheck.php b/src/Checks/WebApiDocumentationCheck.php new file mode 100644 index 0000000..d62cd21 --- /dev/null +++ b/src/Checks/WebApiDocumentationCheck.php @@ -0,0 +1,66 @@ +isPublic()) { + foreach ($node->attrGroups as $attrGroup) { + foreach ($attrGroup->attrs as $attribute) { + $attributeName = $attribute->name->toString(); + if (str_starts_with($attributeName, 'OpenApi\Attributes')) { + foreach ($attribute->args as $arg) { + if ($arg->name->name === 'deprecated' && $arg->value->name->toString() == 'true') { + $this->metricOutput->emitMetric(new Metric( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS, + [] + )); + break; + } + } + + return; + } + } + } + + $className = $inside->namespacedName->toString(); + $this->emitErrorOnLine( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK, + "All public controller methods should be associated with a route and must have + documentation through an OpenAPI Attribute. Method: {$node->name->name}, Class: $className" + ); + } + } +} \ No newline at end of file diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index 0035542..1c04ff2 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -43,6 +43,7 @@ use BambooHR\Guardrail\Checks\UnsafeSuperGlobalCheck; use BambooHR\Guardrail\Checks\UnusedPrivateMemberVariableCheck; use BambooHR\Guardrail\Checks\UseStatementCaseCheck; +use BambooHR\Guardrail\Checks\WebApiDocumentationCheck; use BambooHR\Guardrail\Config; use BambooHR\Guardrail\Evaluators as Ev; use BambooHR\Guardrail\Evaluators\ExpressionInterface; @@ -157,6 +158,7 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new ThrowsCheck($this->index, $output), new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), + new WebApiDocumentationCheck($this->index, $output, $metricOutput), //new ClassStoredAsVariableCheck($this->index, $output) ]; diff --git a/tests/units/Checks/TestData/TestWebApiDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestWebApiDocumentationCheck.1.inc new file mode 100644 index 0000000..346ef3a --- /dev/null +++ b/tests/units/Checks/TestData/TestWebApiDocumentationCheck.1.inc @@ -0,0 +1,29 @@ +assertEquals(2, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK,), ""); + } + + /** + * @return void + */ + public function testOnlyErrorsOnPublicMethods() { + $this->assertEquals(2, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK,), ""); + } + + public function testMethodWithDeprecatedAttribute() { + $output = $this->getOutputFromAnalyzer('.3.inc', ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS); + $this->assertEquals(1, $this->getMetricCountByName($output, ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS)); + } +} \ No newline at end of file From 05d95714b398b374ce4902dd315acca8ab46841f Mon Sep 17 00:00:00 2001 From: bknutson Date: Wed, 15 Jan 2025 12:08:42 -0700 Subject: [PATCH 05/15] fix test by ignoring documentation requirement --- tests/units/Checks/TestTemplates.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/units/Checks/TestTemplates.php b/tests/units/Checks/TestTemplates.php index c032037..89963cb 100644 --- a/tests/units/Checks/TestTemplates.php +++ b/tests/units/Checks/TestTemplates.php @@ -9,7 +9,7 @@ class TestTemplates extends TestSuiteSetup { function testValidTemplate() { - $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc',"Standard.*", ["ignore-errors"=>["Standard.Autoload.Unsafe"]])); + $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc',"Standard.*", ["ignore-errors"=>["Standard.Autoload.Unsafe", ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK]])); } From e76500278e26ab57ea8e7914995c814c869f461d Mon Sep 17 00:00:00 2001 From: bknutson Date: Wed, 15 Jan 2025 15:49:25 -0700 Subject: [PATCH 06/15] [SPN-1447] Start of Service Documentation Guardrail right now this just includes strong typing for params and returns --- src/Checks/ErrorConstants.php | 3 + .../ServiceMethodDocumentationCheck.php | 77 +++++++++++++++++++ src/NodeVisitors/StaticAnalyzer.php | 11 ++- .../TestServiceMethodDocumentationCheck.1.inc | 40 ++++++++++ .../TestServiceMethodDocumentationCheck.2.inc | 42 ++++++++++ .../TestServiceMethodDocumentationCheck.php | 26 +++++++ tests/units/Checks/TestTemplates.php | 8 +- 7 files changed, 202 insertions(+), 5 deletions(-) create mode 100644 src/Checks/ServiceMethodDocumentationCheck.php create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc create mode 100644 tests/units/Checks/TestServiceMethodDocumentationCheck.php diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 080d2e3..3266740 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -40,6 +40,7 @@ class ErrorConstants { const TYPE_INCORRECT_REGEX = 'Standard.Incorrect.Regex'; const TYPE_METRICS_COMPLEXITY = 'Standard.Metrics.Complexity'; const TYPE_METRICS_LINES_OF_CODE = 'Standard.Metrics.Lines'; + const TYPE_METRICS_DEPRECATED_FUNCTIONS = 'Standard.Metrics.Deprecated'; const TYPE_MISSING_BREAK = 'Standard.Switch.Break'; const TYPE_MISSING_CONSTRUCT = 'Standard.Constructor.MissingCall'; const TYPE_NULL_DEREFERENCE = "Standard.Null.Dereference"; @@ -81,6 +82,8 @@ class ErrorConstants { const TYPE_VARIABLE_FUNCTION_NAME = 'Standard.VariableFunctionCall'; const TYPE_VARIABLE_VARIABLE = 'Standard.VariableVariable'; const TYPE_COUNTABLE_EMPTINESS_CHECK = 'Standard.Countable.Emptiness'; + const TYPE_WEB_API_DOCUMENTATION_CHECK = 'Standard.WebApi.Documentation'; + const TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK = 'Standard.ServiceMethod.Documentation'; /** diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php new file mode 100644 index 0000000..9f39cea --- /dev/null +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -0,0 +1,77 @@ +isPublic()) { + foreach ($node->getParams() as $param) { + if ($param->type instanceof Node\UnionType || $param->type instanceof Node\IntersectionType) { + foreach ($param->type->types as $type) { + $this->checkParamTyping($type, $node, $fileName, $inside); + } + } + else { + $this->checkParamTyping($param->type, $node, $fileName, $inside); + } + } + $returnType = $node->getReturnType(); + if ($returnType instanceof Node\UnionType || $returnType instanceof Node\IntersectionType) { + foreach ($returnType->types as $type) { + $this->checkReturnTyping($type->name ?? $type->toString(), $node, $fileName, $inside); + } + } else { + $this->checkReturnTyping($returnType, $node, $fileName, $inside); + } + } + } + + private function checkParamTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { + if (in_array($type?->name ?? $type?->toString() ?? null, [null, 'mixed', 'object'])) { + $this->emitErrorOnLine( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have strong types for all parameters. 'Mixed' and 'object' params are prohibited." + ); + } + } + + private function checkReturnTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { + if (in_array($type, [null, 'mixed', 'object'])) { + $this->emitErrorOnLine( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have a return type. 'Mixed' and 'object' return types are prohibited." + ); + } + } +} \ No newline at end of file diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index 0d69da2..d6da79d 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -33,6 +33,7 @@ use BambooHR\Guardrail\Checks\ReadOnlyPropertyCheck; use BambooHR\Guardrail\Checks\ReturnCheck; use BambooHR\Guardrail\Checks\DependenciesOnVendorCheck; +use BambooHR\Guardrail\Checks\ServiceMethodDocumentationCheck; use BambooHR\Guardrail\Checks\SplatCheck; use BambooHR\Guardrail\Checks\StaticCallCheck; use BambooHR\Guardrail\Checks\StaticPropertyFetchCheck; @@ -43,6 +44,7 @@ use BambooHR\Guardrail\Checks\UnsafeSuperGlobalCheck; use BambooHR\Guardrail\Checks\UnusedPrivateMemberVariableCheck; use BambooHR\Guardrail\Checks\UseStatementCaseCheck; +use BambooHR\Guardrail\Checks\WebApiDocumentationCheck; use BambooHR\Guardrail\Config; use BambooHR\Guardrail\Evaluators as Ev; use BambooHR\Guardrail\Evaluators\ExpressionInterface; @@ -114,9 +116,8 @@ function getTimingsAndCounts() function __construct(SymbolTable $index, OutputInterface $output, MetricOutputInterface $metricOutput, Config $config) { $this->index = $index; - $this->scopeStack = new ScopeStack($output, $config); + $this->scopeStack = new ScopeStack($output, $metricOutput, $config); $this->scopeStack->pushScope(new Scope(true, true, false)); - $this->metricOutput = $metricOutput; /** @var \BambooHR\Guardrail\Checks\BaseCheck[] $checkers */ $checkers = [ @@ -158,6 +159,8 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new ThrowsCheck($this->index, $output), new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), + new WebApiDocumentationCheck($this->index, $output, $metricOutput), + new ServiceMethodDocumentationCheck($this->index, $output, $metricOutput), //new ClassStoredAsVariableCheck($this->index, $output) ]; @@ -216,7 +219,9 @@ public function getEvaluator(Node $node): ?Ev\EvaluatorInterface public function setFile($name, Config $config) { $this->file = $name; - $this->scopeStack = new ScopeStack($this->scopeStack->getOutput(), $config); + $this->scopeStack = new ScopeStack( + $this->scopeStack->getOutput(), $this->scopeStack->getMetricOutput(), $config + ); $this->scopeStack->pushScope(new Scope(true, true, false)); $this->scopeStack->setCurrentFile($name); } diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc new file mode 100644 index 0000000..a51e29a --- /dev/null +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc @@ -0,0 +1,40 @@ +assertEquals(5, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } + + /** + * testParametersSetAndUnset + * + * @return void + */ + public function testReturnTypesSetAndUnset() { + $this->assertEquals(4, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } +} \ No newline at end of file diff --git a/tests/units/Checks/TestTemplates.php b/tests/units/Checks/TestTemplates.php index c032037..8717b27 100644 --- a/tests/units/Checks/TestTemplates.php +++ b/tests/units/Checks/TestTemplates.php @@ -9,8 +9,12 @@ class TestTemplates extends TestSuiteSetup { function testValidTemplate() { - $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc',"Standard.*", ["ignore-errors"=>["Standard.Autoload.Unsafe"]])); - + $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc', "Standard.*", [ + "ignore-errors" => [ + "Standard.Autoload.Unsafe", ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK, + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + ] + ])); } From e7a07a2466a398e5b4ba9b5174f6cec34910bf2f Mon Sep 17 00:00:00 2001 From: bknutson Date: Thu, 16 Jan 2025 07:53:46 -0700 Subject: [PATCH 07/15] add search phrases check --- src/Checks/ErrorConstants.php | 1 + src/Checks/WebApiDocumentationCheck.php | 52 ++++++++++++++----- .../TestWebApiDocumentationCheck.4.inc | 20 +++++++ .../Checks/TestWebApiDocumentationCheck.php | 14 ++++- 4 files changed, 73 insertions(+), 14 deletions(-) create mode 100644 tests/units/Checks/TestData/TestWebApiDocumentationCheck.4.inc diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 5f387ee..5d85485 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -83,6 +83,7 @@ class ErrorConstants { const TYPE_VARIABLE_VARIABLE = 'Standard.VariableVariable'; const TYPE_COUNTABLE_EMPTINESS_CHECK = 'Standard.Countable.Emptiness'; const TYPE_WEB_API_DOCUMENTATION_CHECK = 'Standard.WebApi.Documentation'; + const TYPE_WEB_API_DOCUMENTATION_SEARCH_PHRASES_CHECK = 'Standard.WebApi.Documentation.SearchPhrases'; /** diff --git a/src/Checks/WebApiDocumentationCheck.php b/src/Checks/WebApiDocumentationCheck.php index d62cd21..e326d87 100644 --- a/src/Checks/WebApiDocumentationCheck.php +++ b/src/Checks/WebApiDocumentationCheck.php @@ -9,6 +9,11 @@ use PhpParser\Node\Stmt\ClassLike; class WebApiDocumentationCheck extends BaseCheck { + private const string ATTRIBUTE_NAMESPACE = 'OpenApi\Attributes'; + private const string SEARCH_PHRASES_KEY = 'vector-search-phrases'; + private const string DEPRECATED_KEY = 'deprecated'; + private const string X_KEY = 'x'; + function __construct($index, $output, private readonly MetricOutputInterface $metricOutput) { parent::__construct($index, $output); } @@ -35,24 +40,24 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop foreach ($node->attrGroups as $attrGroup) { foreach ($attrGroup->attrs as $attribute) { $attributeName = $attribute->name->toString(); - if (str_starts_with($attributeName, 'OpenApi\Attributes')) { + if (str_starts_with($attributeName, self::ATTRIBUTE_NAMESPACE)) { + $hasDefinedSearchPhrases = false; foreach ($attribute->args as $arg) { - if ($arg->name->name === 'deprecated' && $arg->value->name->toString() == 'true') { - $this->metricOutput->emitMetric(new Metric( - $fileName, - $node->getLine(), - ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS, - [] - )); - break; - } + $this->checkDeprecatedAttribute($arg, $fileName, $node); + $hasDefinedSearchPhrases = $hasDefinedSearchPhrases ?: $this->hasVectorSearchPhrase($arg); + } + if (!$hasDefinedSearchPhrases) { + $this->emitErrorOnLine( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_WEB_API_DOCUMENTATION_SEARCH_PHRASES_CHECK, + "OpenAPI Attribute must have a vector-search-phrases key defined. Method: {$node->name->name}" + ); } - return; } } } - $className = $inside->namespacedName->toString(); $this->emitErrorOnLine( $fileName, @@ -63,4 +68,27 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop ); } } + + private function checkDeprecatedAttribute($arg, $fileName, $node) { + if ($arg->name->name === self::DEPRECATED_KEY && $arg->value->name->toString() == 'true') { + $this->metricOutput->emitMetric(new Metric( + $fileName, + $node->getLine(), + ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS, + [] + )); + } + } + + private function hasVectorSearchPhrase($arg): bool { + if ($arg->name->name === self::X_KEY && $arg->value instanceof Node\Expr\Array_) { + foreach ($arg->value->items as $item) { + if ($item->key->value === self::SEARCH_PHRASES_KEY) { + return true; + } + } + } + + return false; + } } \ No newline at end of file diff --git a/tests/units/Checks/TestData/TestWebApiDocumentationCheck.4.inc b/tests/units/Checks/TestData/TestWebApiDocumentationCheck.4.inc new file mode 100644 index 0000000..a481047 --- /dev/null +++ b/tests/units/Checks/TestData/TestWebApiDocumentationCheck.4.inc @@ -0,0 +1,20 @@ + []])] + public function withEmptySearchPhrases() { + return false; + } + + #[OA\Get(path: "/test", x: ['vector-search-phrases' => ['my test search phrase']])] + public function withSearchPhrases() { + return false; + } +} \ No newline at end of file diff --git a/tests/units/Checks/TestWebApiDocumentationCheck.php b/tests/units/Checks/TestWebApiDocumentationCheck.php index 0ca4041..df34785 100644 --- a/tests/units/Checks/TestWebApiDocumentationCheck.php +++ b/tests/units/Checks/TestWebApiDocumentationCheck.php @@ -12,18 +12,28 @@ class TestWebApiDocumentationCheck extends TestSuiteSetup { * @return void */ public function testApiAttributeIsPresent() { - $this->assertEquals(2, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK,), ""); + $this->assertEquals(2, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK), ""); } /** * @return void */ public function testOnlyErrorsOnPublicMethods() { - $this->assertEquals(2, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK,), ""); + $this->assertEquals(2, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK), ""); } + /** + * @return void + */ public function testMethodWithDeprecatedAttribute() { $output = $this->getOutputFromAnalyzer('.3.inc', ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS); $this->assertEquals(1, $this->getMetricCountByName($output, ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS)); } + + /** + * @return void + */ + public function testWithAndWithoutVectorSearchPhrases() { + $this->assertEquals(1, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_SEARCH_PHRASES_CHECK), ""); + } } \ No newline at end of file From 34ba8b1f8fbcb49e3f166e02c7bd40d5f032ddbc Mon Sep 17 00:00:00 2001 From: bknutson Date: Thu, 16 Jan 2025 13:49:15 -0700 Subject: [PATCH 08/15] [SPN-1448] Refactor Service Method Documentation Check and add checks to ensure doc blocks match contract --- .../ServiceMethodDocumentationCheck.php | 175 ++++++++++++++---- src/NodeVisitors/StaticAnalyzer.php | 2 +- .../TestServiceMethodDocumentationCheck.1.inc | 32 +++- .../TestServiceMethodDocumentationCheck.2.inc | 25 ++- .../TestServiceMethodDocumentationCheck.3.inc | 53 ++++++ .../TestServiceMethodDocumentationCheck.4.inc | 62 +++++++ .../TestServiceMethodDocumentationCheck.php | 14 ++ 7 files changed, 321 insertions(+), 42 deletions(-) create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc create mode 100644 tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.4.inc diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 9f39cea..9de684d 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -2,17 +2,13 @@ namespace BambooHR\Guardrail\Checks; -use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\Scope; use PhpParser\Node; use PhpParser\Node\Stmt\ClassLike; class ServiceMethodDocumentationCheck extends BaseCheck { - function __construct($index, $output, private readonly MetricOutputInterface $metricOutput) { - parent::__construct($index, $output); - } - + private const array BLOCKED_SERVICE_DOCUMENTATION_TYPES = [null, 'mixed', 'object']; /** * getCheckNodeTypes * @@ -32,46 +28,153 @@ function getCheckNodeTypes() { */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { if ($node instanceof Node\Stmt\ClassMethod && $node->isPublic()) { - foreach ($node->getParams() as $param) { - if ($param->type instanceof Node\UnionType || $param->type instanceof Node\IntersectionType) { - foreach ($param->type->types as $type) { - $this->checkParamTyping($type, $node, $fileName, $inside); - } - } - else { - $this->checkParamTyping($param->type, $node, $fileName, $inside); - } - } - $returnType = $node->getReturnType(); - if ($returnType instanceof Node\UnionType || $returnType instanceof Node\IntersectionType) { - foreach ($returnType->types as $type) { - $this->checkReturnTyping($type->name ?? $type->toString(), $node, $fileName, $inside); - } - } else { - $this->checkReturnTyping($returnType, $node, $fileName, $inside); + $docComment = $node->getDocComment(); + if (empty($docComment)) { + $this->emitMissingDocBlockError($fileName, $node, $inside); + return; } + + $docCommentData = $this->extractDocCommentData($docComment); + $actualParams = array_map(fn($param) => $param->var, $node->getParams()); + $docCommentParams = $docCommentData['params']; + + $this->validateParameters($actualParams, $docCommentParams, $fileName, $node, $inside); + $this->validateReturnType($node, $docCommentData['return'], $fileName, $inside); } } - private function checkParamTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { - if (in_array($type?->name ?? $type?->toString() ?? null, [null, 'mixed', 'object'])) { - $this->emitErrorOnLine( - $fileName, - $node->getLine(), + private function emitMissingDocBlockError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { + $this->emitErrorOnLine($fileName, $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods must have a DocBlock." + ); + } + + private function validateParameters($actualParams, $docCommentParams, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { + if (count($docCommentParams) > count($actualParams)) { + $this->emitErrorOnLine($fileName, $node->getLine(), ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, - "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have strong types for all parameters. 'Mixed' and 'object' params are prohibited." + "Method: {$node->name->name}, Class: {$inside->name->name} - There are extra parameters in your DocBlock that are not present in the method signature." ); } + + foreach ($actualParams as $actualParam) { + $this->validateParameter($actualParam, $docCommentParams, $fileName, $node, $inside); + } } - private function checkReturnTyping($type, Node\Stmt\ClassMethod $node, string $fileName, Node\Stmt\ClassLike $inside) { - if (in_array($type, [null, 'mixed', 'object'])) { - $this->emitErrorOnLine( - $fileName, - $node->getLine(), - ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, - "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods should have a return type. 'Mixed' and 'object' return types are prohibited." - ); + private function validateParameter($actualParam, $docCommentParams, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { + $actualParamName = $actualParam->name ?? $actualParam->getString(); + $docCommentParam = $docCommentParams[$actualParamName] ?? null; + if (!$docCommentParam) { + $this->emitParameterMismatchError($fileName, $node, $inside, $actualParamName); + return; + } + + $actualParamAttribute = $actualParam->getAttribute('inferrer-type'); + if ($this->isComplexType($actualParamAttribute)) { + $this->validateComplexType($actualParamAttribute->types, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); + } else { + $actualParamType = $actualParamAttribute?->name ?? $actualParamAttribute?->toString(); + $this->validateSimpleType($actualParamType, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); } } + + private function isComplexType($type): bool { + return $type instanceof Node\UnionType || $type instanceof Node\IntersectionType; + } + + private function validateComplexType($actualParamTypes, $docCommentParamType, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { + if (!is_array($docCommentParamType) || count($actualParamTypes) !== count($docCommentParamType)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + return; + } + + foreach ($actualParamTypes as $typeObject) { + $actualParamType = $typeObject->name ?? $typeObject->toString(); + if (!in_array($actualParamType, $docCommentParamType) || in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + break; + } + } + } + + private function validateSimpleType($actualParamType, $docCommentParamType, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $paramName): void { + if ($actualParamType !== $docCommentParamType || in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $paramName); + } + } + + private function emitParameterMismatchError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $paramName): void { + $this->emitErrorOnLine($fileName, $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name} - DocBlock does not contain matching parameter: $paramName" + ); + } + + private function emitTypeMismatchError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { + $this->emitErrorOnLine($fileName, $node->getLine(), + ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, + "Method: {$node->name->name}, Class: {$inside->name->name}, Property: $propertyName - DocBlock does not match method signature." + ); + } + + /** + * @param Node\Stmt\ClassMethod $node + * @param $docCommentReturn + * @param string $fileName + * @param ClassLike $inside + * + * @return void + */ + private function validateReturnType(Node\Stmt\ClassMethod $node, $docCommentReturn, string $fileName, Node\Stmt\ClassLike $inside): void { + $propertyName = 'return'; + if (!$docCommentReturn) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + return; + } + + $actualReturn = $node->getReturnType(); + if ($this->isComplexType($actualReturn)) { + $this->validateComplexType($actualReturn->types, $docCommentReturn, $fileName, $node, $inside, $propertyName); + } else { + $this->validateSimpleType($actualReturn?->name ?? $actualReturn?->toString() ?? null, $docCommentReturn, $fileName, $node, $inside, $propertyName); + } + } + + /** + * @param string|null $docComment + * + * @return array + */ + private function extractDocCommentData(?string $docComment) { + $result = [ + 'params' => [], + 'return' => null, + ]; + if (preg_match_all('/@param\s+([^\s$]+(?:\s*[\|&]\s*[^\s$]+)*)?\s*(\$[^\s]+)?/', $docComment, $paramMatches, PREG_SET_ORDER)) { + foreach ($paramMatches as $paramMatch) { + $result['params'][$variableName] = [ + 'type' => $this->extractDocGetVariableType($paramMatch[1] ?? null), + 'variable' => $variableName = ltrim($paramMatch[2] ?? null, '$'), + ]; + } + } + + if (preg_match('/@return\s+([^\s$]+(?:\s*\|\s*[^\s$]+|&[^\s$]+)*)/', $docComment, $returnMatch)) { + $result['return'] = $this->extractDocGetVariableType($returnMatch[1] ?? null); + } + + return $result; + } + + private function extractDocGetVariableType($variableType) { + if (str_contains($variableType, '|')) { + $variableType = array_map('trim', explode('|', $variableType)); + } else if (str_contains($variableType, '&')) { + $variableType = array_map('trim', explode('&', $variableType)); + } + + return $variableType; + } } \ No newline at end of file diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index d6da79d..2df9279 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -160,7 +160,7 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), new WebApiDocumentationCheck($this->index, $output, $metricOutput), - new ServiceMethodDocumentationCheck($this->index, $output, $metricOutput), + new ServiceMethodDocumentationCheck($this->index, $output), //new ClassStoredAsVariableCheck($this->index, $output) ]; diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc index a51e29a..6d8b51b 100644 --- a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc @@ -3,8 +3,8 @@ class MyService { // This method should emit 2 errors /** - * @param string $a - * @param int $b + * @param int $a + * @param string $b * * @return int */ @@ -13,21 +13,49 @@ class MyService { } // This method should emit 2 errors + + /** + * @param mixed $a + * @param object $b + * + * @return int + */ public function specialParamTypes(mixed $a, object $b): int { return 123; } // This method should emit 1 error + + /** + * @param int|string|object $a + * @param int|float $b + * + * @return int + */ public function unionParamTypes(int | string | object $a, int | float $b): int { return 123; } // This method should emit 0 errors + + /** + * @param MyObj2&MyObj $a + * + * @return int + */ public function intersectionParamTypes(MyObj&MyObj2 $a): int { return 123; } // This method should emit 0 errors + + /** + * @param bool $a + * @param int $b + * @param string $c + * + * @return int + */ public function withNormalTypedParams(bool $a, int $b, string $c): int { return 123; } diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc index e7f5122..ebb7847 100644 --- a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc @@ -10,28 +10,47 @@ class MyService { } // This method should emit 1 error + + /** + * @return mixed + */ public function specialReturnType(): mixed { return 123; } // This method should emit 1 error + + /** + * @return object + */ public function specialReturnType2(): object { return 123; } // This method should emit 1 error - public function unionParamTypes(): int|string|object { + + /** + * @return int|string|object + */ + public function unionParamTypes(): int | string | object { return 123; } // This method should emit 0 errors + + /** + * @return MyObj2&MyObj + */ public function intersectionParamTypes(): MyObj&MyObj2 { - return 123; } // This method should emit 0 errors + + /** + * @return bool + */ public function withNormalTypedParams(): bool { - return 123; + return true; } } diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc new file mode 100644 index 0000000..c3661bd --- /dev/null +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.3.inc @@ -0,0 +1,53 @@ +assertEquals(4, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); } + + /** + * @return void + */ + public function testSimpleTypesMatchingAndUnMatchingDocBlocks() { + $this->assertEquals(7, $this->runAnalyzerOnFile('.3.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } + + /** + * @return void + */ + public function testComplexTypesMatchingAndUnMatchingDocBlocks() { + $this->assertEquals(13, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + } } \ No newline at end of file From 4bbf82e8aa811f7d860fa7ce53e552b7b170a1e4 Mon Sep 17 00:00:00 2001 From: bknutson Date: Wed, 22 Jan 2025 17:14:41 -0700 Subject: [PATCH 09/15] move metric emit from evaluator into the new ServiceMethodDocumentationCheck.php --- .../ServiceMethodDocumentationCheck.php | 23 +++++++++++++++++++ src/Evaluators/FunctionLike.php | 9 -------- src/NodeVisitors/StaticAnalyzer.php | 2 +- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 9de684d..0e50616 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -2,12 +2,18 @@ namespace BambooHR\Guardrail\Checks; +use BambooHR\Guardrail\Metrics\Metric; +use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\Scope; use PhpParser\Node; use PhpParser\Node\Stmt\ClassLike; class ServiceMethodDocumentationCheck extends BaseCheck { + function __construct($index, $output, private MetricOutputInterface $metricOutput) { + parent::__construct($index, $output); + } + private const array BLOCKED_SERVICE_DOCUMENTATION_TYPES = [null, 'mixed', 'object']; /** * getCheckNodeTypes @@ -38,6 +44,7 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop $actualParams = array_map(fn($param) => $param->var, $node->getParams()); $docCommentParams = $docCommentData['params']; + $this->emitMetricsForNode($node); $this->validateParameters($actualParams, $docCommentParams, $fileName, $node, $inside); $this->validateReturnType($node, $docCommentData['return'], $fileName, $inside); } @@ -50,6 +57,22 @@ private function emitMissingDocBlockError(string $fileName, Node\Stmt\ClassMetho ); } + /** + * @param Node\Stmt\ClassMethod $node + * + * @return void + */ + private function emitMetricsForNode(Node\Stmt\ClassMethod $node): void { + if (str_contains($node->getDocComment()?->getText(), '@deprecated')) { + $this->metricOutput->emitMetric(new Metric( + $node->name, + $node->getLine(), + ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS, + [] + )); + } + } + private function validateParameters($actualParams, $docCommentParams, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { if (count($docCommentParams) > count($actualParams)) { $this->emitErrorOnLine($fileName, $node->getLine(), diff --git a/src/Evaluators/FunctionLike.php b/src/Evaluators/FunctionLike.php index 282a58a..59f016d 100644 --- a/src/Evaluators/FunctionLike.php +++ b/src/Evaluators/FunctionLike.php @@ -29,15 +29,6 @@ function getInstanceType(): array { * @return void */ function onEnter(Node $node, SymbolTable $table, ScopeStack $scopeStack): void { - if (str_contains($node->getDocComment()?->getText(), '@deprecated')) { - $scopeStack->getMetricOutput()->emitMetric(new Metric( - $node->name, - $node->getLine(), - ErrorConstants::TYPE_METRICS_DEPRECATED_FUNCTIONS, - [] - )); - } - self::handleEnterFunctionLike($node, $scopeStack); } diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index 2df9279..d6da79d 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -160,7 +160,7 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), new WebApiDocumentationCheck($this->index, $output, $metricOutput), - new ServiceMethodDocumentationCheck($this->index, $output), + new ServiceMethodDocumentationCheck($this->index, $output, $metricOutput), //new ClassStoredAsVariableCheck($this->index, $output) ]; From e88a91c36051766c9161275caa21aa7c38299346 Mon Sep 17 00:00:00 2001 From: bknutson Date: Wed, 22 Jan 2025 17:21:45 -0700 Subject: [PATCH 10/15] rename check to open api attribute documentation instead of web api documentation --- src/Checks/ErrorConstants.php | 4 ++-- ...onCheck.php => OpenApiAttributeDocumentationCheck.php} | 6 +++--- src/NodeVisitors/StaticAnalyzer.php | 4 ++-- ...1.inc => TestOpenApiAttributeDocumentationCheck.1.inc} | 0 ...2.inc => TestOpenApiAttributeDocumentationCheck.2.inc} | 0 ...3.inc => TestOpenApiAttributeDocumentationCheck.3.inc} | 0 ...4.inc => TestOpenApiAttributeDocumentationCheck.4.inc} | 0 ...eck.php => TestOpenApiAttributeDocumentationCheck.php} | 8 ++++---- tests/units/Checks/TestTemplates.php | 2 +- 9 files changed, 12 insertions(+), 12 deletions(-) rename src/Checks/{WebApiDocumentationCheck.php => OpenApiAttributeDocumentationCheck.php} (93%) rename tests/units/Checks/TestData/{TestWebApiDocumentationCheck.1.inc => TestOpenApiAttributeDocumentationCheck.1.inc} (100%) rename tests/units/Checks/TestData/{TestWebApiDocumentationCheck.2.inc => TestOpenApiAttributeDocumentationCheck.2.inc} (100%) rename tests/units/Checks/TestData/{TestWebApiDocumentationCheck.3.inc => TestOpenApiAttributeDocumentationCheck.3.inc} (100%) rename tests/units/Checks/TestData/{TestWebApiDocumentationCheck.4.inc => TestOpenApiAttributeDocumentationCheck.4.inc} (100%) rename tests/units/Checks/{TestWebApiDocumentationCheck.php => TestOpenApiAttributeDocumentationCheck.php} (77%) diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 0ba9017..073f74a 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -82,8 +82,8 @@ class ErrorConstants { const TYPE_VARIABLE_FUNCTION_NAME = 'Standard.VariableFunctionCall'; const TYPE_VARIABLE_VARIABLE = 'Standard.VariableVariable'; const TYPE_COUNTABLE_EMPTINESS_CHECK = 'Standard.Countable.Emptiness'; - const TYPE_WEB_API_DOCUMENTATION_CHECK = 'Standard.WebApi.Documentation'; - const TYPE_WEB_API_DOCUMENTATION_SEARCH_PHRASES_CHECK = 'Standard.WebApi.Documentation.SearchPhrases'; + const TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK = 'Standard.OpenApiAttribute.Documentation'; + const TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_SEARCH_PHRASES_CHECK = 'Standard.OpenApiAttribute.Documentation.SearchPhrases'; const TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK = 'Standard.ServiceMethod.Documentation'; diff --git a/src/Checks/WebApiDocumentationCheck.php b/src/Checks/OpenApiAttributeDocumentationCheck.php similarity index 93% rename from src/Checks/WebApiDocumentationCheck.php rename to src/Checks/OpenApiAttributeDocumentationCheck.php index e326d87..1574a64 100644 --- a/src/Checks/WebApiDocumentationCheck.php +++ b/src/Checks/OpenApiAttributeDocumentationCheck.php @@ -8,7 +8,7 @@ use PhpParser\Node; use PhpParser\Node\Stmt\ClassLike; -class WebApiDocumentationCheck extends BaseCheck { +class OpenApiAttributeDocumentationCheck extends BaseCheck { private const string ATTRIBUTE_NAMESPACE = 'OpenApi\Attributes'; private const string SEARCH_PHRASES_KEY = 'vector-search-phrases'; private const string DEPRECATED_KEY = 'deprecated'; @@ -50,7 +50,7 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop $this->emitErrorOnLine( $fileName, $node->getLine(), - ErrorConstants::TYPE_WEB_API_DOCUMENTATION_SEARCH_PHRASES_CHECK, + ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_SEARCH_PHRASES_CHECK, "OpenAPI Attribute must have a vector-search-phrases key defined. Method: {$node->name->name}" ); } @@ -62,7 +62,7 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop $this->emitErrorOnLine( $fileName, $node->getLine(), - ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK, + ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK, "All public controller methods should be associated with a route and must have documentation through an OpenAPI Attribute. Method: {$node->name->name}, Class: $className" ); diff --git a/src/NodeVisitors/StaticAnalyzer.php b/src/NodeVisitors/StaticAnalyzer.php index d6da79d..df021f7 100644 --- a/src/NodeVisitors/StaticAnalyzer.php +++ b/src/NodeVisitors/StaticAnalyzer.php @@ -44,7 +44,7 @@ use BambooHR\Guardrail\Checks\UnsafeSuperGlobalCheck; use BambooHR\Guardrail\Checks\UnusedPrivateMemberVariableCheck; use BambooHR\Guardrail\Checks\UseStatementCaseCheck; -use BambooHR\Guardrail\Checks\WebApiDocumentationCheck; +use BambooHR\Guardrail\Checks\OpenApiAttributeDocumentationCheck; use BambooHR\Guardrail\Config; use BambooHR\Guardrail\Evaluators as Ev; use BambooHR\Guardrail\Evaluators\ExpressionInterface; @@ -159,7 +159,7 @@ function __construct(SymbolTable $index, OutputInterface $output, MetricOutputIn new ThrowsCheck($this->index, $output), new CountableEmptinessCheck($this->index, $output), new DependenciesOnVendorCheck($this->index, $output, $metricOutput), - new WebApiDocumentationCheck($this->index, $output, $metricOutput), + new OpenApiAttributeDocumentationCheck($this->index, $output, $metricOutput), new ServiceMethodDocumentationCheck($this->index, $output, $metricOutput), //new ClassStoredAsVariableCheck($this->index, $output) ]; diff --git a/tests/units/Checks/TestData/TestWebApiDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.1.inc similarity index 100% rename from tests/units/Checks/TestData/TestWebApiDocumentationCheck.1.inc rename to tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.1.inc diff --git a/tests/units/Checks/TestData/TestWebApiDocumentationCheck.2.inc b/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.2.inc similarity index 100% rename from tests/units/Checks/TestData/TestWebApiDocumentationCheck.2.inc rename to tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.2.inc diff --git a/tests/units/Checks/TestData/TestWebApiDocumentationCheck.3.inc b/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.3.inc similarity index 100% rename from tests/units/Checks/TestData/TestWebApiDocumentationCheck.3.inc rename to tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.3.inc diff --git a/tests/units/Checks/TestData/TestWebApiDocumentationCheck.4.inc b/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.4.inc similarity index 100% rename from tests/units/Checks/TestData/TestWebApiDocumentationCheck.4.inc rename to tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.4.inc diff --git a/tests/units/Checks/TestWebApiDocumentationCheck.php b/tests/units/Checks/TestOpenApiAttributeDocumentationCheck.php similarity index 77% rename from tests/units/Checks/TestWebApiDocumentationCheck.php rename to tests/units/Checks/TestOpenApiAttributeDocumentationCheck.php index df34785..f4e0671 100644 --- a/tests/units/Checks/TestWebApiDocumentationCheck.php +++ b/tests/units/Checks/TestOpenApiAttributeDocumentationCheck.php @@ -5,21 +5,21 @@ use BambooHR\Guardrail\Checks\ErrorConstants; use BambooHR\Guardrail\Tests\TestSuiteSetup; -class TestWebApiDocumentationCheck extends TestSuiteSetup { +class TestOpenApiAttributeDocumentationCheck extends TestSuiteSetup { /** * testApiAttributeIsPresent * * @return void */ public function testApiAttributeIsPresent() { - $this->assertEquals(2, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK), ""); + $this->assertEquals(2, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK), ""); } /** * @return void */ public function testOnlyErrorsOnPublicMethods() { - $this->assertEquals(2, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK), ""); + $this->assertEquals(2, $this->runAnalyzerOnFile('.2.inc', ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK), ""); } /** @@ -34,6 +34,6 @@ public function testMethodWithDeprecatedAttribute() { * @return void */ public function testWithAndWithoutVectorSearchPhrases() { - $this->assertEquals(1, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_WEB_API_DOCUMENTATION_SEARCH_PHRASES_CHECK), ""); + $this->assertEquals(1, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_SEARCH_PHRASES_CHECK), ""); } } \ No newline at end of file diff --git a/tests/units/Checks/TestTemplates.php b/tests/units/Checks/TestTemplates.php index 8717b27..908900d 100644 --- a/tests/units/Checks/TestTemplates.php +++ b/tests/units/Checks/TestTemplates.php @@ -11,7 +11,7 @@ class TestTemplates extends TestSuiteSetup { function testValidTemplate() { $this->assertEquals(0, $this->runAnalyzerOnFile('.1.inc', "Standard.*", [ "ignore-errors" => [ - "Standard.Autoload.Unsafe", ErrorConstants::TYPE_WEB_API_DOCUMENTATION_CHECK, + "Standard.Autoload.Unsafe", ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK, ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, ] ])); From a2d7534006a5041e053592f2ccca572c3442493e Mon Sep 17 00:00:00 2001 From: bknutson Date: Thu, 23 Jan 2025 16:27:38 -0700 Subject: [PATCH 11/15] handle both methods and functions --- src/Checks/ServiceMethodDocumentationCheck.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 0e50616..87eb135 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -21,7 +21,7 @@ function __construct($index, $output, private MetricOutputInterface $metricOutpu * @return string[] */ function getCheckNodeTypes() { - return [Node\Stmt\ClassMethod::class]; + return [Node\Stmt\ClassMethod::class, Node\Stmt\Function_::class]; } /** @@ -33,6 +33,7 @@ function getCheckNodeTypes() { * @return void */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { + $this->emitMetricsForNode($node); if ($node instanceof Node\Stmt\ClassMethod && $node->isPublic()) { $docComment = $node->getDocComment(); if (empty($docComment)) { @@ -44,7 +45,6 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop $actualParams = array_map(fn($param) => $param->var, $node->getParams()); $docCommentParams = $docCommentData['params']; - $this->emitMetricsForNode($node); $this->validateParameters($actualParams, $docCommentParams, $fileName, $node, $inside); $this->validateReturnType($node, $docCommentData['return'], $fileName, $inside); } @@ -62,7 +62,7 @@ private function emitMissingDocBlockError(string $fileName, Node\Stmt\ClassMetho * * @return void */ - private function emitMetricsForNode(Node\Stmt\ClassMethod $node): void { + private function emitMetricsForNode($node): void { if (str_contains($node->getDocComment()?->getText(), '@deprecated')) { $this->metricOutput->emitMetric(new Metric( $node->name, From 36130e473cd3133994a5e8fb80b3beb1db64a1e1 Mon Sep 17 00:00:00 2001 From: bknutson Date: Fri, 24 Jan 2025 14:48:38 -0700 Subject: [PATCH 12/15] * remove search phrases requirement in favor of descriptions * fix some of the edge cases for the ServiceMethodDocumentationCheck.php. * Add better error descriptions * only run the checks on services and controllers * handle nullable types in the service check --- src/Checks/ErrorConstants.php | 1 - .../OpenApiAttributeDocumentationCheck.php | 49 ++++++---- .../ServiceMethodDocumentationCheck.php | 96 ++++++++++++++++--- ...stOpenApiAttributeDocumentationCheck.1.inc | 9 +- ...stOpenApiAttributeDocumentationCheck.2.inc | 4 +- ...stOpenApiAttributeDocumentationCheck.3.inc | 9 +- ...stOpenApiAttributeDocumentationCheck.4.inc | 12 +-- .../TestServiceMethodDocumentationCheck.1.inc | 35 ++++++- .../TestServiceMethodDocumentationCheck.2.inc | 5 +- .../TestServiceMethodDocumentationCheck.3.inc | 11 ++- .../TestServiceMethodDocumentationCheck.4.inc | 28 +++++- ...TestOpenApiAttributeDocumentationCheck.php | 2 +- .../TestServiceMethodDocumentationCheck.php | 4 +- 13 files changed, 208 insertions(+), 57 deletions(-) diff --git a/src/Checks/ErrorConstants.php b/src/Checks/ErrorConstants.php index 073f74a..0b9b9aa 100644 --- a/src/Checks/ErrorConstants.php +++ b/src/Checks/ErrorConstants.php @@ -83,7 +83,6 @@ class ErrorConstants { const TYPE_VARIABLE_VARIABLE = 'Standard.VariableVariable'; const TYPE_COUNTABLE_EMPTINESS_CHECK = 'Standard.Countable.Emptiness'; const TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK = 'Standard.OpenApiAttribute.Documentation'; - const TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_SEARCH_PHRASES_CHECK = 'Standard.OpenApiAttribute.Documentation.SearchPhrases'; const TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK = 'Standard.ServiceMethod.Documentation'; diff --git a/src/Checks/OpenApiAttributeDocumentationCheck.php b/src/Checks/OpenApiAttributeDocumentationCheck.php index 1574a64..d7e94ad 100644 --- a/src/Checks/OpenApiAttributeDocumentationCheck.php +++ b/src/Checks/OpenApiAttributeDocumentationCheck.php @@ -6,13 +6,15 @@ use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\Scope; use PhpParser\Node; +use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; class OpenApiAttributeDocumentationCheck extends BaseCheck { private const string ATTRIBUTE_NAMESPACE = 'OpenApi\Attributes'; private const string SEARCH_PHRASES_KEY = 'vector-search-phrases'; private const string DEPRECATED_KEY = 'deprecated'; - private const string X_KEY = 'x'; + private const string DESCRIPTION_KEY = 'description'; + private const string BASE_CONTROLLER = 'BaseController'; function __construct($index, $output, private readonly MetricOutputInterface $metricOutput) { parent::__construct($index, $output); @@ -36,29 +38,29 @@ function getCheckNodeTypes() { * @return void */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { - if ($node instanceof Node\Stmt\ClassMethod && $node->isPublic()) { + if ($node instanceof Node\Stmt\ClassMethod && $this->isControllerMethod($inside) && $node->isPublic() && $node->name->name !== '__construct') { foreach ($node->attrGroups as $attrGroup) { foreach ($attrGroup->attrs as $attribute) { - $attributeName = $attribute->name->toString(); + $attributeName = $attribute?->name?->toString(); if (str_starts_with($attributeName, self::ATTRIBUTE_NAMESPACE)) { - $hasDefinedSearchPhrases = false; + $hasDescription = false; foreach ($attribute->args as $arg) { $this->checkDeprecatedAttribute($arg, $fileName, $node); - $hasDefinedSearchPhrases = $hasDefinedSearchPhrases ?: $this->hasVectorSearchPhrase($arg); + $hasDescription = $hasDescription ?: $this->hasDescription($arg); } - if (!$hasDefinedSearchPhrases) { + if (!$hasDescription) { $this->emitErrorOnLine( $fileName, $node->getLine(), - ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_SEARCH_PHRASES_CHECK, - "OpenAPI Attribute must have a vector-search-phrases key defined. Method: {$node->name->name}" + ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK, + "OpenAPI Attribute must have a description. Method: {$node->name->name}" ); } return; } } } - $className = $inside->namespacedName->toString(); + $className = $inside?->namespacedName?->toString(); $this->emitErrorOnLine( $fileName, $node->getLine(), @@ -69,8 +71,23 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop } } + private function isControllerMethod(ClassLike $inside = null): bool { + if ($inside instanceof Class_) { + $parentClass = $inside->extends?->toString(); + if (str_contains($parentClass, self::BASE_CONTROLLER)) { + return true; + } + if ($inside->extends instanceof Node\Name) { + $parentClass = $this->symbolTable->getClass($inside->extends); + return $this->isControllerMethod($parentClass); + } + } + + return false; + } + private function checkDeprecatedAttribute($arg, $fileName, $node) { - if ($arg->name->name === self::DEPRECATED_KEY && $arg->value->name->toString() == 'true') { + if ($arg?->name?->name === self::DEPRECATED_KEY && $arg?->value?->name?->toString() == 'true') { $this->metricOutput->emitMetric(new Metric( $fileName, $node->getLine(), @@ -80,15 +97,7 @@ private function checkDeprecatedAttribute($arg, $fileName, $node) { } } - private function hasVectorSearchPhrase($arg): bool { - if ($arg->name->name === self::X_KEY && $arg->value instanceof Node\Expr\Array_) { - foreach ($arg->value->items as $item) { - if ($item->key->value === self::SEARCH_PHRASES_KEY) { - return true; - } - } - } - - return false; + private function hasDescription($arg): bool { + return $arg->name->name === self::DESCRIPTION_KEY && !empty($arg->value->value); } } \ No newline at end of file diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 87eb135..48aeb69 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -6,6 +6,7 @@ use BambooHR\Guardrail\Metrics\MetricOutputInterface; use BambooHR\Guardrail\Scope; use PhpParser\Node; +use PhpParser\Node\Stmt\Class_; use PhpParser\Node\Stmt\ClassLike; class ServiceMethodDocumentationCheck extends BaseCheck { @@ -14,7 +15,9 @@ function __construct($index, $output, private MetricOutputInterface $metricOutpu parent::__construct($index, $output); } - private const array BLOCKED_SERVICE_DOCUMENTATION_TYPES = [null, 'mixed', 'object']; + private const array BLOCKED_SERVICE_DOCUMENTATION_TYPES = [null, ...self::NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES]; + private const array NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES = ['mixed', 'object']; + private const string BASE_SERVICE = 'BaseService'; /** * getCheckNodeTypes * @@ -34,7 +37,7 @@ function getCheckNodeTypes() { */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { $this->emitMetricsForNode($node); - if ($node instanceof Node\Stmt\ClassMethod && $node->isPublic()) { + if ($node instanceof Node\Stmt\ClassMethod && $this->isServiceMethod($inside) && $node->isPublic()) { $docComment = $node->getDocComment(); if (empty($docComment)) { $this->emitMissingDocBlockError($fileName, $node, $inside); @@ -50,10 +53,25 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop } } + private function isServiceMethod(?ClassLike $inside = null) { + if ($inside instanceof Class_) { + $parentClass = $inside->extends?->toString(); + if (str_contains($parentClass, self::BASE_SERVICE)) { + return true; + } + if ($inside->extends instanceof Node\Name) { + $parentClass = $this->symbolTable->getClass($inside->extends); + return $this->isServiceMethod($parentClass); + } + } + + return false; + } + private function emitMissingDocBlockError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside): void { $this->emitErrorOnLine($fileName, $node->getLine(), ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, - "Method: {$node->name->name}, Class: {$inside->name->name} - All public Service methods must have a DocBlock." + "Method: {$node->name?->name}, Class: {$inside->name?->name} - All public Service methods must have a DocBlock." ); } @@ -97,6 +115,8 @@ private function validateParameter($actualParam, $docCommentParams, string $file $actualParamAttribute = $actualParam->getAttribute('inferrer-type'); if ($this->isComplexType($actualParamAttribute)) { $this->validateComplexType($actualParamAttribute->types, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); + } else if ($actualParamAttribute instanceof Node\NullableType) { + $this->validateNullableType($actualParamAttribute, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); } else { $actualParamType = $actualParamAttribute?->name ?? $actualParamAttribute?->toString(); $this->validateSimpleType($actualParamType, $docCommentParam['type'], $fileName, $node, $inside, $actualParamName); @@ -108,23 +128,64 @@ private function isComplexType($type): bool { } private function validateComplexType($actualParamTypes, $docCommentParamType, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { - if (!is_array($docCommentParamType) || count($actualParamTypes) !== count($docCommentParamType)) { - $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + $docCommentParamType = is_array($docCommentParamType) ? $docCommentParamType : [$docCommentParamType]; + + // Normalize doc comment types to handle nullable operator (?) + $normalizedDocCommentTypes = array_merge(...array_map(function ($type) { + $types = []; + if (str_starts_with($type, '?')) { + $types[] = 'null'; + $type = substr($type, 1); + } + if (str_ends_with($type, '[]')) { + $types[] = 'array'; + $type = substr($type, 0, -2); + } + $types[] = $type; + return $types; + }, $docCommentParamType)); + + if (count($actualParamTypes) !== count($normalizedDocCommentTypes)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName, 'Parameter count mismatch.'); return; } foreach ($actualParamTypes as $typeObject) { $actualParamType = $typeObject->name ?? $typeObject->toString(); - if (!in_array($actualParamType, $docCommentParamType) || in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { - $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + if (in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName, 'The following Types are not allowed: ' . implode(', ', self::NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES) . ', or null'); break; } + if (!in_array($actualParamType, $normalizedDocCommentTypes)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName, 'Complex Type Mismatch'); + break; + } + } + } + + private function validateNullableType(Node\NullableType $paramType, $docCommentParamTypes, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { + $actualType = $paramType->type->name ?? $paramType->type->toString(); + $allowedTypes = [$actualType, 'null', "?$actualType"]; + $docCommentParamTypes = is_array($docCommentParamTypes) ? $docCommentParamTypes : [$docCommentParamTypes]; + foreach ($docCommentParamTypes as $docCommentType) { + if (in_array($docCommentType, self::NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName, 'The following Types are not allowed: ' . implode(', ', self::NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES)); + } + if (!in_array($docCommentType, $allowedTypes)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName, 'Nullable Type Does Not Match DocBlock'); + } } } private function validateSimpleType($actualParamType, $docCommentParamType, string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $paramName): void { - if ($actualParamType !== $docCommentParamType || in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { - $this->emitTypeMismatchError($fileName, $node, $inside, $paramName); + if (is_array($docCommentParamType)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $paramName, 'Multiple DocBlock Param Types specified for only one actual param type.'); + } else if (($actualParamType === 'array' && str_ends_with($docCommentParamType, '[]') && !str_starts_with($docCommentParamType, '?'))) { + return; + } else if (in_array($actualParamType, self::BLOCKED_SERVICE_DOCUMENTATION_TYPES)) { + $this->emitTypeMismatchError($fileName, $node, $inside, $paramName, 'The following Types are not allowed: ' . implode(', ', self::NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES) . ', or null'); + } else if (($actualParamType !== $docCommentParamType && !str_ends_with($actualParamType, "\\$docCommentParamType"))) { + $this->emitTypeMismatchError($fileName, $node, $inside, $paramName, "DocBlock does not match method signature."); } } @@ -135,10 +196,14 @@ private function emitParameterMismatchError(string $fileName, Node\Stmt\ClassMet ); } - private function emitTypeMismatchError(string $fileName, Node\Stmt\ClassMethod $node, Node\Stmt\ClassLike $inside, string $propertyName): void { + private function emitTypeMismatchError( + string $fileName, Node\Stmt\ClassMethod $node, + Node\Stmt\ClassLike $inside, string $propertyName, + string $errorMessage + ): void { $this->emitErrorOnLine($fileName, $node->getLine(), ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, - "Method: {$node->name->name}, Class: {$inside->name->name}, Property: $propertyName - DocBlock does not match method signature." + "Method: {$node->name?->name}, Class: {$inside->name?->name}, Property: $propertyName - $errorMessage" ); } @@ -151,15 +216,22 @@ private function emitTypeMismatchError(string $fileName, Node\Stmt\ClassMethod $ * @return void */ private function validateReturnType(Node\Stmt\ClassMethod $node, $docCommentReturn, string $fileName, Node\Stmt\ClassLike $inside): void { + // return declarations on constructors are not allowed + if ($node->name->name === '__construct') { + return; + } + $propertyName = 'return'; if (!$docCommentReturn) { - $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName); + $this->emitTypeMismatchError($fileName, $node, $inside, $propertyName, "No Return Type Found"); return; } $actualReturn = $node->getReturnType(); if ($this->isComplexType($actualReturn)) { $this->validateComplexType($actualReturn->types, $docCommentReturn, $fileName, $node, $inside, $propertyName); + } else if ($actualReturn instanceof Node\NullableType) { + $this->validateNullableType($actualReturn, $docCommentReturn, $fileName, $node, $inside, $propertyName); } else { $this->validateSimpleType($actualReturn?->name ?? $actualReturn?->toString() ?? null, $docCommentReturn, $fileName, $node, $inside, $propertyName); } diff --git a/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.1.inc index 346ef3a..0f9cebc 100644 --- a/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.1.inc +++ b/tests/units/Checks/TestData/TestOpenApiAttributeDocumentationCheck.1.inc @@ -1,13 +1,18 @@ []])] - public function withEmptySearchPhrases() { + #[OA\Get(path: "/test", description: '')] + public function withEmptyDescription() { return false; } - #[OA\Get(path: "/test", x: ['vector-search-phrases' => ['my test search phrase']])] - public function withSearchPhrases() { + #[OA\Get(path: "/test", description: 'this is my great description')] + public function withDescription() { return false; } } \ No newline at end of file diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc index 6d8b51b..3e91cad 100644 --- a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.1.inc @@ -1,6 +1,10 @@ a = $a; + } } class MyObj { diff --git a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc index ebb7847..3e7e9c2 100644 --- a/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc +++ b/tests/units/Checks/TestData/TestServiceMethodDocumentationCheck.2.inc @@ -1,6 +1,9 @@ assertEquals(1, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_SEARCH_PHRASES_CHECK), ""); + $this->assertEquals(2, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK), ""); } } \ No newline at end of file diff --git a/tests/units/Checks/TestServiceMethodDocumentationCheck.php b/tests/units/Checks/TestServiceMethodDocumentationCheck.php index 0c74403..2f2a244 100644 --- a/tests/units/Checks/TestServiceMethodDocumentationCheck.php +++ b/tests/units/Checks/TestServiceMethodDocumentationCheck.php @@ -12,7 +12,7 @@ class TestServiceMethodDocumentationCheck extends TestSuiteSetup { * @return void */ public function testParametersSetAndUnset() { - $this->assertEquals(5, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + $this->assertEquals(6, $this->runAnalyzerOnFile('.1.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); } /** @@ -35,6 +35,6 @@ public function testSimpleTypesMatchingAndUnMatchingDocBlocks() { * @return void */ public function testComplexTypesMatchingAndUnMatchingDocBlocks() { - $this->assertEquals(13, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); + $this->assertEquals(15, $this->runAnalyzerOnFile('.4.inc', ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK), ""); } } \ No newline at end of file From 230d80bc07ed75410d583c46ac902ab403880599 Mon Sep 17 00:00:00 2001 From: bknutson Date: Fri, 24 Jan 2025 14:53:20 -0700 Subject: [PATCH 13/15] fix variableName SA error --- src/Checks/ServiceMethodDocumentationCheck.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 48aeb69..0d63e5d 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -249,9 +249,10 @@ private function extractDocCommentData(?string $docComment) { ]; if (preg_match_all('/@param\s+([^\s$]+(?:\s*[\|&]\s*[^\s$]+)*)?\s*(\$[^\s]+)?/', $docComment, $paramMatches, PREG_SET_ORDER)) { foreach ($paramMatches as $paramMatch) { + $variableName = ltrim($paramMatch[2] ?? null, '$'); $result['params'][$variableName] = [ 'type' => $this->extractDocGetVariableType($paramMatch[1] ?? null), - 'variable' => $variableName = ltrim($paramMatch[2] ?? null, '$'), + 'variable' => $variableName, ]; } } From bcd4ebb46b96d59c8c0e60e17113e1d1e11c8f13 Mon Sep 17 00:00:00 2001 From: bknutson Date: Fri, 24 Jan 2025 15:06:40 -0700 Subject: [PATCH 14/15] update method name --- src/Checks/OpenApiAttributeDocumentationCheck.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Checks/OpenApiAttributeDocumentationCheck.php b/src/Checks/OpenApiAttributeDocumentationCheck.php index d7e94ad..70c9353 100644 --- a/src/Checks/OpenApiAttributeDocumentationCheck.php +++ b/src/Checks/OpenApiAttributeDocumentationCheck.php @@ -38,7 +38,7 @@ function getCheckNodeTypes() { * @return void */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { - if ($node instanceof Node\Stmt\ClassMethod && $this->isControllerMethod($inside) && $node->isPublic() && $node->name->name !== '__construct') { + if ($node instanceof Node\Stmt\ClassMethod && $this->isControllerClass($inside) && $node->isPublic() && $node->name->name !== '__construct') { foreach ($node->attrGroups as $attrGroup) { foreach ($attrGroup->attrs as $attribute) { $attributeName = $attribute?->name?->toString(); @@ -71,7 +71,7 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop } } - private function isControllerMethod(ClassLike $inside = null): bool { + private function isControllerClass(ClassLike $inside = null): bool { if ($inside instanceof Class_) { $parentClass = $inside->extends?->toString(); if (str_contains($parentClass, self::BASE_CONTROLLER)) { @@ -79,7 +79,7 @@ private function isControllerMethod(ClassLike $inside = null): bool { } if ($inside->extends instanceof Node\Name) { $parentClass = $this->symbolTable->getClass($inside->extends); - return $this->isControllerMethod($parentClass); + return $this->isControllerClass($parentClass); } } From ca72a0cc0a77ce63491e470545e3056b2ab2f3e9 Mon Sep 17 00:00:00 2001 From: bknutson Date: Fri, 24 Jan 2025 15:19:20 -0700 Subject: [PATCH 15/15] update method name --- src/Checks/ServiceMethodDocumentationCheck.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Checks/ServiceMethodDocumentationCheck.php b/src/Checks/ServiceMethodDocumentationCheck.php index 0d63e5d..d8c8881 100644 --- a/src/Checks/ServiceMethodDocumentationCheck.php +++ b/src/Checks/ServiceMethodDocumentationCheck.php @@ -37,7 +37,7 @@ function getCheckNodeTypes() { */ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { $this->emitMetricsForNode($node); - if ($node instanceof Node\Stmt\ClassMethod && $this->isServiceMethod($inside) && $node->isPublic()) { + if ($node instanceof Node\Stmt\ClassMethod && $this->isServiceClass($inside) && $node->isPublic()) { $docComment = $node->getDocComment(); if (empty($docComment)) { $this->emitMissingDocBlockError($fileName, $node, $inside); @@ -53,7 +53,7 @@ public function run($fileName, Node $node, ClassLike $inside = null, Scope $scop } } - private function isServiceMethod(?ClassLike $inside = null) { + private function isServiceClass(?ClassLike $inside = null) { if ($inside instanceof Class_) { $parentClass = $inside->extends?->toString(); if (str_contains($parentClass, self::BASE_SERVICE)) { @@ -61,7 +61,7 @@ private function isServiceMethod(?ClassLike $inside = null) { } if ($inside->extends instanceof Node\Name) { $parentClass = $this->symbolTable->getClass($inside->extends); - return $this->isServiceMethod($parentClass); + return $this->isServiceClass($parentClass); } }