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.

Sign up for GitHub

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

Jump to bottom

Add @Namespace for GraphQLApi and GraphQLClientApi #2184

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+1,031 −547
Open

Add @Namespace for GraphQLApi and GraphQLClientApi #2184

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d02c67f
Replace using @Name to @Namespace on GraphQLApi and GraphQLClientApi
Sep 6, 2024
10c16d2
Return groups, separate logic between Name and Namespace, add some te…
Sep 11, 2024
eb9df1a
Merge branch 'main' into namespace
Sep 11, 2024
536955a
Without namespace make operationName without capitalize first letter,…
Sep 12, 2024
cc93826
Remove group, refactor for working with namespaces
Sep 19, 2024
1d7ad70
simple code changes, update docs
Sep 22, 2024
698fd8d
fix exception messages
Sep 22, 2024
80c9496
Switch CI to custom branch for 2.11
jmartisk Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../main/java/io/smallrye/graphql/client/vertx/typesafe/VertxTypesafeGraphQLClientProxy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ private JsonObject request(MethodInvocation method) {
}
request.add("query", query);
request.add("variables", variables(method));
request.add("operationName", method.getName());
request.add("operationName", method.getOperationName());
JsonObject result = request.build();
log.tracef("full graphql request: %s", result.toString());
return result;
Expand Down
25 changes: 16 additions & 9 deletions ...t/implementation/src/main/java/io/smallrye/graphql/client/impl/typesafe/QueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,19 @@ public QueryBuilder(MethodInvocation method) {
public String build() {
StringBuilder request = new StringBuilder(method.getOperationTypeAsString());
request.append(" ");
request.append(method.getName());
if (method.hasValueParameters())

request.append(method.getOperationName()); // operation name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
request.append(method.getOperationName()); // operation name
request.append(method.getOperationName());


if (method.hasValueParameters()) {
request.append(method.valueParameters().map(this::declare).collect(joining(", ", "(", ")")));
}

String groupName = method.getGroupName();
if (groupName != null) {
request.append(" { ");
request.append(groupName);
List<String> namespaces = method.getNamespaces();
if (!namespaces.isEmpty()) {
namespaces.forEach(value -> {
request.append(" { ");
request.append(value);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!namespaces.isEmpty()) {
namespaces.forEach(value -> {
request.append(" { ");
request.append(value);
});
}
namespaces.forEach(namespace -> request.append(" { ").append(namespace)});


if (method.isSingle()) {
Expand All @@ -45,11 +50,13 @@ public String build() {

request.append(fields(method.getReturnType()));

if (method.isSingle())
if (method.isSingle()) {
request.append(" }");
}

if (groupName != null)
request.append(" } ");
if (!namespaces.isEmpty()) {
request.append(" } ".repeat(namespaces.size()));
}

return request.toString();
}
Expand Down
9 changes: 5 additions & 4 deletions .../implementation/src/main/java/io/smallrye/graphql/client/impl/typesafe/ResultBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,11 @@ public Object read() {
private JsonObject readData() {
if (!response.containsKey("data") || response.isNull("data"))
return null;
String groupName = method.getGroupName();
JsonObject data = groupName != null
? response.getJsonObject("data").getJsonObject(groupName)
: response.getJsonObject("data");

JsonObject data = response.getJsonObject("data");
for (String namespace : method.getNamespaces()) {
data = data.getJsonObject(namespace);
}
if (method.isSingle() && !data.containsKey(method.getName()))
throw new InvalidResponseException("No data for '" + method.getName() + "'");
return data;
Expand Down
33 changes: 22 additions & 11 deletions ...n/src/main/java/io/smallrye/graphql/client/impl/typesafe/reflection/MethodInvocation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.smallrye.graphql.client.impl.typesafe.reflection;

import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;

import java.io.Closeable;
Expand All @@ -10,10 +11,12 @@
import java.lang.reflect.Modifier;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;

Expand All @@ -23,6 +26,7 @@
import org.eclipse.microprofile.graphql.Name;
import org.eclipse.microprofile.graphql.Query;

import io.smallrye.graphql.api.Namespace;
import io.smallrye.graphql.api.Subscription;
import io.smallrye.graphql.client.core.OperationType;
import io.smallrye.graphql.client.model.MethodKey;
Expand All @@ -36,14 +40,14 @@ public static MethodInvocation of(Method method, Object... args) {
private final TypeInfo type;
private final Method method;
private final Object[] parameterValues;
private final String groupName;
private final List<String> namespaces;
private List<ParameterInfo> parameters;

private MethodInvocation(TypeInfo type, Method method, Object[] parameterValues) {
this.type = type;
this.method = method;
this.parameterValues = parameterValues;
this.groupName = readGroupName(method);
this.namespaces = readNamespace(method);
}

@Override
Expand Down Expand Up @@ -262,18 +266,25 @@ public String getOperationTypeAsString() {
}
}

public String getGroupName() {
return groupName;
public List<String> getNamespaces() {
return namespaces;
}

private String readGroupName(Method method) {
Name annotation = method.getDeclaringClass().getAnnotation(Name.class);
private List<String> readNamespace(Method method) {
Namespace annotation = method.getDeclaringClass().getAnnotation(Namespace.class);
if (annotation != null) {
String groupName = annotation.value().trim();
if (!groupName.isEmpty()) {
return groupName;
}
return Arrays.stream(annotation.value().split("/"))
.map(String::trim)
.collect(Collectors.toList());
}
return null;
return List.of();
}

public String getOperationName() {
return namespaces.stream().map(this::prettyString).collect(joining()) + prettyString(getName());
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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 😁

Copy link
Contributor Author

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:

  1. If there is @Name or @Namespace, capitalize the first letters.
  2. 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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

}

private String prettyString(String value) {
return value.substring(0, 1).toUpperCase() + value.substring(1);
}
}
1 change: 1 addition & 0 deletions client/model-builder/src/main/java/io/smallrye/graphql/client/model/Annotations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ private static Map<DotName, AnnotationInstance> getAnnotationsWithFilter(Type ty
public static final DotName TYPE = DotName.createSimple("org.eclipse.microprofile.graphql.Type");
public static final DotName INTERFACE = DotName.createSimple("org.eclipse.microprofile.graphql.Interface");
public static final DotName UNION = DotName.createSimple("io.smallrye.graphql.api.Union");
public static final DotName NAMESPACE = DotName.createSimple("io.smallrye.graphql.api.Namespace");

public static final DotName MULTIPLE = DotName.createSimple("io.smallrye.graphql.client.typesafe.api.Multiple");

Expand Down
10 changes: 10 additions & 0 deletions client/model-builder/src/main/java/io/smallrye/graphql/client/model/ClientModelBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package io.smallrye.graphql.client.model;

import static io.smallrye.graphql.client.model.Annotations.GRAPHQL_CLIENT_API;
import static io.smallrye.graphql.client.model.Annotations.NAME;
import static io.smallrye.graphql.client.model.ScanningContext.getIndex;

import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import org.jboss.jandex.AnnotationInstance;
import org.jboss.jandex.ClassInfo;
Expand Down Expand Up @@ -56,6 +58,14 @@ private ClientModels generateClientModels() {
Collection<AnnotationInstance> graphQLApiAnnotations = getIndex()
.getAnnotations(GRAPHQL_CLIENT_API);

List<String> errors = graphQLApiAnnotations.stream()
.filter(annotationInstance -> annotationInstance.target().asClass().hasDeclaredAnnotation(NAME))
.map(apiClass -> "Use @Namespace instead of @Name on interface " + apiClass.target().asClass().name())
.collect(Collectors.toList());
if (!errors.isEmpty()) {
throw new RuntimeException(String.join("\n", errors));
}

graphQLApiAnnotations.forEach(graphQLApiAnnotation -> {
ClientModel operationMap = new ClientModel();
ClassInfo apiClass = graphQLApiAnnotation.target().asClass();
Expand Down
24 changes: 16 additions & 8 deletions client/model-builder/src/main/java/io/smallrye/graphql/client/model/QueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import static io.smallrye.graphql.client.model.helper.OperationModel.of;
import static java.util.stream.Collectors.joining;

import java.util.List;

import org.jboss.jandex.MethodInfo;

import io.smallrye.graphql.client.model.helper.DirectiveInstance;
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
request.append(method.getOperationName()); // operation name
request.append(method.getOperationName());


if (method.hasValueParameters()) {
request.append(method.valueParameters().stream().map(method::declare).collect(joining(", ", "(", ")")));
}

String groupName = method.getGroupName();
if (groupName != null) {
request.append(" { ");
request.append(groupName);
List<String> namespaces = method.getNamespaces();
if (!namespaces.isEmpty()) {
namespaces.forEach(value -> {
request.append(" { ");
request.append(value);
});
Copy link
Collaborator

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.

}

if (method.isSingle()) {
Expand All @@ -61,11 +67,13 @@ public String build() {

request.append(method.fields(method.getReturnType()));

if (method.isSingle())
if (method.isSingle()) {
request.append(" }");
}

if (groupName != null)
request.append(" } ");
if (!namespaces.isEmpty()) {
request.append(" } ".repeat(namespaces.size()));
}

return request.toString();
}
Expand Down
37 changes: 21 additions & 16 deletions ...t/model-builder/src/main/java/io/smallrye/graphql/client/model/helper/OperationModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@
import static io.smallrye.graphql.client.model.Annotations.MULTIPLE;
import static io.smallrye.graphql.client.model.Annotations.MUTATION;
import static io.smallrye.graphql.client.model.Annotations.NAME;
import static io.smallrye.graphql.client.model.Annotations.NAMESPACE;
import static io.smallrye.graphql.client.model.Annotations.QUERY;
import static io.smallrye.graphql.client.model.Annotations.SUBCRIPTION;
import static io.smallrye.graphql.client.model.ScanningContext.getIndex;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;

import java.util.Arrays;
import java.util.List;
import java.util.Optional;
import java.util.Stack;
Expand Down Expand Up @@ -36,7 +38,7 @@ public class OperationModel implements NamedElement {
private final Stack<String> expressionStack = new Stack<>();
private Stack<TypeModel> rawParametrizedTypes = new Stack<>();
private final List<DirectiveInstance> directives;
private final String groupName;
private final List<String> namespaces;

/**
* Creates a new {@code OperationModel} instance based on the provided Jandex {@link MethodInfo}.
Expand All @@ -50,7 +52,7 @@ public class OperationModel implements NamedElement {
getDirectiveLocation(), AnnotationTarget.Kind.METHOD)
.map(DirectiveInstance::of)
.collect(toList());
this.groupName = readGroupName(method);
this.namespaces = readNamespace(method);
}

/**
Expand Down Expand Up @@ -391,22 +393,25 @@ private boolean isRawParametrizedType(TypeModel type) {
return type.isCustomParametrizedType() && !type.getFirstRawType().isTypeVariable();
}

public String getGroupName() {
return groupName;
public List<String> getNamespaces() {
return namespaces;
}

private String readGroupName(MethodInfo method) {
List<AnnotationInstance> annotationInstances = method.declaringClass().annotations(NAME);
for (AnnotationInstance annotationInstance : annotationInstances) {
if (annotationInstance.target().kind() == AnnotationTarget.Kind.CLASS) {
if (annotationInstance.target().asClass().name().equals(method.declaringClass().name())) {
String groupName = annotationInstance.value().asString().trim();
if (!groupName.isEmpty()) {
return groupName;
}
}
}
private List<String> readNamespace(MethodInfo method) {
AnnotationInstance annotationInstance = method.declaringClass().declaredAnnotation(NAMESPACE);
if (annotationInstance != null) {
return Arrays.stream(annotationInstance.value().asString().split("/"))
.map(String::trim)
.collect(Collectors.toList());
}
return null;
return List.of();
}

public String getOperationName() {
return namespaces.stream().map(this::prettyString).collect(joining()) + prettyString(getName());
}

private String prettyString(String value) {
return value.substring(0, 1).toUpperCase() + value.substring(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

}
}
Loading
Loading