Merge pull request #968 from bravik/fix_validator_namespace_collisions

Fix validation constraints namespace conflicts in generated definition files

Author: mcg-web

src/Generator/TypeBuilder.php

@@ -44,7 +44,6 @@
 use function reset;
 use function rtrim;
 use function strpos;
-use function strrchr;
 use function strtolower;
 use function substr;
 
@@ -617,20 +616,18 @@ protected function buildConstraints(array $constraints = [], bool $inClosure = t
             if (false !== strpos($name, '\\')) {
                 // Custom constraint
                 $fqcn = ltrim($name, '\\');
-                $name = ltrim(strrchr($name, '\\'), '\\');
-                $this->file->addUse($fqcn);
+                $instance = Instance::new("@\\$fqcn");
             } else {
                 // Symfony constraint
-                $this->file->addUseGroup(static::CONSTRAINTS_NAMESPACE, $name);
                 $fqcn = static::CONSTRAINTS_NAMESPACE."\\$name";
+                $this->file->addUse(static::CONSTRAINTS_NAMESPACE.' as SymfonyConstraints');
+                $instance = Instance::new("@SymfonyConstraints\\$name");
             }
 
             if (!class_exists($fqcn)) {
                 throw new GeneratorException("Constraint class '$fqcn' doesn't exist.");
             }
 
-            $instance = Instance::new($name);
-
             if (is_array($args)) {
                 if (isset($args[0]) && is_array($args[0])) {
                     // Nested instance

tests/Functional/App/Validator/CustomValidator1/Constraint.php

@@ -0,0 +1,28 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Overblog\GraphQLBundle\Tests\Functional\App\Validator\CustomValidator1;
+
+use Overblog\GraphQLBundle\Tests\Functional\App\Validator\MockValidator;
+use Symfony\Component\Validator\Constraint as BaseConstraint;
+
+/**
+ * This and CustomValidator2/Constraint should be named same,
+ * to test that generated type class doesn't include them both into use statement,
+ * which produced a namespace conflict
+ *
+ * @Annotation
+ */
+class Constraint extends BaseConstraint
+{
+    public string $message = 'Mock constraint';
+
+    /**
+     * @return class-string
+     */
+    public function validatedBy(): string
+    {
+        return MockValidator::class;
+    }
+}

tests/Functional/App/Validator/CustomValidator2/Constraint.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Overblog\GraphQLBundle\Tests\Functional\App\Validator\CustomValidator2;
+
+use Overblog\GraphQLBundle\Tests\Functional\App\Validator\MockValidator;
+use Symfony\Component\Validator\Constraint as BaseConstraint;
+
+/**
+ * @Annotation
+ */
+class Constraint extends BaseConstraint
+{
+    public string $message = 'Mock constraint';
+
+    /**
+     * @return class-string
+     */
+    public function validatedBy(): string
+    {
+        return MockValidator::class;
+    }
+}

tests/Functional/App/Validator/MockValidator.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Overblog\GraphQLBundle\Tests\Functional\App\Validator;
+
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+
+/**
+ * Just a dummy validator
+ */
+class MockValidator extends ConstraintValidator
+{
+    public function validate($value, Constraint $constraint): void
+    {
+    }
+}

tests/Functional/App/config/conflictingValidatorNamespaces/config.yml

@@ -0,0 +1,25 @@
+imports:
+    - { resource: ../config.yml }
+
+framework:
+    annotations: true
+    validation:
+        enabled: true
+        enable_annotations: true
+
+overblog_graphql:
+    definitions:
+        config_validation: false
+        class_namespace: "Overblog\\GraphQLBundle\\Validator\\__DEFINITIONS__"
+        schema:
+            query: Mutation
+            mutation: Mutation
+        mappings:
+            types:
+                - type: yaml
+                  dir: "%kernel.project_dir%/config/conflictingValidatorNamespaces/mapping"
+
+services:
+    Overblog\GraphQLBundle\Tests\Functional\App\Mutation\InputValidatorMutation:
+        tags:
+            - { name: "overblog_graphql.mutation", alias: "mutation_mock", method: "mutationMock" }

tests/Functional/App/config/conflictingValidatorNamespaces/mapping/Mutation.types.yml

@@ -0,0 +1,27 @@
+Mutation:
+    type: object
+    config:
+        fields:
+            conflictingValidatorNamespaces:
+                type: Boolean
+                resolve: '@=m("mutation_mock", args, validator)'
+                args:
+                    # Covers case where Symfony's Symfony\Component\Validator\Constraints\Type constraint
+                    # conflicts with GraphQL\Type\Definition\Type
+                    # in generated definition file
+                    test:
+                        type: "String"
+                        validation:
+                            -   Type:
+                                    type: numeric
+                    # Following two args cover the case, where custom constraints with same class name
+                    # but different FQCN conflict with each other
+                    # in generated definition file
+                    test2:
+                        type: "String"
+                        validation:
+                            -   Overblog\GraphQLBundle\Tests\Functional\App\Validator\CustomValidator1\Constraint: ~
+                    test3:
+                        type: "String"
+                        validation:
+                            -   Overblog\GraphQLBundle\Tests\Functional\App\Validator\CustomValidator2\Constraint: ~

tests/Functional/Generator/TypeGeneratorTest.php

@@ -5,7 +5,9 @@
 namespace Overblog\GraphQLBundle\Tests\Functional\Generator;
 
 use Overblog\GraphQLBundle\Generator\Exception\GeneratorException;
+use Overblog\GraphQLBundle\Tests\Functional\App\Validator;
 use Overblog\GraphQLBundle\Tests\Functional\TestCase;
+use Symfony\Component\Validator\Validation;
 use function json_decode;
 
 class TypeGeneratorTest extends TestCase
@@ -94,4 +96,58 @@ public function testInjectValidatorWithoutConstraintsThrowsException(): void
         parent::setUp();
         static::bootKernel(['test_case' => 'validatorWithoutConstraints']);
     }
+
+    /**
+     * In generated type definitions:
+     *  1. Custom constraints should be used by FQCN, to ensure no namespace conflicts occur
+     *  2. Default Symfony validators should be used with aliased namespace to ensure that no
+     *     namespace conflicts occur with Graphql bundle classes (Type for example).
+     *
+     * @runInSeparateProcess
+     */
+    public function testConflictingValidatorNamespaces(): void
+    {
+        if (!class_exists(Validation::class)) {
+            $this->markTestSkipped('Symfony validator component is not installed');
+        }
+
+        parent::setUp();
+        $kernel = static::bootKernel(['test_case' => 'conflictingValidatorNamespaces']);
+
+        // this is the part is why we must make this test run in separate process
+        $query = <<<'EOF'
+            mutation {
+              conflictingValidatorNamespaces(test: "123", test2: "1", test3: "4")
+            }
+            EOF;
+
+        $response = static::query(
+            $query,
+            self::USER_RYAN,
+            'conflictingValidatorNamespaces'
+        )->getResponse()->getContent();
+
+        $this->assertEquals('{"data":{"conflictingValidatorNamespaces":true}}', $response);
+        // end part
+
+        // Validate definition file
+        /** @var string $definitionFile */
+        $definitionFile = file_get_contents($kernel->getCacheDir().'/overblog/graphql-bundle/__definitions__/MutationType.php');
+
+        $this->assertStringContainsString(
+            'use Symfony\Component\Validator\Constraints as SymfonyConstraints;',
+            $definitionFile,
+            'Generated definition file should contain import of Symfony\Component\Validator\Constraints aliased as SymfonyConstraints'
+        );
+        $this->assertStringNotContainsString(
+            'use '.Validator\CustomValidator1\Constraint::class,
+            $definitionFile,
+            'Generated definition file should not contain imports of custom constraints, FQCN should be used instead'
+        );
+        $this->assertStringNotContainsString(
+            'use '.Validator\CustomValidator2\Constraint::class,
+            $definitionFile,
+            'Generated definition file should not contain imports of custom constraints, FQCN should be used instead'
+        );
+    }
 }