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

Improvements on custom scalar types #178

Open
oojacoboo opened this issue Nov 18, 2019 · 7 comments
Open

Improvements on custom scalar types #178

oojacoboo opened this issue Nov 18, 2019 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@oojacoboo
Copy link
Collaborator

I've been struggling with getting a custom scalar type working with GraphQLite. I'm running into a number of issues, but at the end of the day, I'm just not clear on how everything is supposed to work.

The type mappers are quite confusing as there seem to be a lot of them used by GraphQLite internally. IMO, the library shouldn't use anything but the basic scalar types by default, and allow you to add custom, or pre-built, type mappers/scalar types during configuration.

Overall though, I just don't really understand how to get this working. A full code example would be awesome. In fact, an entire sample codebase that shows how everything is wired up would be even more helpful.

Most of the confusion stems from 2 places.

  • Knowing how to register these scalar types properly. Do you use the $factory->addTypeNamespace() method? Do you just $factory->addRootTypeMapper() and allow it to handle the mapping without needing to register the custom scalar?
  • Why do I need a mapper and a scalar type? I must be missing something, but it'd seem to me I could just register a custom scalar type that implements a given interface and the default mapper would attempt using it. Maybe there is some additional flexibility to having a custom root typemapper as well (probably is), but I'm not seeing it.
@oojacoboo
Copy link
Collaborator Author

I was finally able to get custom scalar types working. The issue, at the end of the day was that the mapper must not return a new instance of the GraphQL type. By using a static cached property of the type object on the mapper, it resolves this issue.

For those that might come across this confusing error and see this issue, the error in question is:

Schema must contain unique named types but contains multiple types named "Date". Make sure that type loader returns the same instance as defined in Foo.date (see http://webonyx.github.io/graphql-php/type-system/#type-registry)."

The error message made me think that the getter method that was annotated had to return the actual GraphQL type and that I couldn't map it accordingly.

It would still be really helpful for the docs to explain more on how to write and register custom scalar types. I think this is a very common need for people and I agree with the reference implementation's opinion of not implementing these by default as there is a wide variety of needs for each application. Therefore, I suggest the removal of the DateTime scalar type from being mapped and instead support a way for it to be configured/mapped, should the developer wish to do so.

@oojacoboo oojacoboo reopened this Dec 18, 2019
@oojacoboo
Copy link
Collaborator Author

oojacoboo commented Dec 18, 2019

I'm reopening this issue. I've updated to the latest code and things have broken with type mapping. I have most of it pieced together again, but the custom scalar type mapping is broken again.

The docs on this subject aren't exhaustive enough. I have no idea how to handle the following:

/**
 * @GraphQLite\Type()
 */
class Product
{
    protected Money $amount;

    /**
     * @GraphQLite\Field()
     */
    public function getAmount(): Money
    {
        return $this->amount;
    }
}

How do I tell GraphQLite how to handle a Money object, how to convert that to a scalar value? I see the docs on creating the mapper and factory. I have done that and believe that to all be working correctly.

Does this Money object have to extend GraphQL\Type\Definition\ScalarType? If so, how is one supposed to go about that? That class has a pretty hairy constructor.

@oojacoboo
Copy link
Collaborator Author

oojacoboo commented Dec 19, 2019

Okay, after some digging, I was able to figure out the issue. I'll leave some details here for other folks that might come across this.

Firstly, you're going to want to create a new scalar type object that extends GraphQL\Type\Definition\ScalarType and implement it to do the conversion. It's probably very possible to create a single one of these that follows an interface. I haven't gone down that path though. I know there are some other interfaces pointing to using serialize, etc.

Here is my custom type mapper that I created which handles all of our custom types:

<?php

declare(strict_types = 1);

namespace Acme\GraphQL\Type;

use GraphQL\Type\Definition\InputType;
use GraphQL\Type\Definition\NamedType;
use GraphQL\Type\Definition\OutputType;
use GraphQL\Type\Definition\ScalarType;
use phpDocumentor\Reflection\DocBlock;
use phpDocumentor\Reflection\Type;
use Acme\Cog\Date;
use Acme\GraphQL\Type\Scalar\Date as DateType;
use TheCodingMachine\GraphQLite\Mappers\Root\RootTypeMapperInterface;

/**
 * Handles the type mapping for GraphQL
 *
 * @author Jacob Thomason <[email protected]>
 */
class Mapper implements RootTypeMapperInterface
{

    protected RootTypeMapperInterface $next;

    protected array $typeMaps;


    /**
     * Constructor
     *
     * @param RootTypeMapperInterface $next
     */
    public function __construct(RootTypeMapperInterface $next)
    {
        $this->next = $next;

        // Define your type maps here
        $this->typeMaps = [
            'Date' => [Date::class => new DateType()],
        ];
    }


    /**
     * Gets the type maps
     */
    protected function getTypeMaps(): array
    {
        return $this->typeMaps;
    }


    /**
     * Gets the respective type for a type name
     *
     * @param Type $type
     */
    protected function getType(Type $type): ?ScalarType
    {
        $fqcn = ltrim((string)$type, '\\');
        foreach ($this->getTypeMaps() as $map) {
            if (isset($map[$fqcn])) {
                return $map[$fqcn];
            }
        }

        return null;
    }


    /**
     * Returns a GraphQL type by name.
     * If this root type mapper can return this type in "toGraphQLOutputType" or "toGraphQLInputType", it should
     * also map these types by name in the "mapNameToType" method.
     *
     * @param string $typeName The name of the GraphQL type
     */
    public function mapNameToType(string $typeName): NamedType
    {
        if (isset($this->getTypeMaps()[$typeName])) {
            return array_values($this->getTypeMaps()[$typeName])[0];
        }

        return $this->next->mapNameToType($typeName);
    }


    /**
     * Handles the conversion to an input type
     *
     * @param Type $type
     * @param InputType|null $subType
     * @param string $argumentName
     * @param \ReflectionMethod $refMethod
     * @param DocBlock $docBlockObj
     */
    public function toGraphQLInputType(
        Type $type,
        ?InputType $subType,
        string $argumentName,
        \ReflectionMethod $refMethod,
        DocBlock $docBlockObj
    ): InputType
    {
        return $this->getType($type)
            ?: $this->next->toGraphQLInputType($type, $subType, $argumentName, $refMethod, $docBlockObj);
    }


    /**
     * Handles the conversion to an output type
     *
     * @param Type $type
     * @param OutputType|null $subType
     * @param \ReflectionMethod $refMethod
     * @param DocBlock $docBlockObj
     */
    public function toGraphQLOutputType(
        Type $type,
        ?OutputType $subType,
        \ReflectionMethod $refMethod,
        DocBlock $docBlockObj
    ): OutputType
    {
        return $this->getType($type)
            ?: $this->next->toGraphQLOutputType($type, $subType, $refMethod, $docBlockObj);
    }
}

Now, the only thing you need to do is define your "type maps" in the constructor. The key in the array is the type name you're using in your GraphQLite annotations. The array value assigned to that key is your domain object/type mapped to the scalar type that Webonyx graphql lib expects.

Honestly, I think you could add an interface to your domain objects/types, and have one class that implements the ScalarType. I think this may be what AnyScalarType was trying to accomplish. But I couldn't ever make any sense of that lib and how it tied into graphqlite.

@moufmouf
Copy link
Member

Hey @oojacoboo ,

You are certainly right.
The RootTypeMapperInterface allows to hook into type resolving and allows anyone to do almost anything. But it is hard to use.

And defining a scalar type is certainly something common enough to try to simplify it.

Your Mapper class is a great start to simplify this.

Out of curiosity, if you were to define an interface for Scalar types, how would you design it?

@oojacoboo
Copy link
Collaborator Author

oojacoboo commented Dec 26, 2019

@moufmouf honestly, after a little more thought here, I think I'd prefer to not pollute my domain objects/scalars with the particulars of GraphQL. The class that I've written above is fairly agnostic. The only thing that's required is the mapping in the constructor. If that mapping could be exposed and state managed through the config, that'd be ideal, IMO. You'd just provide the simple $typeMaps mapping array to a config setter and that's that.

Of course, the highly customizable nature of the RootTypeMapperInterface could remain, but providing a generic/simple map like this would be ideal. It's unlikely that most people would ever need to actually write one themselves. The type mapper doesn't define how the actual scalar is serialized/deserialized just the mapping from a string to a class. The logic for the actual serializing/deserializing would be handled in the Webonyx ScalarType object. So, you just need to tell GraphQLite where to map the annotation definition/string. Like, in my example above, I'm mapping Date, which returns my domain, Acme\Cog\Date, object and I want Acme\GraphQL\Type\Scalar\Date to handle all my GraphQL serialization/deserialization logic.

I haven't looked into the internals here, but there are some inconsistencies with the strings being returned and used throughout the RootTypeMapperInterface implementation. For instance, the class string Date, which I assume could be the FQCN, would be the same as the domain object's FQCN. So, that seem redundant and difficult to deal with.

At the end of the day, having a simple ['Acme\Cog\Date' => 'Acme\GraphQL\Type\Scalar\Date'] map/array is mostly what you need. AKA, tell GraphQLite, when it gets a return object of... Acme\Cog\Date, handle it with this scalar object Acme\GraphQL\Type\Scalar\Date. If you wanted to stick with annotations, you could create a new annotation definition put on the domain object to define the GraphQL scalar type to "handle"the serialization/deserialization as a return type.

@moufmouf moufmouf added this to the v4.1 milestone Dec 27, 2019
@mailopl
Copy link

mailopl commented Jan 16, 2020

Thanks @oojacoboo you've been a much of help.

I was able to extract a CustomScalar class that's inherited by Email, Password, Money and other ValueObject types I need to map.

I was able to register type mapper factory (in Laravel) like this:

public function boot()
    {
        $this->app->get(SchemaFactory::class)->addRootTypeMapperFactory(
            new ValueObjectsTypeMapperFactory()
        );
    }

I agree this should be part of the core, to be configured by the user in the end and also done a little bit more automatically.

Consider this example:

/**
 * @GraphQLite\Type()
 */
class Product
{
    protected Money $amount;
    protected Email $email;
    protected Discontinued $discontinued;

    /**
     * @GraphQLite\Field()
     */
    public function getAmount(): Money
    {
        return $this->amount;
    }

    /**
     * @GraphQLite\Field()
     */
    public function getEmail(): Email
    {
        return $this->email;
    }

    /**
     * @GraphQLite\Field()
     */
    public function getDiscontinued(): Discontinued
    {
        return $this->discontinued;
    }
}
// That clearly is a string scalar that could be handled automatically
// without even using interfaces

class Email {
    public function __toString()
    {
        return (string) $this->getValue();
    }
}
// That clearly is a more complex structure, but possible to encode and return serialized data
class Money implements \JsonSerializable {

}

So:

{
  product {
      email
      amount
      discontinued
  }  
}

Would automatically return:

{
    "data": {
    "product": {
        "email": "[email protected]",
        "amount": "{amount:420,currency:USD}",
        "discontinued": "false"
    }
  }
}

Also I was looking for some standard describing ValueObject interfaces but found none.
I imagine it's pretty common for people to use StringValueInterface / IntegerValueInterface / FloatValueInterface like so (which doesn't pollute the domain by internals of Graphql types):

class Email implements StringValueInterface {
}

But found no standard for it.

@oojacoboo
Copy link
Collaborator Author

@mailopl glad that was helpful.

While you're right that those classes could be handled automatically, I personally am not a fan of that approach and much prefer explicit definitions. I'm not a fan of magic methods, in general, including __toString as, if not used carefully, can end up exposing internals. Also, if it's preferred to handle those scalars in that fashion, that'd always be something you could define.

Regardless, I think there is a much more simplified way of handling this for most all scalar cases and the need to add a custom type mapper/factory is unnecessary.

@oojacoboo oojacoboo added the enhancement New feature or request label Mar 29, 2021
@oojacoboo oojacoboo changed the title Docs on custom scalar types Improvements on custom scalar types Jun 12, 2022
@oojacoboo oojacoboo added the help wanted Extra attention is needed label Jun 12, 2022
@oojacoboo oojacoboo added the good first issue Good for newcomers label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants