graphql-java-generator/graphql-maven-plugin-project

DataFetchersDelegate for scalar-returning operations

mheidt opened this issue · 20 comments

mheidt commented

Hello,

Assume you have this model:

type Foo{
   foo(p: String): String
   fu(p: String): Bar
}

If I am right, DataFetcherDelegate classes will only be generated for fu, because it returns an object and not a scalar type.

I would propose an enhancement, that you generate those delegates as well when foo is defined as extension.

extend type Foo{
   foo(p: String): String
}

Without I have to solve it by defining a StringResponse type if I need a newTypeWiring.
You need to know, that I want to work with your generated classes as they are.
Or is it possible to overwrite the GraphQLProvider's buildWiring?

Thanks,
Markus

etienne-sf commented

Hello,

If I am right, DataFetcherDelegate classes will only be generated for fu, because it returns an object and not a scalar type.
Yes, that's it

generate those delegates as well when foo is defined as extension
This seems very confusing to me.

I don't understand why you need to do such a thing.
Why can't do you implementation code in the Foo data fetcher ?

BTW, I see two ideas to help your there:

  1. Have a CustomScalar for the foo attribute:
type Foo{
   foo(p: String): FooCustomScalar
   fu(p: String): Bar
}

But this won't create a DataFetcherDelegate

  1. Create a dedicated type (which will create a DataFetcherDelegate)
type Foo{
   foo(p: String): FooType
   fu(p: String): Bar
}

Etienne

etienne-sf commented

And if your objectif is to get the "p" parameter, then you retrieve it from the context, in the DataFetcherDelegate.

mheidt commented

Thanks Etienne,

I will have a look into the custom scalars.
I don't want to have a return object though, because the existing clients expect Long
And I need it, because foo is using the Batchloader mechanism. Have a look into my other issue in the server example github.
The foo datafecher get() returns the object, but I need a CompleteFuture as result...
You need to know, that I am refactoring existing code and want to use your generated classes directly and don't want to copy them into "/src" and change them here and there.

mheidt commented

Etienne,
I don't understand how scalar types can help me here
I made a local LongResponse extending GraphQLScalarType, hooked that in the plugin config as javaType Long.
Added the scalar LongResponse into the .graphqls, used it and rebuilt the sources.
I don't see it in the DataFetchersDelegate.
But when I started this issue, I wasn't aware that I rather need a BatchLoaderDelegate instead.
So maybe we should think into that direction.
Although I think, that a @directive is needed to force the builder to make an entry into the DataFetchersDelegate.

Or isn't it your goal to be schema-first(-and-ever) and expect everybody to make changes into the classes you generated?

etienne-sf commented

Hello,

Your use case is still very unclear to me.

Yes, the goal of this plugin is to be a schema-first tool.
But I can not create a DataFetcherDelegate when an attribute is added with the extend GraphQL extension: this would break existing implementation, and need additional code on server side, when this is specific to your use case.

As you said, it could be solved by a specific directive that would create a DataFetcherDelegate.

But as I don't understand your use case, I don't understand why this would be useful.

So, here what I understand:

My understanding is that you need to use the Batch Loader mechanism.
That is : the calculation for the foo attribute would me much faster if you execute its calculation for several entities in a row.

Note that, to execute the BatchLoader, you juste need an entity with an Id.

Then, you mix this with the fact that you write this type as a String, wheras you need a Long.
So you should use a Long scalar type, for instance: graphql.Scalars.GraphQLLong (see the graphql-maven-plugin-samples-allGraphQLCases-client pom file for a sample).

Then your schema would look like this:

scalar Long

type Foo{
   id: ID
   foo(p: String): FooSubType
   fu(p: String): Bar
}

type FooSubType {
   id: ID
   value: Long
}

With this schema, you'll have a Batch Loader created for the FooSubType. And the value type would be a Long, as your client code expect it.

Etienne

mheidt commented

Hello Etienne,

Ok, I will try the ID mechanism.
But in that case I need to call it with
foo { value }
?
not just foo as if it would be a Long?
Now I just create a newWiringTyp with a dataFetcher returning a CompletableFuture.
I reckon, your code will return a CompletableFuture?

But I will give it a try and might come up with more questions soon.

etienne-sf commented

Hello,

Yes, with the schema I proposed in my last message, you need to call it with
foo { value }
But this allows to have a Batch Loader

Please note that I still don't understand your use case. So, if you have further questions, please start by properly describe your use case.

mheidt commented

I have an existing application I need to refactor. The former developer used your script for an intial build only and made all the changes "inline".
I have a field configured like:
shortname: String
A new line into GraphQLProvider was added manually
.type(newTypeWiring("LocationMM").dataFetcher("shortname", locationShortnameDataFetcher))

The autowired datafetcher looks like

public class LocationShortnameDataFetcher implements DataFetcher<Object>

	@Override
	public Object get(DataFetchingEnvironment environment) throws Exception {
...
		return dataLoader.load(locationShortnameDLRequestDTO); //returning CompletableFuture<String>
	}
	
        public BatchLoader<LocationShortnameDLRequestDTO, String> locationShortnameBatchLoader() {
		return (List<LocationShortnameDLRequestDTO> keys) -> CompletableFuture.supplyAsync(() -> {
                    return locationUtil.getLocationShortnameByLocationIdsFallback(keys);
                }, new TraceableExecutorService(this.beanFactory, Executors.newSingleThreadExecutor(), "locationShortnameBatchLoader"));
        }
}

The later was used here

public class CustomGraphQLInvocation implements GraphQLInvocation {
...
DataLoader<LocationShortnameDLRequestDTO, String> locationShortnameLoader = DataLoader.newDataLoader(locationShortnameDataFetcher.locationShortnameBatchLoader());
        dataLoaderRegistry.register("locationShortnameDL", locationShortnameLoader);

I will break this and try to use your BatchLoader. But it is a little restriction of your tool, that it is not possible to return a Long only. I have to change the clients as well, which will be possible in my case, but not necessarily in everybody's.

type FooSubType {
   id: ID
   value: Long @returnValue
}

or something like that could make it possible.

mheidt commented

Etienne, do you have a sample project where I can see how your generated BatchLoader code works in action?
Or a documentation?
My existing code has this line to the "real" code

public BatchLoader<LocationShortnameDLRequestDTO, String> locationShortnameBatchLoader() {
		return (List<LocationShortnameDLRequestDTO> keys) -> CompletableFuture.supplyAsync(() -> {
                    return locationUtil.getLocationShortnameByLocationIdsFallback(keys);
                }, new TraceableExecutorService(this.beanFactory, Executors.newSingleThreadExecutor(), "locationShortnameBatchLoader"));
        }

I understand, that I have to do something like

@Component
public class ShortnameResponseDF implements DataFetchersDelegateShortnameResponseMM {

    @Autowired
    private LocationUtil locationUtil;

    @Override
    public List<ShortnameResponseMM> batchLoader(List<UUID> keys) {
        List<LocationShortnameDLRequestDTO> dtoKeys = null;
        return locationUtil.getLocationShortnameByLocationIdsFallback(dtoKeys)
            .stream().map((String s) ->{
                ShortnameResponseMM result = new ShortnameResponseMM();
                result.setValue(s);
                return result;
            }).collect(Collectors.toList());
    }
}

But how/where do I collect/create the UUID so that I have the LocationShortnameDLRequestDTO?

What is the Best Practice?
Creating a random UUID and then?

   @Override
    public CompletableFuture<ShortnameResponseMM> shortname(DataFetchingEnvironment dataFetchingEnvironment, DataLoader<UUID, ShortnameResponseMM> dataLoader, LocationMM origin) {
        return dataLoader.load(UUID.randomUUID());
    }

But how do I map from the UUID to the data in origin or dataFetchingEnvironment that I need to create the RequestDTO?
And where do I store it, so that that my DataFetcher knows about it?

I can imagine trillions of ways. But I would like to know the Best Practice, you suggest.

Thanks a lot.

mheidt commented

Etienne,

We have an operation like this

locationInUseWsCount(type: LocationWsCountType!, days: Long, applicationGroups: [String]): LocationInUseWsCount

Unfortunately the generated code doesn't compile

public DataFetcher<CompletableFuture<LocationInUseWsCount>> dataFetchersDelegateLocationMMLocationInUseWsCountWithDataLoader() {
		return dataFetchingEnvironment -> {
        LocationWsCountType type = (LocationWsCountType) graphqlUtils.getArgument(dataFetchingEnvironment.getArgument("type"), "${argument.type.graphQLTypeSimpleName}", "java.util.UUID", LocationWsCountType.class);
        Long days = (Long) graphqlUtils.getArgument(dataFetchingEnvironment.getArgument("days"), "${argument.type.graphQLTypeSimpleName}", "java.util.UUID", Long.class);
        @SuppressWarnings("unchecked")
        List<String> applicationGroups = (List<String>) graphqlUtils.getArgument(dataFetchingEnvironment.getArgument("applicationGroups"), "${argument.type.graphQLTypeSimpleName}", "java.util.UUID", String.class);
        LocationMM source = dataFetchingEnvironment.getSource();

        DataLoader<UUID, LocationInUseWsCount> dataLoader = dataFetchingEnvironment.getDataLoader("LocationInUseWsCount");

        // This dataLoader may be null. Let's hande that:
        if (dataLoader != null)
            return dataFetchersDelegateLocationMM.locationInUseWsCount(dataFetchingEnvironment, dataLoader, source, type, days, applicationGroups);
        else
            return CompletableFuture.supplyAsync(
                () -> dataFetchersDelegateLocationMM.locationInUseWsCount(dataFetchingEnvironment, source, type, days, applicationGroups));
    };

The types that are passed are the correct types. Long for days for example

But in DataFetchersDelegateLocationMM, they are

public CompletableFuture<LocationInUseWsCount> locationInUseWsCount(DataFetchingEnvironment dataFetchingEnvironment, DataLoader<UUID, LocationInUseWsCount> dataLoader, LocationMM origin, LocationInUseWsCount type, LocationInUseWsCount days, LocationInUseWsCount applicationGroups);

all LocationInUseWsCount types (the return type of the operation)

Are we doing something wrong or is this a bug?

Kind regards,
Markus

etienne-sf commented

If you have a valid GraphQL schema, and the generated code doesn't compile, then for sure, it's a bug!

If you provide me the GraphQL schema, I'll correct that.
Are you using the last plugin version, 1.11.1 ?

And yes, there is a sample on how to use BatchLoader. Take a look here: https://github.com/graphql-java-generator/GraphQL-Forum-Maven-Tutorial-server (search for BatchLoader)
It should answer your question: But how do I map from the UUID to the data in origin or dataFetchingEnvironment that I need to create the RequestDTO?

Etienne

mheidt commented

Try this

directive @RelayConnection
on FIELD_DEFINITION

 

type Query {
location(id: ID!): LocationMM!
locations(filter: FilterMM): [LocationMM!]@RelayConnection
}

 

input FilterMM {
locations: [String]
}

 

type LocationMM {
id: ID!
name: String!
locOnlineWsCount(days: Int = 0): LocationOnlineWsCount
}

 

type LocationOnlineWsCount {
id: ID!
value: Int!
}

and yes:

<dependency>
        <groupId>com.graphql-java-generator</groupId>
        <artifactId>graphql-java-runtime</artifactId>
        <version>1.11.1</version>
</dependency>
etienne-sf commented

Ok, I confirm the compilation error.

I'll work on this one quickly.

While waiting for the correction, you can manually correct the generated DataFetchersDelegateLocationMM class, I guess.

etienne-sf commented

Ok, the code now properly compiles. But the field's parameter is not transmitted yet to the dataloader.

I'll correct that, then release a 1.11.2 version

etienne-sf commented

The 1.11.2 version is beeing released, and should be available soon.

You have to activate the generateBatchLoaderEnvironment plugin parameter to true, in the pom.xml.
All your batchLoader method will need a new parameter (environment).

You'll find a samples in the allGraphQLCases sample:

  1. How to associate a parameter to an id:
    Take a look at the oneWithIdSubType method of this class:
    https://github.com/graphql-java-generator/graphql-maven-plugin-project/blob/master/graphql-maven-plugin-samples/graphql-maven-plugin-samples-allGraphQLCases-server/src/main/java/org/allGraphQLCases/server/impl/DataFetchersDelegateAllFieldCasesImpl.java

  2. How to retrieve this parameter from an id:
    https://github.com/graphql-java-generator/graphql-maven-plugin-project/blob/master/graphql-maven-plugin-samples/graphql-maven-plugin-samples-allGraphQLCases-server/src/main/java/org/allGraphQLCases/server/impl/DataFetchersDelegateAllFieldCasesWithIdSubtypeImpl.java

mheidt commented

Etienne, thanks for the quick fix.
And for the sample at 2.

Could you please explain, why it is not possible to define a directive the generator is looking for in order to do exactly the same as if the return value would contain an ID.

So that we can make
foo(p: String): FooSubType
a
foo(p: String): String @yourdirective

Internally you could create a FooString class with an ID and process it as usual.

etienne-sf commented

Of course.
But this is a very specific need.

I'm already spending a lot of my personal time for the plugin.
So I won't develop such specific need (at least, not for free). My opinion is that you can do it, with a separate GraphQL type. The only consequence is to add a getter. It seems quite good and simple...

mheidt commented

Ok, personally I can change my clients.
But I think you need this feature, when your user base grows, that is really using your plugin in every build and doesn't use it to generate an initial template to start with.

etienne-sf commented

BTW, the plug-in already allows you to create your own templates.

Take a look at the custom templates feature.