Add Test case for ArrayTypes when using @DefaultValues

[janknobloch]

Hey,
i hope im not missing anything but i have not found any specifics on @DefaultValues to be expanded for String[].

I think it would be nice to be consistent and being able to set default values for String arrays similar to the existing @DefaultValue approach:

public retType methodName(@Name("myVar") @DefaultValue("123") long myVar)

analog:

public retType methodName(@Name("myVars") @DefaultValues(values = {"123", "345"}) String[] myVars)

adding an additional annotation @DefaultValues could solve this issue easily as per example:

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Target({ElementType.PARAMETER,ElementType.FIELD,ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface DefaultValues {

    /**
     * @return the names to use for the GraphQL argument.
     */
    String[] values() default "";
}

[andymc12]

This is an interesting issue. From looking at https://stackoverflow.com/questions/37167628/defaultvalue-for-a-list it seems that the JAX-RS community has not figured out a good answer for this either. The difference is that they have ParamConverter providers that could convert a single string into multiple array or collection elements.

There are a few different ways to solve it (introducing the GraphQL equivalent of ParamConverters, making @DefaultValues but making it a plural annotation of @DefaultValue, etc.), but I think yours is the probably the best.

@phillip-kruger @t1 @thejibz @jefrajames what do you all think?

[thejibz]

The @DefaultValues solution appeals to me too :) I would add that I think it should be Object[] values() default ""; inside the annotation body to accommodate for other scalar types (Int, Float, Bool, ID).

What about list of list of list ? i.e in graphql myfield: [[[String]]].

(Speaking of that, it reminds me that I have to raise an issue in the smallrye-graphql-server regarding the processing of field with type like Integer[][][] that didn't translate correctly. List<List<List<Integer>>> works though ^^)

[t1]

LGTM. Maybe just use value instead of values so the annotation parameter name should be optional:

public retType methodName(@Name("myVars") @DefaultValues({"123", "345"}) String[] myVars)

[andymc12]

I would add that I think it should be Object[] values() default "";

I think we'd want to leave it as String[] to be consistent with the current @DefaultValue annotation. The idea was that the default value should be able to be converted from a string to the parameter type (e.g. "true"/"false" for booleans, "123" for ints, etc. - and complex types would be submitted as JSON like @DefaultValue("{\"name\": \"Fred\", \"address\": \"123 Cherry Lane\", ...}").

Maybe just use value instead of values so the annotation parameter name should be optional:

+1

[janknobloch]

Thanks for the lively discussion everyone, what I got so far from the discussion is:

1. Stick to String types for now to be consistent
2. Use value instead of values to keep things optional

I'll monitor this thread and try to provide a pull request over the weekend if we find consensus here - if your ok with it. :-)

(one more thing.. can one elaborate briefly on the value vs values definition - I haven't had the time (yet) to look deeper into the code base but it seems this is an graphql specific behaviour on how you internally handle your annotations correct?

Have a great day everyone.

[phillip-kruger]

I thought @DefaultValue support taking a json string ? So we should be able to use json format for arrays ? No ? Who wants to confirm that ? :)

[janknobloch]

@phillip-kruger : if im not mistaken i tried the following :

@DefaultValue("[\"IMAGE_FORMAT_1408x792\", \"IMAGE_FORMAT_427x241\"]") String[] resolutions

receiving an

Caused by: javax.json.bind.JsonbException: Can't deserialize JSON array into: class java.lang.String

so it doesn't seem to work.

[phillip-kruger]

Ok, so let's look into that. As json should work, so this is a bug then :) We can add a test in the TCK to test json array of string. As far as I remember there is a basic (scalar) type, and there is a json complex type test. So this will be a good addition.

[phillip-kruger]

Also this seems to be a bug in smallrye. So let's open the bug there and fix it there. (We can add the test to the TCK after that.)

[phillip-kruger]

smallrye/smallrye-graphql#355

[phillip-kruger]

Hey @janknobloch
Fixed in SmallRye (so you can use json array): see example here: https://github.com/smallrye/smallrye-graphql/blob/562e16586bcfd88d2edf329409460590cb8a4e9b/server/tck/src/test/java/io/smallrye/graphql/test/apps/defaultvalue/api/DefaultValueParrotAPI.java

[janknobloch]

@phillip-kruger, sweet, let me see if I can provide the proper test-cases on the weekend. :-)

[phillip-kruger]

We can just use the once in SmallRye, just move it up to MP

[janknobloch]

Alright so we can close this ? Feel free to close this issue.

[phillip-kruger]

We can keep it open as a reminder to move the test case up at some point

[phillip-kruger]

@janknobloch - B.t.w, what runtime are you using ?

[janknobloch]

Not at my desk anymore but i think I was using openjdk11. - let me verify tmr morning. Why ?

[phillip-kruger]

I mean are you using Open liberty or Quarkus ? Just so we can tell you when and what version will contain the fix. For Quarkus it should be in 1.7 that is out next week and Open Liberty the upcoming version, @andymc12 will have more details

[janknobloch]

Ah sorry - I’m running Quarkus 1.6.1. 😊. thanks also for keeping me up to date - I made a workaround for now so no rush😉 But @phillip-kruger I guess I found another issue with your very nice Graphql-quarkus-extension. Getting some NPE during schema genaeration only in conjunction when using the open-api extension and having a hard time to narrow down the cause of this error. I’ll open an issue for this at the quarkus repo and would really appreciate some input sooner or later. Have a great evening / morning everyone.

[phillip-kruger]

Ok great. So this issue will be sorted in 1.7.0.Final. W.r.t the other issue, yes log a issue and if you have a reproducer, even better. Also try with 1.7.0.CR2 just for in case.

[janknobloch]

I will thanks! I’ll keep u posted. And thanks everyone for your contributions.