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.

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 custom scalar support to client value formatter #2083

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sap-ali
Copy link
Contributor

@sap-ali sap-ali commented Apr 10, 2024

In the event that a request contains fields that are objects and within those objects there are field definitions with a type other than the primitives in the ValueFormatter, such as:

class CreateEntity {
    List<DependentEntity> dependent;
}
class DependentEntity {
    MyCustomScalar value;
}
class MyCustomScalar implements CustomStringScalar {
...
}

an IllegalStateException is thrown as below:

java.lang.IllegalStateException: Could not format class MyCustomScalar: Unsupported type.
	at io.smallrye.graphql.client.impl.core.utils.ValueFormatter.format(ValueFormatter.java:48)
	at io.smallrye.graphql.client.impl.core.InputObjectFieldImpl.build(InputObjectFieldImpl.java:14)
	at io.smallrye.graphql.client.impl.core.InputObjectImpl.build(InputObjectImpl.java:14)
	at io.smallrye.graphql.client.impl.core.utils.ValueFormatter.format(ValueFormatter.java:34)
	at io.smallrye.graphql.client.impl.core.utils.ValueFormatter._processIterable(ValueFormatter.java:63)
	at io.smallrye.graphql.client.impl.core.utils.ValueFormatter.format(ValueFormatter.java:41)
	at io.smallrye.graphql.client.impl.core.InputObjectFieldImpl.build(InputObjectFieldImpl.java:14)
	at io.smallrye.graphql.client.impl.core.InputObjectImpl.build(InputObjectImpl.java:14)
	at io.smallrye.graphql.client.impl.core.utils.ValueFormatter.format(ValueFormatter.java:34)
	at io.smallrye.graphql.client.impl.core.ArgumentImpl.build(ArgumentImpl.java:12)
	at io.smallrye.graphql.client.impl.core.FieldImpl._buildArgs(FieldImpl.java:45)
	at io.smallrye.graphql.client.impl.core.FieldImpl.build(FieldImpl.java:19)
	at io.smallrye.graphql.client.impl.core.OperationImpl.lambda$buildWrapper$0(OperationImpl.java:77)
	at java.base/java.util.Arrays$ArrayList.forEach(Arrays.java:4305)
	at io.smallrye.graphql.client.impl.core.OperationImpl.buildWrapper(OperationImpl.java:77)
	at io.smallrye.graphql.client.impl.core.OperationImpl._buildFields(OperationImpl.java:68)
	at io.smallrye.graphql.client.impl.core.OperationImpl.build(OperationImpl.java:43)
	at io.smallrye.graphql.client.impl.core.DocumentImpl.build(DocumentImpl.java:11)
	at io.smallrye.graphql.client.vertx.dynamic.VertxDynamicGraphQLClient.buildRequest(VertxDynamicGraphQLClient.java:384)
	at io.smallrye.graphql.client.vertx.dynamic.VertxDynamicGraphQLClient.executeSync(VertxDynamicGraphQLClient.java:115)

However, when the field is actually a custom scalar, the formatter should be able to support the serialization. This PR fixes the issue with the formatter.

Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

LGTM

@phillip-kruger
Copy link
Member

Can you do do a full build locally and commit again (so the formatter wont complain)

@jmartisk
Copy link
Member

jmartisk commented Apr 11, 2024

I'm not sure how exactly this is intended to be used - AFAIR we don't have any CustomScalar support in the client library, so I can't imagine how this could be put to work - but maybe I'm missing something - could you demonstrate it with a higher level test or something?

@phillip-kruger
Copy link
Member

Maybe from another client (like JavaScript ?)

@jmartisk
Copy link
Member

No, this is a change in our client's code.
I never tried to use CustomScalars with a client, so I'm surprised this (maybe) somehow works. If it does, we should have an integration test as well as some documentation to demonstrate it

@phillip-kruger
Copy link
Member

O, sorry, I thought this was the server side. Ignore me :)

@t1
Copy link
Collaborator

t1 commented Apr 11, 2024

I think the problem is that the CustomStringScalar et.al. are not part of the client API. The dependencies of the modules should prevent this code from building, shouldn't it?

@t1
Copy link
Collaborator

t1 commented Apr 11, 2024

BTW: I didn't want to say that it would not be useful to have this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants