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

Map a complex class to a scalar #251

Open
t1 opened this issue May 15, 2020 · 10 comments
Open

Map a complex class to a scalar #251

t1 opened this issue May 15, 2020 · 10 comments

Comments

@t1
Copy link
Contributor

t1 commented May 15, 2020

This is an issue split out of the discussion in #94, as we where talking about separate issues (see meeting notes from today).

In Java we often use types that only have a single String field. Something like:

public class Email {
    private String value;
    // getters, constructors, etc.
}

An email field in SuperHero should be represented as a String in the Schema; to convert it to a String, use the toString method; to convert it from a String, use either a constructor public Email(String value) or a static constructor method public static Email valueOf(String value). The valueOf method could also be named parse or of. This logic is the same as used in MP Config for Automatic Converters.

The very welcome side effect would be that we wouldn't need the special handling for LocalDate, Integer, and many more standard classes any more.

@ybroeker
Copy link
Contributor

How would it be decided for which types this would be done, via a separate annotation?

And should it always be a string scalar? Theoretically, boolean and numbers would also be possible (or any other scalar-type).

@t1
Copy link
Contributor Author

t1 commented May 16, 2020

I assume that hardly anybody really wants to have wrapper classes showing up in the Schema; so I'm in favor of doing it for any class that matches the criteria. If that's a problem, we could add a config option containing a list of fully qualified class names that should be converted automatically. And if this configuration is *, all matching classes will be.

Other types would work, too, but Strings are the 99,99% case ;-) Instead of toString, we could use toLong, etc. But a parse method doesn't make a lot of sense, does it?

@ybroeker
Copy link
Contributor

I'm not sure if there are clear criteria to identify such wrapper classes.
An example from the TCK would be Team, if you just added a constructor Team(String) to allow creation of empty teams, it would suddenly be a String scalar.
(I also think it would be a breaking change? Token from the current TCK is such a wrapper class and there String would make sense, but currently it is required to be a Type.)

A config option would be a possibility for this, but I personally would prefer an annotation for this. Do you see any disadvantages with an annotation for that?

Other types would work, too, but Strings are the 99,99% case ;-) Instead of toString, we could use toLong, etc. But a parse method doesn't make a lot of sense, does it?

I can imagine some cases where string is not enough, e.g. something like Velocity, Weight, Length.

But for the start string is probably enough.

@t1
Copy link
Contributor Author

t1 commented May 16, 2020

You're right, the Team example may not be what a user would expect. It's clear for MP Config or even the GraphQL Client, but not for generating schemas. The constructor methods would be clear, though, wouldn't they?

I'm perfectly fine with an annotation like @Scalar for your own classes. But maybe you use some 3rd party library that you also want to have converted, so I assume the config would still be needed.

something like Velocity, Weight, Length.

I think these would generally also require a unit. But you're right there will be use-cases. Maybe it's not 99,99% but only 99,9% ;-)

ybroeker added a commit to ybroeker/smallrye-graphql that referenced this issue May 16, 2020
* Transform types when a transformation is required instead of keeping a list of types to transform
* Specify concrete types for transformations. Makes code simpler (eg, use
numbers directly instead of formatting->parsing->formatting->parsing) and
more explicit
* Handle chars correctly, char-inputs didn't work before
* Handle BigInteger/BigDecimal correctly, previously those didn't work correctly if formatted (truncated to double or simply failing)
* Handle arrays of formatted types correctly, didn't worked before

* Build a foundation for eclipse/microprofile-graphql#251

Signed-off-by: Yannick Bröker <[email protected]>
@ybroeker
Copy link
Contributor

Constructor methods should be clear (well, I'd be surprised if there isn't a code base somewhere where parse(String) does something else than parsing the string...).

Maybe it is enough for a first implementation to use only toString and parse/valueOf/of, and skip constructors and annotations?
An annotation could be added later if there is a need, e.g. to use constructors or other types than String.

If an annotation is used, annotating third party libs should be possible with #111 once this is solved.

@phillip-kruger
Copy link
Member

I am in favour of a more explicit approach, by adding an annotation. And for the case where these classes is in a 3rd party libs, this is an existing case, that we need to solve, and this should be solved the same way. See #111

@t1
Copy link
Contributor Author

t1 commented May 16, 2020

Okay. I'm fine with that.

ybroeker added a commit to ybroeker/smallrye-graphql that referenced this issue May 27, 2020
* Transform types when a transformation is required instead of keeping a list of types to transform
* Specify concrete types for transformations. Makes code simpler (eg, use
numbers directly instead of formatting->parsing->formatting->parsing) and
more explicit
* Handle chars correctly, char-inputs didn't work before
* Handle BigInteger/BigDecimal correctly, previously those didn't work correctly if formatted (truncated to double or simply failing)
* Handle arrays of formatted types correctly, didn't worked before

* Build a foundation for eclipse/microprofile-graphql#251

Signed-off-by: Yannick Bröker <[email protected]>
t1 pushed a commit to t1/smallrye-graphql that referenced this issue May 29, 2020
* Transform types when a transformation is required instead of keeping a list of types to transform
* Specify concrete types for transformations. Makes code simpler (eg, use
numbers directly instead of formatting->parsing->formatting->parsing) and
more explicit
* Handle chars correctly, char-inputs didn't work before
* Handle BigInteger/BigDecimal correctly, previously those didn't work correctly if formatted (truncated to double or simply failing)
* Handle arrays of formatted types correctly, didn't worked before

* Build a foundation for eclipse/microprofile-graphql#251

Signed-off-by: Yannick Bröker <[email protected]>
@t1
Copy link
Contributor Author

t1 commented Jun 1, 2020

This needs special consideration for BigInteger and BigDecimal to be mapped from Strings, as they only have a normal constructor for those. The valueOf constructor methods take only numeric values.

@ybroeker
Copy link
Contributor

ybroeker commented Jun 1, 2020

This would not be usable for numbers anyway (at least on the server side).

The mapping from string to number is only necessary if the number is formatted, and then a NumberFormatter must be used anyway.

@phillip-kruger phillip-kruger changed the title Automatic conversion for custom wrapper classes Map a complex class to a scalar Jun 15, 2020
@phillip-kruger
Copy link
Member

This is now supported (per field) in SmallRye with @ToScalar annotation. We keep this open until we move that to the spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants