Skip to content
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

BUGFIX: Throw on duplicate derived model names #421

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/Config/ModelConfiguration.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,12 @@ public function getOperationConfig(string $operation): array

/**
* @param string $class
* @param array $mapping
* @return string
* @throws SchemaBuilderException
*/
public function getTypeName(string $class): string
public function getTypeName(string $class, $mapping = []): string
{
$mapping = $this->get('type_mapping', []);
$custom = $mapping[$class] ?? null;
if ($custom) {
return $custom;
}

$typeName = $this->formatClass($class);
$prefix = $this->getPrefix($class);

Expand Down
8 changes: 7 additions & 1 deletion src/Schema/DataObject/DataObjectModel.php
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,13 @@ public function getDataObject(): DataObject
*/
public function getTypeName(): string
{
return $this->getModelConfiguration()->getTypeName(get_class($this->dataObject));
$class = get_class($this->dataObject);
$mapping = $this->getSchemaConfig()->getTypeMapping();
if (isset($mapping[$class])) {
return $mapping[$class];
}

return $this->getModelConfiguration()->getTypeName($class);
}

/**
Expand Down
10 changes: 10 additions & 0 deletions src/Schema/Schema.php
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,16 @@ public function removeScalar(string $name): self
public function addModel(ModelType $modelType, ?callable $callback = null): Schema
{
$existing = $this->models[$modelType->getName()] ?? null;
if ($existing) {
Schema::invariant(
$existing->getModel()->getSourceClass() === $modelType->getModel()->getSourceClass(),
'Name collision for model "%s". It is used for both %s and %s. Use the tyepMapping setting in your schema
config to provide a custom name for the model.',
$modelType->getName(),
$existing->getModel()->getSourceClass(),
$modelType->getModel()->getSourceClass()
);
}
$typeObj = $existing
? $existing->mergeWith($modelType)
: $modelType;
Expand Down
9 changes: 9 additions & 0 deletions src/Schema/SchemaConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ public function setTypeMapping(array $typeMapping): self
return $this->set('typeMapping', $typeMapping);
}

/**
* @return array
* @throws SchemaBuilderException
*/
public function getTypeMapping(): array
{
return $this->get('typeMapping', []);
}

/**
* @param string[] $fields
* @return $this
Expand Down
41 changes: 0 additions & 41 deletions tests/Config/SchemaContextTest.php

This file was deleted.

14 changes: 14 additions & 0 deletions tests/Fake/SubFake/FakePage.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace SilverStripe\GraphQL\Tests\Fake\SubFake;

use SilverStripe\GraphQL\Tests\Fake\FakeSiteTree;

class FakePage extends FakeSiteTree
{
private static $table_name = "GraphQL_SubFakePage";

private static $db = [
'SubFakePageField' => 'Varchar',
];
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
use SilverStripe\ORM\FieldType\DBField;
use SilverStripe\ORM\FieldType\DBTime;

class DBTimeTest extends SapphireTest
class DBTimeArgsTest extends SapphireTest
{
public function testApply()
{
Expand Down
32 changes: 32 additions & 0 deletions tests/Schema/SchemaContextTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@
use SilverStripe\Dev\SapphireTest;
use SilverStripe\GraphQL\Schema\DataObject\DataObjectModel;
use SilverStripe\GraphQL\Schema\Exception\SchemaBuilderException;
use SilverStripe\GraphQL\Schema\Field\Field;
use SilverStripe\GraphQL\Schema\Resolver\DefaultResolverStrategy;
use SilverStripe\GraphQL\Schema\Schema;
use SilverStripe\GraphQL\Schema\SchemaBuilder;
use SilverStripe\GraphQL\Schema\SchemaConfig;
use SilverStripe\GraphQL\Schema\Type\ModelType;
use SilverStripe\GraphQL\Schema\Type\Type;
use SilverStripe\GraphQL\Tests\Fake\DataObjectFake;
use SilverStripe\GraphQL\Tests\Fake\FakeSiteTree;
use SilverStripe\GraphQL\Tests\Fake\SchemaContextTestResolverA;
use SilverStripe\GraphQL\Tests\Fake\SchemaContextTestResolverB;

class SchemaContextTest extends SapphireTest
{
Expand Down Expand Up @@ -41,4 +46,31 @@ public function testGetTypeNameForClass()
$storeableSchema->getConfig()->getTypeNameForClass(FakeSiteTree::class)
);
}

public function testResolverDiscovery()
{
$context = new SchemaConfig([
'resolvers' => [
SchemaContextTestResolverA::class,
SchemaContextTestResolverB::class,
],
'resolverStrategy' => [DefaultResolverStrategy::class, 'getResolverMethod']
]);

$result = $context->discoverResolver(new Type('TypeName'), new Field('fieldName'));
$this->assertEquals('resolveTypeNameFieldName', $result->getMethod());
$this->assertEquals(SchemaContextTestResolverA::class, $result->getClass());

$result = $context->discoverResolver(new Type('TypeName'), new Field('foo'));
$this->assertEquals('resolveTypeName', $result->getMethod());
$this->assertEquals(SchemaContextTestResolverA::class, $result->getClass());

$result = $context->discoverResolver(new Type('Nothing'), new Field('foo'));
$this->assertEquals('resolve', $result->getMethod());
$this->assertEquals(SchemaContextTestResolverA::class, $result->getClass());

$result = $context->discoverResolver(new Type('Nothing'), new Field('specialField'));
$this->assertEquals('resolveSpecialField', $result->getMethod());
$this->assertEquals(SchemaContextTestResolverB::class, $result->getClass());
}
}
26 changes: 24 additions & 2 deletions tests/Schema/SchemaTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@

use SilverStripe\Core\Config\Config;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\GraphQL\Schema\DataObject\DataObjectModel;
use SilverStripe\GraphQL\Config\ModelConfiguration;
use SilverStripe\GraphQL\Schema\DataObject\ModelCreator;
use SilverStripe\GraphQL\Schema\Exception\SchemaBuilderException;
use SilverStripe\GraphQL\Schema\Field\Mutation;
use SilverStripe\GraphQL\Schema\Field\Query;
use SilverStripe\GraphQL\Schema\Interfaces\SchemaStorageInterface;
use SilverStripe\GraphQL\Schema\Schema;
use SilverStripe\GraphQL\Schema\SchemaConfig;
use SilverStripe\GraphQL\Schema\Type\Enum;
Expand All @@ -19,7 +18,9 @@
use SilverStripe\GraphQL\Schema\Type\Type;
use SilverStripe\GraphQL\Schema\Type\UnionType;
use SilverStripe\GraphQL\Tests\Fake\DataObjectFake;
use SilverStripe\GraphQL\Tests\Fake\FakePage;
use SilverStripe\GraphQL\Tests\Fake\FakeSiteTree;
use SilverStripe\GraphQL\Tests\Fake\SubFake\FakePage as SubFakePage;

class SchemaTest extends SapphireTest
{
Expand Down Expand Up @@ -259,6 +260,27 @@ public function testValidConfig()
Schema::assertValidConfig([]);
}

public function testDuplicateModelThrows()
{
$schema = $this->buildSchema();
$schema->addModelbyClassName(FakePage::class);
$this->expectException(SchemaBuilderException::class);
$schema->addModelbyClassName(SubFakePage::class);
}

public function testTypeMapping()
{
$schema = $this->buildSchema();
$schema->getConfig()->setTypeMapping([
SubFakePage::class => 'SubFakePage',
]);
$schema->addModelbyClassName(FakePage::class);
$schema->addModelbyClassName(SubFakePage::class);

$this->assertInstanceOf(ModelType::class, $schema->getModel('FakePage'));
$this->assertInstanceOf(ModelType::class, $schema->getModel('SubFakePage'));
}

public static function noop()
{
}
Expand Down