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

graphql-maven-plugin is a Maven Plugin for GraphQL, based on graphql-java. It accelerates the development for both the client and the server, by generating the Java code. It allows a quicker development when in contract-first approach, by avoiding to code the boilerplate code.
https://graphql-maven-plugin-project.graphql-java-generator.com
MIT License
115 stars 47 forks source link

spring-graphql and ambiguous mapping #214

Open mheidt opened 1 week ago

mheidt commented 1 week ago

Hello Etienne,

After a couple of years I found the courage to make the jump to version 2. But I need a little help. I tried this: public class ApplicationMMTC extends de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController{

A little hint for your documentation: You are saying that this would have worked: public class ApplicationMMController extends de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController{

But I needed to use a different name!

But when I try to override the SchemaMapping @Override @SchemaMapping(field = "contacts") public Object contacts(DataFetchingEnvironment dataFetchingEnvironment, de.lsy.cmon.aggregator.graphql.service.ApplicationMM origin, @Argument("contactFilter") de.lsy.cmon.aggregator.graphql.service.ContactFilter contactFilter) {

I get a Factory method 'graphQlSource' threw exception; nested exception is java.lang.IllegalStateException: Ambiguous mapping. Cannot map 'applicationMMTC' method

Isn't it possible to really override the mapping or do I have to use different names? Or is there a magic to deactivate some mappings of the generated Controller?

Cheers, Markus

mheidt commented 4 days ago

@etienne-sf Hello Etienne, It seems, that this is a limination of spring-graphql? Shouldn' I be able to force the generator to only create unmapped functions for certain mapping I intend to overwrite? That way I can put those functions including the @SchemaMapping in an overriding class. If you could ommit @SchemaMapping annotation in the Controller class, which I can list in the pom?

etienne-sf commented 4 days ago

Hello,

I tried to repeat the issue, but had no time to really dig into it.

The integration tests contain an overriding of a controller. But it may differ from your use case, as:

I will give it another try.

In the meantime, can you confirm that your overriding class has these annotations: ´´´ @Controller @SchemaMapping(typeName=xxx) ´´´ ?

Étienne

mheidt commented 4 days ago

etienne-214.zip I attached some files with information

also the jar that contains the schema ctraffic-graphql-schema-0.4.63.zip

mheidt commented 4 days ago

When I use the same name for the TypeController, I get this error: Failed to parse configuration class [de.lsy.cmon.aggregator.AggregatorApp]; nested exception is org.springframework.context.annotation.ConflictingBeanDefinitionException: Annotation-specified bean name 'applicationMMController' for bean class [de.lsy.cmon.aggregator.service.graphql.typecontroller.ApplicationMMController] conflicts with existing, non-compatible bean definition of same name and class [de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController]

And I guess, if I use a different one, this might interfere: `GraphQLPluginAutoConfiguration

@Bean @ConditionalOnMissingBean(name = "applicationMMController") @SuppressWarnings("static-method") de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController applicationMMController(BatchLoaderRegistry registry) { return new de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController(registry); } `

But what about this comment in your generated code? // This annotation is used to maintain compatibility with earlier version of the // plugin. Code that uses Spring Boot annotations should use remove this method // and use the @BatchMapping annotation instead

etienne-sf commented 1 day ago

Hello,

This issue is related to Spring, and the constraint you add by with your overriding bean having a different name. I can repeat this issue, with a different name.

So the first question is: why do you need to have a different name?

This stackoverflow post post explains that each method is saved as a bean. So you need to have the overriding mapping to have the same name (which includes the class name). A workaround can be found in this spring github issue.

This is mainly a spring issue.

The other way, would be that the generated code doesn't declare the controller you want to override as being a controller. But this would be complex to implement and most important, complex to use.

Or perhaps you have another idea ?

Etienne

mheidt commented 1 day ago

If I try to extend with same name, I get this error during startup:

Caused by: org.springframework.context.annotation.ConflictingBeanDefinitionException: Annotation-specified bean name 'applicationMMController' for bean class [de.lsy.cmon.aggregator.service.graphql.typecontroller.ApplicationMMController] conflicts with existing, non-compatible bean definition of same name and class [de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController]

mheidt commented 1 day ago

I found a way but first I don't know if it is really working..at least the server starts up. And secondly I would need changes from your side.

I changed your generated code, so that there are delegate methods:

@SchemaMapping(field = "applicationServers") // This annotation is used to maintain compatibility with earlier version of the
                                    // plugin. Code that uses Spring Boot annotations should use remove this method
                                    // and use the @BatchMapping annotation instead
    public Object applicationServers(DataFetchingEnvironment dataFetchingEnvironment            , de.lsy.cmon.aggregator.graphql.service.ApplicationMM origin,
            @Argument("filter") de.lsy.cmon.aggregator.graphql.service.ApplicationServerFilter filter) {
        return  this.applicationServersDelegate(dataFetchingEnvironment, origin , filter);
    }

    protected Object applicationServersDelegate(DataFetchingEnvironment dataFetchingEnvironment         , de.lsy.cmon.aggregator.graphql.service.ApplicationMM origin,
                                     @Argument("filter") de.lsy.cmon.aggregator.graphql.service.ApplicationServerFilter filter) {
        return  this.dataFetchersDelegateApplicationMM.applicationServers(dataFetchingEnvironment, origin , filter);
    }

Then I could do something like this:

@Controller()
@ConditionalOnMissingBean(name="applicationMMController")
public class ApplicationMMTC extends de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController {

...

@Override
public Object applicationServersDelegate(DataFetchingEnvironment dataFetchingEnvironment            , de.lsy.cmon.aggregator.graphql.service.ApplicationMM origin,
                                     @Argument("filter") de.lsy.cmon.aggregator.graphql.service.ApplicationServerFilter filter) {

But in my GraphqlConfig I also need this:

@Bean(name = "applicationMMController")
@Primary
public ApplicationMMController customApplicationMMController(BatchLoaderRegistry registry) {
    return new ApplicationMMTC(registry);  // Ensure this returns your custom controller
}

I am not quite happy, because it's a lot of boilerplate code, you still need on my side.

etienne-sf commented 1 day ago

If I try to extend with same name, I get this error during startup:

Caused by: org.springframework.context.annotation.ConflictingBeanDefinitionException: Annotation-specified bean name 'applicationMMController' for bean class [de.lsy.cmon.aggregator.service.graphql.typecontroller.ApplicationMMController] conflicts with existing, non-compatible bean definition of same name and class [de.lsy.cmon.aggregator.graphql.service.util.ApplicationMMController]

I've done it twice in the graphql-maven-plugin-samples-allGraphQLCases-server sample, for integration tests. I didn't encounter this issue.

Do you have exactly the same annotations on:

etienne-sf commented 1 day ago

About the proposal you did, with the delegate methods, it's a nice direction. But there already is a delegation, with the DataFetcherDelegate. What's the real gain, here ?

So the question is ! why do you want to override the Controller? My understanding is that it's because of one of these:

So, to begin with, can you precise the reason why you need to overide the generated mapping ?

etienne-sf commented 1 day ago

And a last thought: can you take a look at the discussion in issue #215, especially this message. It may be the solution to this issue.

(but I'm still interested in your response to my previous question)

mheidt commented 21 hours ago

@etienne-sf I need to override it, because I want to batchload some paths which your generator didn't generate the proper code for.

 DataLoader<BatchloaderKey, List<ContactMM>> dataLoader = dataLoaderRegistry.getDataLoader(ContactBatchloader.BATCHLOADER_NAME); // Holen Sie sich den DataLoader aus dem DataLoaderRegistry
        return dataFetchersDelegate.contactsEx(dataFetchingEnvironment,
            dataLoader,
            origin,
            dataFetchingEnvironment.getArguments());

But you are right. Let me move that part to the delegator.

mheidt commented 21 hours ago

I have my own SpringApplication class and this was missing:

// To allow Controller overriding, the controller are declared as @Controller (mandatory for the AnnotatedControllerResolver), 
        // but they must be found by the autoconfiguration class to allow them to be overriding.
        // So we prevent the default component scan to find them. They will be available as default bean in GraphQLPluginAutoConfiguration 
        excludeFilters = {@Filter(type = FilterType.REGEX, pattern = "de.lsy.cmon.aggregator.graphql.service.util\\.[^.]*Controller") }
mheidt commented 21 hours ago

Regarding the #215. I don't have that need to have less delegates. And I like, that I see at startup, if I missed a delegator implementation. And as this is the spring way, I don't need any changes there.

mheidt commented 20 hours ago

In the past (version 1), I had the following structure in my delegate implementation:


 @Deprecated // use contactsEx instead
    @Override
    public List<ContactMM> contacts(DataFetchingEnvironment dataFetchingEnvironment, ApplicationMM origin, ContactFilter contactFilter) {
        return origin.getContacts();
    }

    @Override
    public CompletableFuture<List<ContactMM>> contactsEx(DataFetchingEnvironment dataFetchingEnvironment, DataLoader<BatchloaderKey, List<ContactMM>> dataLoader, ApplicationMM origin, Map<String, Object> payload) {

How is it possible then, that I return the dataloader CompletableFuture instead?

mheidt commented 20 hours ago

I guess that you have to make the delegate functions return Object as well? So that we can close this one and the real issue is #216?