-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPN-1438] Guardrail to add deprecated metric for class methods in the codebase #124
Merged
bknutson123
merged 20 commits into
master
from
BK/integators/SPN-1438/guardrailDeprecatedMetric
Jan 27, 2025
Merged
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
7ace23a
[SPN-1438] Guardrail to add deprecated metric for class methods in th…
bknutson123 0514e18
simplify enter method in FunctionLike Evaluator
bknutson123 b704af0
remove unused class property
bknutson123 f4798fc
[SPN-1440] New Guardrail Check to ensure documentation is present and…
bknutson123 05d9571
fix test by ignoring documentation requirement
bknutson123 e765002
[SPN-1447] Start of Service Documentation Guardrail
bknutson123 e7a07a2
add search phrases check
bknutson123 17a8c05
Merge branch 'refs/heads/BK/integators/SPN-1438/guardrailDeprecatedMe…
bknutson123 34ba8b1
[SPN-1448] Refactor Service Method Documentation Check and add checks…
bknutson123 9c8d4ac
Merge pull request #126 from BambooHR/BK/integators/SPN-1447/strongTy…
bknutson123 4bbf82e
move metric emit from evaluator into the new ServiceMethodDocumentati…
bknutson123 64eaa72
Merge branch 'refs/heads/BK/integators/SPN-1438/guardrailDeprecatedMe…
bknutson123 e88a91c
rename check to open api attribute documentation instead of web api d…
bknutson123 40ce6a4
Merge pull request #125 from BambooHR/BK/integators/SPN-1440/deprecat…
bknutson123 a2d7534
handle both methods and functions
bknutson123 7b4bedb
Merge pull request #127 from BambooHR/BK/integators/SPN-1440/deprecat…
bknutson123 36130e4
* remove search phrases requirement in favor of descriptions
bknutson123 230d80b
fix variableName SA error
bknutson123 bcd4ebb
update method name
bknutson123 ca72a0c
update method name
bknutson123 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
<?php | ||
|
||
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\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 DESCRIPTION_KEY = 'description'; | ||
private const string BASE_CONTROLLER = 'BaseController'; | ||
|
||
function __construct($index, $output, private readonly MetricOutputInterface $metricOutput) { | ||
parent::__construct($index, $output); | ||
} | ||
|
||
/** | ||
* getCheckNodeTypes | ||
* | ||
* @return string[] | ||
*/ | ||
function getCheckNodeTypes() { | ||
return [Node\Stmt\ClassMethod::class]; | ||
} | ||
|
||
/** | ||
* @param string $fileName The name of the file we are parsing | ||
* @param Node $node Instance of the Node | ||
* @param ClassLike|null $inside Instance of the ClassLike (the class we are parsing) [optional] | ||
* @param Scope|null $scope Instance of the Scope (all variables in the current state) [optional] | ||
* | ||
* @return void | ||
*/ | ||
public function run($fileName, Node $node, ClassLike $inside = null, Scope $scope = null) { | ||
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(); | ||
if (str_starts_with($attributeName, self::ATTRIBUTE_NAMESPACE)) { | ||
$hasDescription = false; | ||
foreach ($attribute->args as $arg) { | ||
$this->checkDeprecatedAttribute($arg, $fileName, $node); | ||
$hasDescription = $hasDescription ?: $this->hasDescription($arg); | ||
} | ||
if (!$hasDescription) { | ||
$this->emitErrorOnLine( | ||
$fileName, | ||
$node->getLine(), | ||
ErrorConstants::TYPE_OPEN_API_ATTRIBUTE_DOCUMENTATION_CHECK, | ||
"OpenAPI Attribute must have a description. Method: {$node->name->name}" | ||
); | ||
} | ||
return; | ||
} | ||
} | ||
} | ||
$className = $inside?->namespacedName?->toString(); | ||
$this->emitErrorOnLine( | ||
$fileName, | ||
$node->getLine(), | ||
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" | ||
); | ||
} | ||
} | ||
|
||
private function isControllerClass(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->isControllerClass($parentClass); | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
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 hasDescription($arg): bool { | ||
return $arg->name->name === self::DESCRIPTION_KEY && !empty($arg->value->value); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,276 @@ | ||
<?php | ||
|
||
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\Class_; | ||
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, ...self::NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES]; | ||
private const array NULLABLE_BLOCKED_SERVICE_DOCUMENTATION_TYPES = ['mixed', 'object']; | ||
private const string BASE_SERVICE = 'BaseService'; | ||
/** | ||
* getCheckNodeTypes | ||
* | ||
* @return string[] | ||
*/ | ||
function getCheckNodeTypes() { | ||
return [Node\Stmt\ClassMethod::class, Node\Stmt\Function_::class]; | ||
} | ||
|
||
/** | ||
* @param string $fileName The name of the file we are parsing | ||
* @param Node $node Instance of the Node | ||
* @param ClassLike|null $inside Instance of the ClassLike (the class we are parsing) [optional] | ||
* @param Scope|null $scope Instance of the Scope (all variables in the current state) [optional] | ||
* | ||
* @return void | ||
*/ | ||
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()) { | ||
$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 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." | ||
); | ||
} | ||
|
||
/** | ||
* @param Node\Stmt\ClassMethod $node | ||
* | ||
* @return void | ||
*/ | ||
private function emitMetricsForNode($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(), | ||
ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, | ||
"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 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 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); | ||
} | ||
} | ||
|
||
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 { | ||
$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, 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 (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."); | ||
} | ||
} | ||
|
||
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, | ||
string $errorMessage | ||
): void { | ||
$this->emitErrorOnLine($fileName, $node->getLine(), | ||
ErrorConstants::TYPE_SERVICE_METHOD_DOCUMENTATION_CHECK, | ||
"Method: {$node->name?->name}, Class: {$inside->name?->name}, Property: $propertyName - $errorMessage" | ||
); | ||
} | ||
|
||
/** | ||
* @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 { | ||
// return declarations on constructors are not allowed | ||
if ($node->name->name === '__construct') { | ||
return; | ||
} | ||
|
||
$propertyName = 'return'; | ||
if (!$docCommentReturn) { | ||
$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); | ||
} | ||
} | ||
|
||
/** | ||
* @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) { | ||
$variableName = ltrim($paramMatch[2] ?? null, '$'); | ||
$result['params'][$variableName] = [ | ||
'type' => $this->extractDocGetVariableType($paramMatch[1] ?? null), | ||
'variable' => $variableName, | ||
]; | ||
} | ||
} | ||
|
||
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; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also
isServiceClass()
, not method.