diff --git a/.twig-cs-fixer.php b/.twig-cs-fixer.php index 5359cf0..20110fa 100644 --- a/.twig-cs-fixer.php +++ b/.twig-cs-fixer.php @@ -3,6 +3,7 @@ declare(strict_types=1); use Ruudk\GraphQLCodeGenerator\Twig\GraphQLFormatterRule; +use Ruudk\GraphQLCodeGenerator\Twig\GraphQLOverfetchingRule; use Ruudk\GraphQLCodeGenerator\Twig\GraphQLTokenParser; use TwigCsFixer\Config\Config; use TwigCsFixer\File\Finder; @@ -13,6 +14,7 @@ $ruleset->addStandard(new TwigCsFixer()); $ruleset->addRule(new GraphQLFormatterRule()); +$ruleset->addRule(new GraphQLOverfetchingRule()); $finder = Finder::create() ->in('tests'); diff --git a/README.md b/README.md index dba32b0..a3fabf4 100644 --- a/README.md +++ b/README.md @@ -657,6 +657,7 @@ Register `GraphQLFormatterRule` (and the `GraphQLTokenParser` so Twig CS Fixer u declare(strict_types=1); use Ruudk\GraphQLCodeGenerator\Twig\GraphQLFormatterRule; +use Ruudk\GraphQLCodeGenerator\Twig\GraphQLOverfetchingRule; use Ruudk\GraphQLCodeGenerator\Twig\GraphQLTokenParser; use TwigCsFixer\Config\Config; use TwigCsFixer\File\Finder; @@ -667,6 +668,7 @@ $ruleset = new Ruleset(); $ruleset->addStandard(new TwigCsFixer()); $ruleset->addRule(new GraphQLFormatterRule()); +$ruleset->addRule(new GraphQLOverfetchingRule()); $finder = Finder::create() ->in('tests'); diff --git a/src/Twig/GraphQLOverfetchingRule.php b/src/Twig/GraphQLOverfetchingRule.php new file mode 100644 index 0000000..5562940 --- /dev/null +++ b/src/Twig/GraphQLOverfetchingRule.php @@ -0,0 +1,60 @@ + + */ + private array $analyzed = []; + + public function __construct( + ?OverfetchingAnalyzer $analyzer = null, + ) { + $this->analyzer = $analyzer ?? new OverfetchingAnalyzer(); + } + + #[Override] + protected function process(int $tokenIndex, Tokens $tokens) : void + { + $token = $tokens->get($tokenIndex); + + if ( ! $token->isMatching(Token::BLOCK_NAME_TYPE, 'types')) { + return; + } + + $fileName = $token->getFilename(); + + if (isset($this->analyzed[$fileName])) { + return; + } + + $this->analyzed[$fileName] = true; + + $source = @file_get_contents($fileName); + + if ($source === false) { + return; + } + + foreach ($this->analyzer->analyze($source, $fileName) as $message) { + $this->addError($message, $token); + } + } +} diff --git a/src/Twig/Overfetching/Binding.php b/src/Twig/Overfetching/Binding.php new file mode 100644 index 0000000..d4de1f2 --- /dev/null +++ b/src/Twig/Overfetching/Binding.php @@ -0,0 +1,31 @@ +fqcn, false); + } +} diff --git a/src/Twig/Overfetching/OverfetchingAnalyzer.php b/src/Twig/Overfetching/OverfetchingAnalyzer.php new file mode 100644 index 0000000..d6efe77 --- /dev/null +++ b/src/Twig/Overfetching/OverfetchingAnalyzer.php @@ -0,0 +1,357 @@ + + */ + private array $used = []; + + public function __construct( + private readonly ShapeReader $reader = new ShapeReader(), + ) {} + + /** + * @return list + */ + public function analyze(string $source, string $fileName) : array + { + $this->used = []; + + try { + $environment = new Environment(new ArrayLoader(), [ + 'cache' => false, + 'autoescape' => false, + 'optimizations' => 0, + 'strict_variables' => false, + ]); + $environment->addExtension(new GraphQLExtension()); + + $module = $environment->parse( + $environment->tokenize(new Source($source, $fileName)), + ); + } catch (Throwable) { + return []; + } + + $typesNode = $this->findTypesNode($module); + + if ($typesNode === null) { + return []; + } + + /** + * @var array $mapping + */ + $mapping = $typesNode->getAttribute('mapping'); + + $scope = new Scope(); + $roots = []; + + foreach ($mapping as $name => $definition) { + $fqcn = ltrim($definition['type'], '\\'); + + if ($this->reader->read($fqcn) === null) { + continue; + } + + $scope = $scope->with($name, new Binding($fqcn)); + $roots[$name] = $fqcn; + } + + if ($roots === []) { + return []; + } + + $this->walk($module, $scope); + + $messages = []; + + foreach ($roots as $name => $fqcn) { + $this->report($fqcn, $name, [], $messages); + } + + return $messages; + } + + private function findTypesNode(Node $node) : ?TypesNode + { + if ($node instanceof TypesNode) { + return $node; + } + + foreach ($node as $child) { + $found = $this->findTypesNode($child); + + if ($found !== null) { + return $found; + } + } + + return null; + } + + /** + * Walks the tree in order, returning the (possibly extended) scope so a + * `{% set %}` is visible to its following siblings. + */ + private function walk(Node $node, Scope $scope) : Scope + { + if ($node instanceof TypesNode || $node instanceof GraphQLNode) { + return $scope; + } + + if ($node instanceof NameExpression) { + // A bare variable: the whole object escapes (printed, passed to a + // function/macro/include). We cannot trace further, so everything + // below it counts as used. + $binding = $scope->get($this->nameOf($node)); + + if ($binding->fqcn !== null) { + $this->markAllUsed($binding->fqcn, []); + } + + return $scope; + } + + if ($node instanceof GetAttrExpression) { + $this->resolveChain($node, $scope); + + return $scope; + } + + if ($node instanceof ForNode) { + $sequence = $this->typeOf($node->getNode('seq'), $scope); + + $body = $scope + ->with($this->nameOf($node->getNode('value_target')), $sequence->element()) + ->with($this->nameOf($node->getNode('key_target')), Binding::unknown()) + ->with('loop', Binding::unknown()); + + $this->walk($node->getNode('body'), $body); + + if ($node->hasNode('else')) { + $this->walk($node->getNode('else'), $scope); + } + + return $scope; + } + + if ($node instanceof SetNode && ! $node->getAttribute('capture')) { + $names = $node->getNode('names'); + $values = $node->getNode('values'); + + $nameList = array_values(iterator_to_array($names)); + $valueList = array_values(iterator_to_array($values)); + + if (count($nameList) === 1 && count($valueList) === 1) { + return $scope->with( + $this->nameOf($nameList[0]), + $this->typeOf($valueList[0], $scope), + ); + } + + foreach ($values as $value) { + $this->walk($value, $scope); + } + + return $scope; + } + + $childScope = $scope; + + foreach ($node as $child) { + $childScope = $this->walk($child, $childScope); + } + + return $scope; + } + + private function nameOf(Node $node) : string + { + $name = $node->hasAttribute('name') ? $node->getAttribute('name') : null; + + return is_string($name) ? $name : ''; + } + + /** + * Resolves the type an expression evaluates to, recording any attribute + * accesses it performs as used along the way. + */ + private function typeOf(Node $node, Scope $scope) : Binding + { + if ($node instanceof NameExpression) { + return $scope->get($this->nameOf($node)); + } + + if ($node instanceof GetAttrExpression) { + return $this->resolveChain($node, $scope); + } + + $this->walk($node, $scope); + + return Binding::unknown(); + } + + private function resolveChain(GetAttrExpression $node, Scope $scope) : Binding + { + $steps = []; + $current = $node; + + while ($current instanceof GetAttrExpression) { + $attribute = $current->getNode('attribute'); + + $value = $attribute instanceof ConstantExpression + ? $attribute->getAttribute('value') + : null; + + $steps[] = is_string($value) ? $value : null; + + $current = $current->getNode('node'); + } + + $steps = array_reverse($steps); + + $binding = $this->typeOf($current, $scope); + $fqcn = $binding->fqcn; + + foreach ($steps as $step) { + if ($fqcn === null) { + return Binding::unknown(); + } + + if ($step === null) { + // Dynamic access (`foo[bar]`): we cannot tell which field is + // read, so conservatively treat the whole subtree as used. + $this->markAllUsed($fqcn, []); + + return Binding::unknown(); + } + + $shape = $this->reader->read($fqcn); + + if ($shape === null || ! isset($shape->properties[$step])) { + return Binding::unknown(); + } + + $property = $shape->properties[$step]; + $this->used[$fqcn . "\0" . $step] = true; + + $fqcn = $property->targetFqcn; + + if ($step === $steps[array_key_last($steps)]) { + return new Binding($fqcn, $property->list); + } + } + + return new Binding($fqcn); + } + + /** + * @param list $visited + */ + private function markAllUsed(string $fqcn, array $visited) : void + { + if (in_array($fqcn, $visited, true)) { + return; + } + + $shape = $this->reader->read($fqcn); + + if ($shape === null) { + return; + } + + $visited[] = $fqcn; + + foreach ($shape->properties as $property) { + $this->used[$fqcn . "\0" . $property->name] = true; + + if ($property->targetFqcn === null) { + continue; + } + + $target = $this->reader->read($property->targetFqcn); + + if ($target !== null && $target->source === $shape->source) { + $this->markAllUsed($property->targetFqcn, $visited); + } + } + } + + /** + * @param list $visited + * @param list $messages + */ + private function report(string $fqcn, string $path, array $visited, array &$messages) : void + { + if (in_array($fqcn, $visited, true)) { + return; + } + + $shape = $this->reader->read($fqcn); + + if ($shape === null) { + return; + } + + $visited[] = $fqcn; + + foreach ($shape->properties as $property) { + if ( ! $property->fetchedField || $property->api) { + continue; + } + + $childPath = $path . '.' . $property->name; + + if ( ! isset($this->used[$fqcn . "\0" . $property->name])) { + $messages[] = sprintf( + '`%s` is fetched in the GraphQL operation but never used in the template (overfetching).', + $childPath, + ); + + continue; + } + + if ($property->targetFqcn === null) { + continue; + } + + $target = $this->reader->read($property->targetFqcn); + + // Only recurse into nested object selections that belong to the + // same template. A fragment spread points at a different template + // and is validated when that template is linted. + if ($target !== null && $target->source === $shape->source) { + $this->report($property->targetFqcn, $childPath, $visited, $messages); + } + } + } +} diff --git a/src/Twig/Overfetching/Scope.php b/src/Twig/Overfetching/Scope.php new file mode 100644 index 0000000..9cf9061 --- /dev/null +++ b/src/Twig/Overfetching/Scope.php @@ -0,0 +1,33 @@ + $bindings + */ + public function __construct( + private array $bindings = [], + ) {} + + public function with(string $name, Binding $binding) : self + { + $bindings = $this->bindings; + $bindings[$name] = $binding; + + return new self($bindings); + } + + public function get(string $name) : Binding + { + return $this->bindings[$name] ?? Binding::unknown(); + } +} diff --git a/src/Twig/Overfetching/Shape.php b/src/Twig/Overfetching/Shape.php new file mode 100644 index 0000000..7bb81ac --- /dev/null +++ b/src/Twig/Overfetching/Shape.php @@ -0,0 +1,27 @@ + + */ + public array $properties, + ) {} +} diff --git a/src/Twig/Overfetching/ShapeProperty.php b/src/Twig/Overfetching/ShapeProperty.php new file mode 100644 index 0000000..1b42b4a --- /dev/null +++ b/src/Twig/Overfetching/ShapeProperty.php @@ -0,0 +1,39 @@ +data` are real fields; derived + * helpers like `*OrThrow` or `is` reference other properties + * and never cause any extra data to be fetched. + */ + public bool $fetchedField, + /** + * The FQCN of the generated class this property resolves to (an object + * selection or a fragment spread), or `null` for scalar/enum leaves. + */ + public ?string $targetFqcn, + /** + * Whether the property is a list (`list`). Relevant when the + * property is iterated with `{% for %}`. + */ + public bool $list, + ) {} +} diff --git a/src/Twig/Overfetching/ShapeReader.php b/src/Twig/Overfetching/ShapeReader.php new file mode 100644 index 0000000..3f1a44e --- /dev/null +++ b/src/Twig/Overfetching/ShapeReader.php @@ -0,0 +1,303 @@ +data`), whether it is tagged `@api`, and the + * generated class it resolves to. + */ +final class ShapeReader +{ + private readonly Parser $parser; + private readonly NodeFinder $finder; + + /** + * @var array + */ + private array $cache = []; + + public function __construct() + { + $this->parser = new ParserFactory()->createForNewestSupportedVersion(); + $this->finder = new NodeFinder(); + } + + public function read(string $fqcn) : ?Shape + { + $fqcn = ltrim($fqcn, '\\'); + + if (array_key_exists($fqcn, $this->cache)) { + return $this->cache[$fqcn]; + } + + return $this->cache[$fqcn] = $this->doRead($fqcn); + } + + private function doRead(string $fqcn) : ?Shape + { + if ( ! class_exists($fqcn)) { + return null; + } + + try { + $fileName = new ReflectionClass($fqcn)->getFileName(); + } catch (Throwable) { + return null; + } + + if ($fileName === false || ! is_file($fileName)) { + return null; + } + + $code = @file_get_contents($fileName); + + if ($code === false) { + return null; + } + + try { + $stmts = $this->parser->parse($code); + } catch (Throwable) { + return null; + } + + if ($stmts === null) { + return null; + } + + $stmts = new NodeTraverser(new NameResolver())->traverse($stmts); + + $useMap = $this->collectUseMap($stmts); + + $class = $this->finder->findFirstInstanceOf($stmts, Class_::class); + + if ( ! $class instanceof Class_) { + return null; + } + + $namespace = $this->namespaceOf($fqcn); + $source = $this->readGeneratedSource($class); + + $properties = []; + + foreach ($class->getProperties() as $property) { + if ( ! $property->isPublic() || $property->isStatic()) { + continue; + } + + foreach ($property->props as $prop) { + $name = $prop->name->toString(); + + [$targetFqcn, $isList] = $this->resolveType($property, $useMap, $namespace); + + $properties[$name] = new ShapeProperty( + $name, + $this->hasApiTag($property), + $this->getterReadsData($property), + $targetFqcn, + $isList, + ); + } + } + + return new Shape($fqcn, $source, $properties); + } + + /** + * @param array $stmts + * @return array + */ + private function collectUseMap(array $stmts) : array + { + $map = []; + + foreach ($this->finder->findInstanceOf($stmts, Use_::class) as $use) { + if ($use->type !== Use_::TYPE_NORMAL) { + continue; + } + + foreach ($use->uses as $useUse) { + $fqcn = $useUse->name->toString(); + $alias = $useUse->alias?->toString() ?? $useUse->name->getLast(); + $map[$alias] = $fqcn; + } + } + + return $map; + } + + private function readGeneratedSource(Class_ $class) : ?string + { + foreach ($class->attrGroups as $group) { + foreach ($group->attrs as $attr) { + if ($attr->name->getLast() !== 'Generated') { + continue; + } + + foreach ($attr->args as $arg) { + $isSource = $arg->name === null || $arg->name->toString() === 'source'; + + if ( ! $isSource) { + continue; + } + + $value = $arg->value; + + if ($value instanceof Node\Scalar\String_) { + return $value->value; + } + } + } + } + + return null; + } + + private function hasApiTag(Property $property) : bool + { + $doc = $property->getDocComment()?->getText(); + + if ($doc === null) { + return false; + } + + return preg_match('/^\s*\*?\s*@api\b/m', $doc) === 1; + } + + private function getterReadsData(Property $property) : bool + { + foreach ($property->hooks as $hook) { + if ($hook->name->toString() !== 'get') { + continue; + } + + $body = $hook->body; + + if ($body === null) { + continue; + } + + $nodes = $body instanceof Node ? [$body] : $body; + + $found = $this->finder->findFirst($nodes, static function (Node $node) : bool { + return $node instanceof PropertyFetch + && $node->var instanceof Variable + && $node->var->name === 'this' + && $node->name instanceof Identifier + && $node->name->toString() === 'data'; + }); + + if ($found !== null) { + return true; + } + } + + return false; + } + + /** + * @param array $useMap + * @return array{0: ?string, 1: bool} + */ + private function resolveType(Property $property, array $useMap, string $namespace) : array + { + $type = $property->type; + + if ($type instanceof NullableType) { + $type = $type->type; + } + + if ($type instanceof UnionType) { + foreach ($type->types as $member) { + if ($member instanceof Name) { + return [$member->toString(), false]; + } + } + + return [null, false]; + } + + if ($type instanceof Name) { + return [$type->toString(), false]; + } + + if ($type instanceof Identifier && in_array($type->toString(), ['array', 'iterable'], true)) { + $element = $this->elementFromVar($property, $useMap, $namespace); + + return [$element, $element !== null]; + } + + return [null, false]; + } + + /** + * @param array $useMap + */ + private function elementFromVar(Property $property, array $useMap, string $namespace) : ?string + { + $doc = $property->getDocComment()?->getText(); + + if ($doc === null) { + return null; + } + + if (preg_match('/@var\s+(?:list|array|iterable|non-empty-list)\s*<\s*(?:[^,>]+,\s*)?\\\\?([A-Za-z_][\w\\\\]*)\s*>/', $doc, $m) === 1) { + return $this->resolveClassName($m[1], $useMap, $namespace); + } + + if (preg_match('/@var\s+\\\\?([A-Za-z_][\w\\\\]*)\s*\[\s*\]/', $doc, $m) === 1) { + return $this->resolveClassName($m[1], $useMap, $namespace); + } + + return null; + } + + /** + * @param array $useMap + */ + private function resolveClassName(string $name, array $useMap, string $namespace) : ?string + { + if (str_contains($name, '\\')) { + return ltrim($name, '\\'); + } + + if (isset($useMap[$name])) { + return $useMap[$name]; + } + + if (in_array($name, ['string', 'int', 'float', 'bool', 'mixed', 'array', 'object', 'null'], true)) { + return null; + } + + return $namespace === '' ? $name : $namespace . '\\' . $name; + } + + private function namespaceOf(string $fqcn) : string + { + $position = strrpos($fqcn, '\\'); + + return $position === false ? '' : substr($fqcn, 0, $position); + } +} diff --git a/tests/GraphQLOverfetchingRule/GraphQLOverfetchingRuleTest.php b/tests/GraphQLOverfetchingRule/GraphQLOverfetchingRuleTest.php new file mode 100644 index 0000000..7bdc4f4 --- /dev/null +++ b/tests/GraphQLOverfetchingRule/GraphQLOverfetchingRuleTest.php @@ -0,0 +1,62 @@ +lint(self::TEMPLATES . '/no_overfetching.html.twig.fixture')->getViolations(); + + self::assertSame([], $violations); + } + + public function testReportsUnusedTopLevelField() : void + { + $violations = $this->lint(self::TEMPLATES . '/overfetching.html.twig.fixture')->getViolations(); + + self::assertCount(1, $violations); + self::assertSame(Violation::LEVEL_ERROR, $violations[0]->getLevel()); + self::assertSame( + '`data.viewer` is fetched in the GraphQL operation but never used in the template (overfetching).', + $violations[0]->getMessage(), + ); + } + + public function testReportsUnusedNestedField() : void + { + $violations = $this->lint(self::TEMPLATES . '/overfetching_nested.html.twig.fixture')->getViolations(); + + self::assertCount(1, $violations); + self::assertSame(Violation::LEVEL_ERROR, $violations[0]->getLevel()); + self::assertSame( + '`data.viewer.name` is fetched in the GraphQL operation but never used in the template (overfetching).', + $violations[0]->getMessage(), + ); + } + + private function lint(string $filePath) : Report + { + $env = new StubbedEnvironment(); + $linter = new Linter($env, new Tokenizer($env)); + + $ruleset = new Ruleset(); + $ruleset->addRule(new GraphQLOverfetchingRule()); + + return $linter->run([new SplFileInfo($filePath)], $ruleset); + } +} diff --git a/tests/GraphQLOverfetchingRule/templates/no_overfetching.html.twig.fixture b/tests/GraphQLOverfetchingRule/templates/no_overfetching.html.twig.fixture new file mode 100644 index 0000000..dc161fb --- /dev/null +++ b/tests/GraphQLOverfetchingRule/templates/no_overfetching.html.twig.fixture @@ -0,0 +1,22 @@ +{% types { + data: '\\Ruudk\\GraphQLCodeGenerator\\Twig\\Generated\\Fragment\\AdminProjectList', +} %} + +{% graphql %} +fragment AdminProjectList on Query { + viewer { + name + } + projects { + ...AdminProjectRow + } +} +{% endgraphql %} + +

Good day, {{ data.viewer.name }}

+ +

Your projects

+ +{% for project in data.projects %} + {{ include('_project_row.html.twig', {project: project.adminProjectRow}) }} +{% endfor %} diff --git a/tests/GraphQLOverfetchingRule/templates/overfetching.html.twig.fixture b/tests/GraphQLOverfetchingRule/templates/overfetching.html.twig.fixture new file mode 100644 index 0000000..d8e5091 --- /dev/null +++ b/tests/GraphQLOverfetchingRule/templates/overfetching.html.twig.fixture @@ -0,0 +1,20 @@ +{% types { + data: '\\Ruudk\\GraphQLCodeGenerator\\Twig\\Generated\\Fragment\\AdminProjectList', +} %} + +{% graphql %} +fragment AdminProjectList on Query { + viewer { + name + } + projects { + ...AdminProjectRow + } +} +{% endgraphql %} + +

Your projects

+ +{% for project in data.projects %} + {{ include('_project_row.html.twig', {project: project.adminProjectRow}) }} +{% endfor %} diff --git a/tests/GraphQLOverfetchingRule/templates/overfetching_nested.html.twig.fixture b/tests/GraphQLOverfetchingRule/templates/overfetching_nested.html.twig.fixture new file mode 100644 index 0000000..6d772a8 --- /dev/null +++ b/tests/GraphQLOverfetchingRule/templates/overfetching_nested.html.twig.fixture @@ -0,0 +1,22 @@ +{% types { + data: '\\Ruudk\\GraphQLCodeGenerator\\Twig\\Generated\\Fragment\\AdminProjectList', +} %} + +{% graphql %} +fragment AdminProjectList on Query { + viewer { + name + } + projects { + ...AdminProjectRow + } +} +{% endgraphql %} + +{% if data.viewer %} +

Good day

+{% endif %} + +{% for project in data.projects %} + {{ include('_project_row.html.twig', {project: project.adminProjectRow}) }} +{% endfor %}