spring-data-rest icon indicating copy to clipboard operation
spring-data-rest copied to clipboard

Feature regression : Spring Data REST controller must not use @RequestMapping on class level as this would cause double registration with Spring MVC

Open yyvess opened this issue 2 years ago • 6 comments

On latest version @RequestMapping isn't more allowed at class level due to this change =>

https://github.com/spring-projects/spring-data-rest/commit/46dc6e03fcd13cadfafeab22ba813c12a8dc9688

@RequestMapping is now only allowed on method level but that isn't an valid option for method on an abstract controller.

@RepositoryRestController
@RequestMapping("/api/widgets")
public class WidgetController  extends BaseControler {
    // ...
}

public class BaseControler {
    @GetMapping("/get")
    public ResponseEntity<String> get() {
        return ResponseEntity.ok().body(....);
    }
}

Proposition of fix =>

Add a field "path/value" on the annotation @RepositoryRestController to be able to set a root path at class level.

@RepositoryRestController("/api/widgets")
@RequiredArgsConstructor
public class WidgetController  extends BaseControler {
    // ...
}
public class BaseControler {
    @GetMapping("/get")
    public ResponseEntity<String> get() {
        return ResponseEntity.ok().body(....);
    }
}

Stackoverflow discussion => https://stackoverflow.com/questions/69825704/spring-data-rest-controller-must-not-use-requestmapping-on-class-level-as-this

yyvess avatar Nov 18 '21 09:11 yyvess

I was hit also by this change and moving request mapping into class level does behave differently than mapping on class level. There is perhaps involved some miss-configuration (or miss-use) of paths. My change on simple (showcase for this problematic update) project, which caused uri routing for my case to broke: https://github.com/luvarqpp/poc-springRequestMapping/commit/5606c6e0faf1c19be59f567c85fa2d1f72d51c7d

luvarqpp avatar Dec 01 '21 12:12 luvarqpp

This issue is also blocking us from upgrading to 2.5.6 or later. We do use abstract controllers extensively and we know of no other work-around. Any chance we could get some movement on the associated pull request?

terje2001 avatar Jan 24 '22 21:01 terje2001

We unfortunately cannot move back to the original arrangement as those declarations, in combination with how Spring MVC handles controllers in the 5.x generation, is effectively the reason for a CVE.

I'll review what it'd mean to support type level mappings on our very own @RepositoryRestController but the workaround for the 2.5.x branch is to move to path definitions on the individual method level. Reuse of common prefixes can be achieved through defining constants for the paths in use.

odrotbohm avatar Feb 07 '22 15:02 odrotbohm

We unfortunately cannot move back to the original arrangement as those declarations, in combination with how Spring MVC handles controllers in the 5.x generation, is effectively the reason for a CVE.

I'll review what it'd mean to support type level mappings on our very own @RepositoryRestController but the workaround for the 2.5.x branch is to move to path definitions on the individual method level. Reuse of common prefixes can be achieved through defining constants for the paths in use.

We're absolutely fine with not going back to the old solution, but we'd like to avoid having to just duplicating constant values from our abstract controller classes onto a load of methods (also increasing the risk of future additions ignoring or not adhering to the current base paths). That really feels like moving backwards.

We'd thus also gladly move to a new approach according to MR #2088. If we can help getting this forward in any way let us know.

sambernet avatar Apr 26 '22 14:04 sambernet

The fix in PR #2088 would work for us as we are running into this issue as well, and we have a tone of Abstract Controllers with a lot of methods. Duplicating all of the @RequestMappings and overriding all of the methods would be hundreds of developer hours to complete, and this seems like a simple solution.

zampettim avatar Jun 15 '22 12:06 zampettim

Should fixed from Spring data version 4.0.0-M5

yyvess avatar Aug 04 '22 12:08 yyvess

I see, the solution is actually quite nice going forward @odrotbohm 👍

The only issue we have is that now we are stuck on Spring-Boot 2.5.5 - unless we:

  • migrate all regular controllers with class-level @RequestMapping to use static base paths on all controller methods instead
  • refactor all our abstract controllers (with their implementations) to some sort of template pattern, so the abstract controllers don't define the mappings anymore, but just provide the common logic in some protected methods and maybe some constants for the common paths to be used in concrete child classes - and then duplicate all the actual controller methods & annotations on all those concrete child classes
  • this is just so we can upgrade to latest 2.7 branch - which is a pre-condition (or at least highly recommended) for the upgrade to 3.0
  • upgrade to 3.0 will in turn offer the solution we need to avoid migrating all our abstract controllers (from #2157), and allow us to migrate everything back 🤔

Which means we're kind of deadlocked or at least running in circles 😁 Don't get me wrong - this can be dealt with, and I understand why it was necessary. It just still leaves me a bit clueless about how to proceed 😕

Would it maybe be possible to backport that change to the 2.7 branch?

sambernet avatar Sep 19 '22 22:09 sambernet

It turns out this was already released with 3.7.2 and I just forgot to close the ticket, i.e. this should already be available in the most recent Spring Boot 2.7.

odrotbohm avatar Sep 20 '22 11:09 odrotbohm