-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add @Namespace for GraphQLApi and GraphQLClientApi #2184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
Most of the changes are not from the @Name
→ @Namespace
change, but from using "namespace" instead of "group" and the fact that they can be nested.
request.append(method.getName()); | ||
if (method.hasValueParameters()) | ||
|
||
request.append(method.getOperationName()); // operation name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request.append(method.getOperationName()); // operation name | |
request.append(method.getOperationName()); |
if (!namespaces.isEmpty()) { | ||
namespaces.forEach(value -> { | ||
request.append(" { "); | ||
request.append(value); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!namespaces.isEmpty()) { | |
namespaces.forEach(value -> { | |
request.append(" { "); | |
request.append(value); | |
}); | |
} | |
namespaces.forEach(namespace -> request.append(" { ").append(namespace)}); |
} | ||
|
||
public String getOperationName() { | ||
return namespaces.stream().map(this::prettyString).collect(joining()) + prettyString(getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. Why do we do this? And if we wouldn't uppercase the very first character, most changes to the tests would become unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example
@Namespace("first/second/third/someMynotherName")
class Api {
@Query("findAll")
////
}
If don create first letter to upper case, operation will firstsecondthirdsomeMynotherNamefindAll. it is unreadable.
And create it only findAll incorrect, becouse in code may be other findAll method. And if we will call just findAll, using some router (with analytics), it can break call analytics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I get it: if the user wants to send multiple operations in one request, their names have to be distinct. But with the typesafe client, you can send multiple request only with a @Multiple
annotation. The operation name is only scoped within a query string, not globally. I think we can safely skip this. Try to write a test to prove me wrong 😜
BTW, this code does uppercase the very first letter: FirstSecondThirdSomeMynotherName
... but the F
could also remain f
😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the Quarkus tests failed, I saw that Quarkus also checks the operation name.
And maybe it’s worth making sure that the operation name is formed like this:
- If there is
@Name
or@Namespace
, capitalize the first letters. - If not, leave it as is.
As a result, the quarkus build will not crash, and there will be a minimum changes in the tests.
As for @Multiple
, I'll take a look today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm obviously biased by the Java naming convention that methods start with a lowercase character. That's why I'd like to stick with that, also for operation names. I.e., also if there is a @Namespace
annotation, the very first character should be lowercase. E.g., @Namespace("users") ... @Query List<User> all() {...
would result in an operation name usersAll
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just referred to examples from the graphql documentation https://graphql.org/learn/queries/#operation-name
But of course I can change it to your example, no problem.
@@ -34,15 +36,19 @@ public QueryBuilder(MethodInfo method) { | |||
public String build() { | |||
StringBuilder request = new StringBuilder(method.getOperationTypeAsString()); | |||
request.append(" "); | |||
request.append(method.getName()); // operationName | |||
|
|||
request.append(method.getOperationName()); // operation name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request.append(method.getOperationName()); // operation name | |
request.append(method.getOperationName()); |
List<String> namespaces = method.getNamespaces(); | ||
if (!namespaces.isEmpty()) { | ||
namespaces.forEach(value -> { | ||
request.append(" { "); | ||
request.append(value); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in the QueryBuilder.
I really look forward to get rid of this kind of code duplication when @mskacelik finishes his work.
@Retention(RetentionPolicy.RUNTIME) | ||
@Target({ ElementType.TYPE }) | ||
@Documented | ||
@Experimental("Experimental namespaces on api class. Separate namespaces via '/' (example - admin/users)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the second sentence into Javadoc. And add a hint that the class name is being used as the default value
.
return value.substring(0, 1).toUpperCase() + value.substring(1); | ||
} | ||
|
||
private void addNamespacedRootObject(GraphQLObjectType.Builder mutationBuilder, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameters can also be for queries, can't they? Maybe better rename it to, e.g., operationBuilder
/namespaceOperations
/operationType
?
|
||
GraphQLObjectType namedType = createNamedType(rootName, group, operations); | ||
if (!groupContainer.getOperations().isEmpty() || !graphQLFieldDefinitions.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!groupContainer.getOperations().isEmpty() || !graphQLFieldDefinitions.isEmpty()) { | |
if (groupContainer.getOperations().isEmpty() && graphQLFieldDefinitions.isEmpty()) { | |
return List.of(); | |
} |
objectTypeBuilder = objectTypeBuilder.field(graphQLFieldDefinition); | ||
} | ||
|
||
objectTypeBuilder.fields(graphQLFieldDefinitions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the second one unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here passed nested objects.
@Namespace("admin")
, @Namespace("admin/users")
. Each contains some operations. Here builds admin definition (with its opeartions), and in the next adds definitions 'users' and etc..
|
||
@GraphQLApi | ||
@Namespace("first/second") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so much nicer! 😁
I don't like it in this form yet. I think it would be better to roll back the changes. And expand the existing code. To enable work with Namespace, you need to add a config to switch to working with them. And by default, use what is there. With this approach, Quarkus will not break. And it will be possible to refine it (add additional configs, checks) without any problems. I will rewrite it |
I just wanted to repeat it: thanks, @RoMiRoSSaN. You're doing a great job! |
Hi, @t1 Summary at this moment
I only have a question about the operation names. Quarkus tests fails on the check operation name. How should it be done?
And I'm waiting for your questions and suggestions |
@jmartisk Please, look at this later too. When you have time |
If we would follow my comment above, the names would be |
@@ -185,7 +195,8 @@ public <T> T build(Class<T> apiClass) { | |||
endpoint, | |||
websocketUrl, executeSingleOperationsOverWebsocket, httpClient, webClient, subprotocols, | |||
websocketInitializationTimeout, | |||
allowUnexpectedResponseFields); | |||
allowUnexpectedResponseFields, | |||
namespaceAnnotation != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you pass a useNamespace
boolean when you have already looked at the annotation and can now already just pass the information about the particular namespace as a List<String>
? That would avoid having to use reflection later on.
client/model-builder/src/test/java/io/smallrye/graphql/client/model/ClientModelBuilderTest.java
Outdated
Show resolved
Hide resolved
|
||
if (nameAnnotation != null && namespaceAnnotation != null) { | ||
throw new RuntimeException("You can only use one of the annotations - @Name or @Namespace " + | ||
"over the GraphQLClientApi interface. Please, fix next interface: " + apiClass.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"over the GraphQLClientApi interface. Please, fix next interface: " + apiClass.getName()); | |
"over the GraphQLClientApi interface. Please, fix the interface: " + apiClass.getName()); |
if (!errors.isEmpty()) { | ||
throw new RuntimeException("Subscriptions can't be nested. " + | ||
"Move your subscriptions to another @GraphQLApi class, not marked @Namespace or @Name. " + | ||
"Check next places: " + String.join("; ", errors)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Check next places: " + String.join("; ", errors)); | |
"Check these places: " + String.join("; ", errors)); |
@jmartisk It might be worth adding a check that the strings in |
.collect(Collectors.toList()); | ||
if (!errorInterfaces.isEmpty()) { | ||
throw new RuntimeException("You can only use one of the annotations - @Name or @Namespace " + | ||
"over the GraphQLClientApi interface. Please, fix next interfaces: " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"over the GraphQLClientApi interface. Please, fix next interfaces: " + | |
"over the GraphQLClientApi interface. Please, fix the following interfaces: " + |
@@ -75,6 +80,19 @@ private ClientModels generateClientModels() { | |||
return clientModels; | |||
} | |||
|
|||
private void validateAnnotations(Collection<AnnotationInstance> graphQLApiAnnotations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we name this validateNamespaceAnnotations
to make it more clear what it does?
Have you got a branch with the planned Quarkus-side changes too so that I can have a look? |
@jmartisk Hi. Here are the changes with comments. They are minimal. The main thing is to remove group processing. |
Yeah let's put the |
Please, check https://github.com/RoMiRoSSaN/quarkus/tree/namespaces |
@RoMiRoSSaN I took the liberty to copy it into my own branch: https://github.com/jmartisk/quarkus/tree/smallrye-graphql-2.11 this branch will serve to capture any necessary changes that we will do in Quarkus along with upgrading to SmallRye GraphQL 2.11 (when it's released). I've also pushed a commit here to change the CI build so that the Quarkus part of the CI runs with a snapshot from that branch - so it should reflect the Quarkus-side changes. Let's see what the CI says now (it should fully pass now) |
Everything seems to be fine. |
Ok I think this is pretty much good to go. @RoMiRoSSaN please mark it as ready if you agree |
I've rebased it and squashed some commits, let's run the CI one more time |
Thanks! |
This PR is only open to see what changes may be needed to changes suggestion by @t1 from this discussion #2173.
For viewing only, not for merge.