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

How to create your own scalar #94

Open
phillip-kruger opened this issue Oct 22, 2019 · 19 comments
Open

How to create your own scalar #94

phillip-kruger opened this issue Oct 22, 2019 · 19 comments
Labels
Meeting discussion To mark issues we want to discuss in the weekly meeting
Milestone

Comments

@phillip-kruger
Copy link
Member

The current spec text mentioned that users can create their own scalar types, however not how.

My suggestion is to create a new Annotation @CustomScalarMarker and an interface CustomScalar that will allow a user to create a Scalar, example:

@CustomScalarMarker
public class LocalDateScalarProvider implements CustomScalar<LocalDate, String> {

    @Override
    public String getName() {
        return "Date";
    }

    @Override
    public String getDescription() {
        return "Date Scalar";
    }

    @Override
    public String serialize(LocalDate localDate) {
        return localDate.format(DateTimeFormatter.ISO_DATE);
    }

    @Override
    public LocalDate deserialize(String fromScalar) {
        return LocalDate.parse(fromScalar);
    }

}

This is just an example on how it could be done. Any other thoughts ?

@andymc12
Copy link
Contributor

Ha! I missed this when I replied to issue #81.

I think this is a good approach, but it means that the implementation would need to scan application classes for the @CustomScalarMarker and then parse the generic type from CustomScalar to perform the mapping. That is probably ok, but will have an impact to application startup performance.

I wonder if we could use MP Config in some manner to avoid the startup scanning - maybe something like:
mp.graphql.customScalarConverters=com.mypkg.LocalDateScalarProvider,com.mypkg.SomeOtherScalarProvider,...

@phillip-kruger
Copy link
Member Author

We are already doing a lot of startup scanning at the moment right? Maybe it's mostly happening in SPQR but it's happening ? But maybe we support both approaches ? Or use SPI ?

@kaqqao
Copy link

kaqqao commented Oct 24, 2019

I'd say the scanning is not necessary to support this feature, as a custom scalar only becomes relevant if it's reachable from somewhere. We map them only when we encounter them, no need to proactively search for them, right?
Also, no classpath scanning is done to support the current spec (and is off by default in SPQR).

But... I'm generally against the idea of introducing special syntax/feature that doesn't tackle a general problem. I'd rather solve customizing the type mapping generally, and implement custom scalars in terms of that solution. Even if the general solution isn't publicaly accessible in the current spec version (I'd go as far as to argue that's actually preferable). While custom mapping is a much more difficult problem, I don't think it is avoidable. And once we expand the spec to solve it, the special syntax around any special case (here the scalars) becomes tech debt and a cognitive overload.

I can be persuaded otherwise if we see a clear migration path from the special implementation to the general in the future.

@kaqqao
Copy link

kaqqao commented Oct 24, 2019

Oops, I misunderstood the classpath scanning point above. Still, can't we rely on all the providers being managed beans in the CDI container?

@andymc12
Copy link
Contributor

Still, can't we rely on all the providers being managed beans in the CDI container?

Yes, I think so. In this case, @CustomScalarMarker would need to be a bean-defining annotation. Then we could use CDI to get all instances and automatically register the mappings. Good point!

@andymc12 andymc12 added this to the 1.1 milestone Nov 15, 2019
@andimarek
Copy link

A custom scalar requires three different methods to be implemented (in the future there will be actually a forth one). The naming of the methods are historically a little bit confusing, if you define a new Interface you have the chance of naming them better. See graphql/graphql-js#2357 and maybe this blog post (https://www.graphql.de/blog/scalars-in-depth/) for some background.

The real challenge here is that one of the methods requires an Abstract syntax tree (Ast) Node to be passed in. This comes from GraphQL Java and would mean you depend on GraphQL Java or you make it generic (like Object) and say every implementation can pass in something implementation specific. Not sure what the general goal in terms of being a complete or "leaky" abstraction.

@kaqqao
Copy link

kaqqao commented Feb 19, 2020

@andimarek Just being curious, what's the 4th scalar method you mentioned?

As for custom scalars, the only real possible serialization for new types is String, so I think we can get away with just unpacking StringValue from graphql-java and passing that to the custom impl, for the foreseeable future at least. But I'm generally in favor or leaky abstraction here, and giving the implementor the ability to always reach the lower level API (that's what I've done for SPQR as well).

@andimarek
Copy link

@kaqqao the 4th method allows for converting an input value to an Ast for printing default values in the SDL. See graphql/graphql-js#1817 and graphql-java/graphql-java#1745 for details.

Not sure I follow your comment about StringValue: custom scalars can be arbitrary AST and StringValue is not sufficient. For example JSON scalars are input objects like {a: "A"} or you could allow for enum Names like SOME_VALUE or even numbers which exceeds Int (123456789012343). Here is a example for parsing input object ast in a custom JSON scalar

@kaqqao
Copy link

kaqqao commented Feb 19, 2020

@andimarek Ah, the value -> AST mapping is useful indeed.

You're of course right, any value could be used for a scalar, I was just (clumsily, on the phone) suggesting that the custom-scalars-via-annotations-only API doesn't need to support 100% of possible cases, as strings are the serialization format behind 99% of custom scalars in the wild (according to my experience and investigation), and offering just that (for now?) would already get us a long way.

For more advanced cases, I'm a proponent of a more general type mapping customization API (similar to SPQR's TypeMapper or even convertors, or Jsonb's adapters), which would cater to any case we don't deal with out of the box. Complex scalars being just one example.

@t1
Copy link
Contributor

t1 commented Apr 2, 2020

Sometimes the built-in Java field based serialization is not enough. Maybe we could utilize JSON-B for more complex use-cases.

But scalars are not complex. They map from/to one primitive (String/ID, int, double, or boolean). The most common use-case is a type-safe wrapper class, e.g. CustomerId with only a single non-transient, non-static field, e.g. String value. We could support this with a simple @Scalar annotation, so that the (de)serialization is done by (de)serializing this single field while it would be an error to have more than one. The schema would just show the 'unwrapped' type.

I think this would solve 99% of all use-cases and it would be a simple change.

This is independent of the type semantics within the schema, e.g. this is an email address or a local date or a customer id. The Custom Scalar Spec URIs will define this in GraphQL, and we may support that by adding an annotation method uri in the @Scalar annotation or so.

@kaqqao
Copy link

kaqqao commented Apr 3, 2020

Scalars are allowed to be anything, including complex deeply nested objects (they're still leaf types, so no sub-selection possible). E.g. the object (a.k.a. JSON) scalar allows any structure to be passed.
So it makes sense to generalize the described unwrapping functionality (it reminds me of Jackson's @JsonUnwrap).

That said, I stick by my original suggestion to implement (but not necessarily immediately make public) a general type adapting/mapping facility (maybe akin to JSONB type adapters/serializers?) and then implement this feature on top of that. E.g. unwrapping could be achieved by an adapter that substitutes a container type for its contained type.

@t1
Copy link
Contributor

t1 commented Apr 3, 2020

@kaqqao: All the examples that come to my mind could either be represented just as well as normal objects (often with many non-null fields) or have a broadly used canonical string representation. Maybe you have a good use-case in mind?

@t1
Copy link
Contributor

t1 commented May 4, 2020

We could also add a configuration option with a list of automatic scalars, i.e. types that overload toString and can be constructed from a single String, i.e. have a constructor or a constructor method named of, valueOf, or parse. It could be configured for *, so all types that fulfill those criteria are automatically considered to be scalars. The Smallrye Client API already works that way (without the config).

This could also be expanded to standard GraphQL scalar types other than String.

@t1
Copy link
Contributor

t1 commented May 7, 2020

I have another question: a not so uncommon use case is that we may want to have something to be a scalar as well as a complex type, e.g. an email field may be requested as a scalar string, but I may also want to request the domain or the status of the email validation process. How would we support this?

@t1
Copy link
Contributor

t1 commented May 8, 2020

@phillip-kruger : Can we discuss this today, too? (if we have time) This is one of the bigger pain points we currently have in our project.

@phillip-kruger
Copy link
Member Author

For sure !

@phillip-kruger phillip-kruger added the Meeting discussion To mark issues we want to discuss in the weekly meeting label May 8, 2020
@t1
Copy link
Contributor

t1 commented May 14, 2020

My last comment about evolving a field from scalar to complex is related to #32 Union Types, I guess, but I have no idea how this would work in Java. Any hints?

@t1
Copy link
Contributor

t1 commented May 15, 2020

Just re-read the GraphQL spec about Unions. So Unions won't help for this use-case, as they must not reference Scalars, i.e. union Email = String | EmailInfo is invalid. I don't see any alternative but to use different field names, e.g. email and emailString. Or is there?

@t1
Copy link
Contributor

t1 commented May 15, 2020

As discussed in the meeting today, I've created a new issue #251 for Automatic Conversion, as these are not really custom scalar types in the schema, but only implicit mappings for custom wrapper Java types. Most of my comments here apply to that issue, not really here :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meeting discussion To mark issues we want to discuss in the weekly meeting
Projects
None yet
Development

No branches or pull requests

5 participants