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

[BUG] @RequestMapping annotation not allowed on @FeignClient interfaces #13488

Closed
4 of 6 tasks
cTAbdIsi opened this issue Sep 21, 2022 · 15 comments · Fixed by #15898
Closed
4 of 6 tasks

[BUG] @RequestMapping annotation not allowed on @FeignClient interfaces #13488

cTAbdIsi opened this issue Sep 21, 2022 · 15 comments · Fixed by #15898

Comments

@cTAbdIsi
Copy link

cTAbdIsi commented Sep 21, 2022

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

Code generation with plugin failed with error @RequestMapping annotation not allowed on @FeignClient interfaces after update to 6.1.0

openapi-generator version

6.1.0

Generation Details

This is our configuration for the maven plugin:

<plugin>
    <groupId>org.openapitools</groupId>
    <artifactId>openapi-generator-maven-plugin</artifactId>
    <version>${openapi-generator-maven-plugin.version}</version>
    <configuration>
        <configOptions>
            <dateLibrary>java8</dateLibrary>
            <disableHtmlEscaping>true</disableHtmlEscaping>
            <hateoas>false</hateoas>
            <hideGenerationTimestamp>true</hideGenerationTimestamp>
            <interfaceOnly>true</interfaceOnly>
            <sourceFolder>models</sourceFolder>
            <useTags>true</useTags>
        </configOptions>
        <generateModels>true</generateModels>
        <generatorName>spring</generatorName>
        <library>spring-cloud</library>
        <skipIfSpecIsUnchanged>true</skipIfSpecIsUnchanged>
    </configuration>
    <executions>
        <execution>
            <id>generate-api</id>
            <goals>
                <goal>generate</goal>
            </goals>
            <configuration>
                <apiPackage>${project.groupId}.${project.artifactId}.api.v1</apiPackage>
                <inputSpec>${project.basedir}/docs/openapi.yaml</inputSpec>
                <modelPackage>${project.groupId}.${project.artifactId}.api.v1.model</modelPackage>
                <modelNameSuffix>ApiV1</modelNameSuffix>
            </configuration>
        </execution>
    </executions>
</plugin>

In openapi-generator version < 6.1.0, the plugin generate following code:

@Generated(value = "org.openapitools.codegen.languages.SpringCodegen")
@Validated
@Tag(name = "Test")
public interface TestApi {
...
}

This is the result after updating to 6.1.0+.

@Generated(value = "org.openapitools.codegen.languages.SpringCodegen")
@Validated
@Tag(name = "Test")
@RequestMapping("${openapi.localServices.base-path:/test/v1}")
public interface TestApi {

We use this interface to setup the feign clients like this:

@FeignClient(name = "test",
             url = "${sdu.test.url:}",
             configuration = {TestApiConfig.class, ApiClientConfig.class})
public interface TestApiClient extends TestApi {
}
Steps to reproduce
Related issues/PRs
Suggest a fix
@dersvenhesse
Copy link
Contributor

The related change is probably #10573 where @RequestMapping was moved from the controller to the interface.

@MelleD
Copy link
Contributor

MelleD commented Sep 27, 2022

@RomainPruvostMHH and @welshm could you please habe look and revert this change?

@welshm
Copy link
Contributor

welshm commented Sep 28, 2022

@RomainPruvostMHH and @welshm could you please habe look and revert this change?

I can try to take a look at this today or tomorrow

@MelleD
Copy link
Contributor

MelleD commented Sep 28, 2022

@welshm i commented the issue in der merged PR modules/openapi-generator/src/main/resources/JavaSpring/api.mustache

Not sure if we have a condition, that we know that we use Feign on this time and can add @RequestMapping just if we don't use Feign.

@welshm
Copy link
Contributor

welshm commented Sep 28, 2022

@welshm i commented the issue in der merged PR modules/openapi-generator/src/main/resources/JavaSpring/api.mustache

Not sure if we have a condition, that we know that we use Feign on this time and can add @RequestMapping just if we don't use Feign.

I think we have a condition to check (because it exists in other .mustache) but I will verify and post back here

@MelleD
Copy link
Contributor

MelleD commented Sep 28, 2022

@welshm i commented the issue in der merged PR modules/openapi-generator/src/main/resources/JavaSpring/api.mustache
Not sure if we have a condition, that we know that we use Feign on this time and can add @RequestMapping just if we don't use Feign.

I think we have a condition to check (because it exists in other .mustache) but I will verify and post back here

Ah nice, then i think it's an easy fix. Maybe i can look into it tomorrow

@MelleD
Copy link
Contributor

MelleD commented Sep 28, 2022

@welshm found no condition. Thought {{^useSpringCloudClient}} could work, but did not find where this condition is set.

Tested it with

   @Test
   public void testNoRequestMappingAnnotation() throws IOException {
      final SpringCodegen codegen = new SpringCodegen();
      codegen.setLibrary( "spring-cloud" );

      final Map<String, File> files = generateFiles( codegen, "src/test/resources/2_0/petstore.yaml" );

      // Check that the @RequestMapping annotation is not generated in the Api file
      final File petApiFile = files.get( "PetApi.java" );
      JavaFileAssert.assertThat( petApiFile ).assertTypeAnnotations().hasSize( 3 ).containsWithName( "Validated" )
            .containsWithName( "Generated" ).containsWithName( "Tag" );

   }

@MelleD
Copy link
Contributor

MelleD commented Sep 28, 2022

@welshm created a new boolean for the feign client:
#13546

If this is okay, i would adapt the other test(s)

@DarrMirr
Copy link

DarrMirr commented Oct 6, 2022

Please, fix this bug, because Spring Framework expects annotation @RequestMapping on type-level and start to register request mapping for such class. Such bug make impossible to use Feign client due to ambiguous mapping.

Here is code sample from Spring source code :

public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMapping
		implements MatchableHandlerMapping, EmbeddedValueResolverAware {

	/**
	 * {@inheritDoc}
	 * <p>Expects a handler to have either a type-level @{@link Controller}
	 * annotation or a type-level @{@link RequestMapping} annotation.
	 */
	@Override
	protected boolean isHandler(Class<?> beanType) {
		return (AnnotatedElementUtils.hasAnnotation(beanType, Controller.class) ||
				AnnotatedElementUtils.hasAnnotation(beanType, RequestMapping.class));
	}
}

@welshm
Copy link
Contributor

welshm commented Oct 6, 2022

Please, fix this bug, because Spring Framework expects annotation @RequestMapping on type-level and start to register request mapping for such class. Such bug make impossible to use Feign client due to ambiguous mapping.

Here is code sample from Spring source code :

public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMapping
		implements MatchableHandlerMapping, EmbeddedValueResolverAware {

	/**
	 * {@inheritDoc}
	 * <p>Expects a handler to have either a type-level @{@link Controller}
	 * annotation or a type-level @{@link RequestMapping} annotation.
	 */
	@Override
	protected boolean isHandler(Class<?> beanType) {
		return (AnnotatedElementUtils.hasAnnotation(beanType, Controller.class) ||
				AnnotatedElementUtils.hasAnnotation(beanType, RequestMapping.class));
	}
}

Are you suggesting that the annotation should be on the controller implementation as opposed to the interface?

Can you take a look at the output from #13546 and see if that is what you'd expect? That will remove the annotation from the interface, but does not re-add it to the controller.

@DarrMirr
Copy link

Hello @welshm. Thank you for response and link to #13546. I think it solve this issue.

@vvinod1310
Copy link

Hello,

May I know when this fix would be released?

Thank you!

@borsch
Copy link
Member

borsch commented Jan 13, 2023

@vvinod1310 this is part of 6.2.1

@borsch borsch closed this as completed Jan 15, 2023
@ctbarnev
Copy link
Contributor

Is anyone else encountering this bug when using the Kotlin-Spring generator with library spring-cloud?

We're using openapi-generator version: 6.6.0

Our generated interface:

@Validated
@RequestMapping("\${api.base-path:/api/test/v1}")
interface TestApi {

The generated FeignClient:

@FeignClient(
    name="Test",
    url="\${test.url:http://localhost/api/test/v1}", 
    configuration = [ClientConfiguration::class]
)
interface TestApiClient : TestApi

Then when running our application we encounter the same exception:
@RequestMapping annotation not allowed on @FeignClient interfaces

Think the fix made in this pull request should also be implemented in the Kotlin-Spring generator right? Or am I missing something?

@ctbarnev
Copy link
Contributor

I created a MR to fix this issue also for the Kotlin-Spring generator, hopefully someone can have a look?

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

Successfully merging a pull request may close this issue.

8 participants