Refactor transformation

[ybroeker]

• 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 (Testcase in Add some testcases eclipse/microprofile-graphql#252)

• 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 (Testcase in Add some testcases eclipse/microprofile-graphql#252)

• Build a foundation for Map a complex class to a scalar eclipse/microprofile-graphql#251

[phillip-kruger]

Do we have JUnit 5 yet ?

[ybroeker]

It is at least already used in the client-implementation.

[phillip-kruger]

Hi @ybroeker . Thanks for this. I am not sure "why?" though. eems like nothing have really changed i.t.o functionally ? Some decisions, that was made in the implementation has moved to the schema builder ? Why ?

[ybroeker]

There are some errors, There were still a few errors, like @NumberFormat int[] or handling of char (see eclipse/microprofile-graphql#252). These require different types on graphql side and on "resolver side" (String[] <-> int[], String <-> char), which has not been considered yet.

To fix this, the corresponding type needs to be known, so it is stored in the schema model. For example for int it is Integer, for a formatted int a String, for LocalDate a String, for char a String.

Since both types are always known, shouldTransform can be simplified - just compare types instead keeping a list of types to transform.

Also the transformers can be simplified a bit.
e.g. now the NumberTransformer gets only numbers, before this was not clear, in some cases it was a String, so the input was formatted first and then immediately parsed. With FormattedNumberTransformer this leads to the behaviour, that a @NumberFormat BigDecimal-unput was first parsed from String to double (loosing some information), then converted back to String to be then parsed to BigDecimal, now it is parsed only once directly to BigDecimal.

And as a bonus, eclipse/microprofile-graphql#252 would be trivial to implement.

[phillip-kruger]

Hi @ybroeker . Thanks for the explanation. So we should still be able to fix those cases without changing the model. Might be a bit more code, but I think the decision on how the transform work should stay in the impl. We might have to also check if this is a collection and then apply accordingly. W.r.t JUnit, we are waiting on the smallrye parent to release before we should use it. Let me check on that in the mean time.

[ybroeker]

So we should still be able to fix those cases without changing the model. Might be a bit more code, but I think the decision on how the transform work should stay in the impl.

Do you see any disadvantages in keeping the information in the model as well?
The respective scalar type is already known there, this just adds the Java type for this scalar type.

The matching type could be searched for at runtime, but I'm not sure if that would make things easier?
Just asking out of interest, it would actually be a fairly simple change, just need to keep a Map of types for each scalar.

We might have to also check if this is a collection and then apply accordingly.

Collections worked correctly before, but there were errors with Array, which are fixed now..

W.r.t JUnit, we are waiting on the smallrye parent to release before we should use it. Let me check on that in the mean time.

JUnit 5 could also be easily reverted, this just makes some things easier.
All previously existing tests with JUnit 4 are not affected by Junit 5 either.

[phillip-kruger]

Hi @ybroeker - Apologies for taking so long to reply, I am on PTO at the moment, I'll look at this next week.

[phillip-kruger]

Hey @ybroeker . Can you revert to JUnit 4 and look at the Conflicting files? Once fixed we can merge.

[phillip-kruger]

Thanks @ybroeker !