Updating Federation directives #2010
Conversation
|
Will check the test results. |
|
I had to implement the following methods due to the upgrade of
Without these implementations, errors were produced during the execution of I believe these |
|
Hmm, the A rather crazy idea that I have is that
But if it turns out to be too hacky and unwieldy, I'd say we shouldn't force-fit it... I'm not sure how much these things are in use, and whether it's absolutely necessary to support them at the cost of introducing ugly hacks. |
|
As for the change you've introduced that now shows the default values of directive arguments if they're not explicitly specified, I think that's a good idea. While AFAIU it's not strictly necessary, I guess it will help clear out some confusion. |
| @@ -87,6 +89,7 @@ private IndexView createCustomIndex() { | |||
| indexer.index(convertClassToInputStream(Repeatable.class)); | |||
|
|
|||
| // directives from the API module | |||
| indexer.index(convertClassToInputStream(Authenticated.class)); | |||
There was a problem hiding this comment.
Note mostly to myself: the same change will also be needed in Quarkus (https://github.com/quarkusio/quarkus/blob/3.7.0.CR1/extensions/smallrye-graphql/deployment/src/main/java/io/quarkus/smallrye/graphql/deployment/SmallRyeGraphQLProcessor.java#L275) - I'll take care of that after we release SR-GQL with this change
Sounds like this is doable. I will give it a go as soon as I can. |
|
I managed to get this working with the following type of directive (example for @Policy(policies = {@PolicyGroup({"policy1", "policy2"}), @PolicyGroup({"policy3"})})I believe this approach is better than allowing users to define an array of strings separated by commas: @Policy(policies = {"policy1, policy2", "policy3"})Now, achieving all of this isn't actually that hacky:
Now my schema is successfuly generated: directive @policy(policies: [[String!]!]!) on SCALAR | OBJECT | FIELD_DEFINITION | INTERFACE | ENUM
type Product @authenticated @key(fields : "id")@policy(policies : [["policy1", "policy2"], ["policy3"]]) @requiresScopes(scopes : [["scope1", "scope2"], ["scope3"]]) {
...
}However, a problem arose during testing. When running the test This is the object public class SuperHero implements Character {
private List<Team> teamAffiliations;
private List<@NonNull String> superPowers;Therefore, the GraphQL type should be So my question is, in the context of handling |
| @@ -46,12 +45,36 @@ public DirectiveType create(ClassInfo classInfo) { | |||
| directiveType.setLocations(getLocations(classInfo.declaredAnnotation(DIRECTIVE))); | |||
| directiveType.setRepeatable(classInfo.hasAnnotation(Annotations.REPEATABLE)); | |||
|
|
|||
| Class<?> directiveClass = null; | |||
| try { | |||
| ClassLoader loader = Thread.currentThread().getContextClassLoader(); | |||
There was a problem hiding this comment.
Please no loading of application classes in the schema-builder module - this happens at build time, and loading application classes may lead to a lot problems especially with native mode.
I see you're using this for checking the directive's type, can you just use something like
// create these just once, as a constant
DotName POLICY = DotName.createSimple(Policy.class.getName());
DotName REQUIRES_SCOPES = DotName.createSimple(RequiresScopes.class.getName());
(...)
if(classInfo.name().equals(POLICY) || classInfo.name().equals(REQUIRES_SCOPES)) {
...
}
?
I guess you don't have to deal with inheritance here because Java annotations can't inherit.
There was a problem hiding this comment.
Thanks for the suggestion, fixed with static final variables:
a55b152#diff-5270b78e6efcb5bcd4deb9ab5d9532e4e3e8fed016364e918a7bd48accbda320R30
| import org.jboss.jandex.ClassInfo; | ||
| import org.jboss.jandex.DotName; | ||
| import org.jboss.jandex.MethodInfo; | ||
| import org.jboss.jandex.*; |
There was a problem hiding this comment.
| // of strings, where none of the nested elements can be null | ||
| AnnotationInstance nonNullAnnotation = AnnotationInstance.create(NON_NULL, null, | ||
| Collections.emptyList()); | ||
| DotName stringDotName = DotName.createSimple(String.class.getName()); |
There was a problem hiding this comment.
This can be pulled up into a constant so we only create this DotName once
There was a problem hiding this comment.
| for (AnnotationValue annotationValue : annotationInstance.values()) { | ||
| directiveInstance.setValue(annotationValue.name(), valueObject(annotationValue)); | ||
| if (RequiresScopes.class.isAssignableFrom(directiveClass) || Policy.class.isAssignableFrom( |
There was a problem hiding this comment.
again,
DotName POLICY = DotName.createSimple(Policy.class.getName());
DotName REQUIRES_SCOPES = DotName.createSimple(RequiresScopes.class.getName());
if(classInfo.name().equals(POLICY) || classInfo.name().equals(REQUIRES_SCOPES)) {
...
}
should do It
There was a problem hiding this comment.
In this case, I resolved this by using Policy.class.getName(), since DirectiveType#name is stored as string, not as DotName.
a55b152#diff-a9f696258582406ccbe182f6ecafe672d772737c348aafb1e843d62d3f76e722R70
| do { | ||
| if (wrapper.isCollectionOrArrayOrMap()) { | ||
| graphQLInputType = list(graphQLInputType); | ||
| // Wrapper itself can also be mandatory | ||
| if (wrapper.isNotEmpty()) { | ||
| graphQLInputType = GraphQLNonNull.nonNull(graphQLInputType); |
There was a problem hiding this comment.
Actually I think the wrapper.isNotEmpty() method has a rather confusing name and it should instead be isWrappedTypeNotNull() - it refers to non-nullability of the wrapped type, not the wrapper itself. Therefore moving this graphQLInputType = GraphQLNonNull.nonNull(graphQLInputType); after graphQLInputType = list(graphQLInputType) is wrong because then you mark the OUTER type (the wrapper) as non-null based on the INNER type being non-null.
I think we should rename isNotEmpty to isWrappedTypeNotNull and then here we would have this method as follows:
private GraphQLInputType createGraphQLInputType(Field field) {
GraphQLInputType graphQLInputType = referenceGraphQLInputType(field);
Wrapper wrapper = dataFetcherFactory.unwrap(field, false);
// Field can have a wrapper, like List<String>
if (wrapper != null && wrapper.isCollectionOrArrayOrMap()) {
// Loop as long as there is a wrapper
do {
if (wrapper.isCollectionOrArrayOrMap()) {
if (wrapper.isWrappedTypeNotNull()) {
graphQLInputType = GraphQLNonNull.nonNull(graphQLInputType);
}
graphQLInputType = list(graphQLInputType);
wrapper = wrapper.getWrapper();
} else {
wrapper = null;
}
} while (wrapper != null);
}
// Check if field is mandatory
if (field.isNotNull()) {
graphQLInputType = GraphQLNonNull.nonNull(graphQLInputType);
}
return graphQLInputType;
}
??? The tests pass for me then. But I may not properly understand the reason for your changes here.
AFAIU there's no concept of not-empty collections in GraphQL. If the inner type is marked as not-null, it means that the collection must not contain null entries, NOT that it must not be empty.
There was a problem hiding this comment.
Maybe @phillip-kruger remembers details about the wrapper.isNotEmpty method and can correct me, but I think it means that the wrapped type is not-null
There was a problem hiding this comment.
That makes a lot more sense, thank you for the explanation. I implemented the fixes you mentioned and now the tests pass successfully.
f0bd744
|
Kudos for the idea with |
jmartisk
force-pushed
the
issue-federation-2.5
branch
from
fcbf5fb to
0f3887e
Compare
…eld, default value directive set
jmartisk
force-pushed
the
issue-federation-2.5
branch
from
0f3887e to
b7331c3
Compare
|
I've rebase, squashed the commits and added one more little fix, hopefully it's all good This might become a more general fix because we don't have straightforward support for custom scalars in directive arguments |
I started the implementation of supporting directive @link as well, which uses scalar Import. It can be either a string or an object so I will have to check the posibility of implementing this either way. |
Nice! Let's do it in a separate PR please. Together with some other PRs that are going in right now, it's becoming a bit too much to handle for me. |
|
Thanks for your contribution, this is awesome |
#2009