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

@RepsitoryRestController adds the repository twice (under root and basePath) [DATAREST-972] #1342

Closed
spring-projects-issues opened this issue Jan 6, 2017 · 11 comments
Assignees
Labels

Comments

@spring-projects-issues
Copy link

Robert Rackl opened DATAREST-972 and commented

GIVEN a spring-data-rest app, that exposes some repository as HATEOAS REST endpoints.

WHEN you now want to override the behaviour of a response handler, this is described in the spring-data doc:
http://docs.spring.io/spring-data/rest/docs/current/reference/html/#customizing-sdr.overriding-sdr-response-handlers

BUT this does not work. Here are two related posts that describe the inconsistency:

The simplest test I could find was: When you create a controller with the @BasePathAwareController annotation, then

  • you MUST ONLY set the @RequestMapping at the class level ans MUST NOT set it at method / type level (which is not documented yet) for it to work at all
  • but then still the controller is added twice: at root level and with basePath prefix.

Here is my code:

@RepositoryRestController    
@RequestMapping("postBallotJson")
public class BallotRestController {
  @Autowired
  BallotRepo ballotRepo;
 
  @RequestMapping(method = POST)  
  public @ResponseBody PersistentEntityResource postBallot(@RequestBody Resource<BallotModel> newBallotRes,
                            PersistentEntityResourceAssembler resourceAssembler)
  {
    log.trace("=> POST /ballot "+newBallotRes);
    BallotModel newBallot = newBallotRes.getContent();
     [... do some additinal customization with  newBallot .. ]
     BallotModel createdBallot = ballotRepo.save(newBallot);
     return resourceAssembler.toResource(createdBallot);
  }

When I start the app I see in the log:

Mapped "{[/postBallotJson],methods=[POST]}" onto public org.springframework.http.ResponseEntity org.doogie.liquido.rest.BallotRestController.postBallot(org.doogie.liquido.model.BallotModel,org.springframework.data.rest.webmvc.PersistentEntityResourceAssembler)

and also further down  (/liquido/v2/   is my basePath)

Mapped "{[/liquido/v2/postBallotJson],methods=[POST]}" onto public org.springframework.http.ResponseEntity org.doogie.liquido.rest.BallotRestController.postBallot(org.doogie.liquido.model.BallotModel,org.springframework.data.rest.webmvc.PersistentEntityResourceAssembler)

And only the resource in the root path /postBallotJson works.

Expected Behaviour

The resource with the base path prefix should be the only one and work. /liquido/v2/postBallotJson


Affects: 2.5.4 (Hopper SR4)

Reference URL: https://github.com/mike-boddin/spring-data-rest-example

6 votes, 10 watchers

@spring-projects-issues
Copy link
Author

Robert Rackl commented

I found out one more thing. RepositoryRestController extends BasePathAwareController. If I use the @BasePathAwareController anotation it works as expected.

@spring-projects-issues
Copy link
Author

Greg Turnquist commented

@Oliver Gierke

I'm not sure why we need both @RepositoryRestResource AND @BasePathAwareController. They appear to have the exact same annotations. One simply extends the other.

I DID notice that @RepositoryRestResource failed to work with a test app I create today to examine spring-projects/spring-hateoas#434. However, the custom endpoint worked just fine using @BasePathAwareController.

However, when forming a link using Spring HATEOAS's ControllerLinkBuilder, Spring HATEOAS doesn't have insight into the BasePathAwareHandlerMapping, and it hence unable to create a proper link.

I don't know if we need to morph this issue into plans to deprecate/remove @RepositoryRestController while letting Spring HATEOAS's #434 be the place to code support for BasePathAwareHandlingMapping

@spring-projects-issues
Copy link
Author

Greg Turnquist commented

I may have uncovered where the thing is registered twice:

	@Bean
	public DelegatingHandlerMapping restHandlerMapping() {

		Map<String, CorsConfiguration> corsConfigurations = config().getCorsRegistry().getCorsConfigurations();

		RepositoryRestHandlerMapping repositoryMapping = new RepositoryRestHandlerMapping(resourceMappings(), config(),
				repositories());
		repositoryMapping.setJpaHelper(jpaHelper());
		repositoryMapping.setApplicationContext(applicationContext);
		repositoryMapping.setCorsConfigurations(corsConfigurations);
		repositoryMapping.afterPropertiesSet();

		BasePathAwareHandlerMapping basePathMapping = new BasePathAwareHandlerMapping(config());
		basePathMapping.setApplicationContext(applicationContext);
		basePathMapping.setCorsConfigurations(corsConfigurations);
		basePathMapping.afterPropertiesSet();

		List<HandlerMapping> mappings = new ArrayList<HandlerMapping>();
		mappings.add(basePathMapping);
		mappings.add(repositoryMapping);

		return new DelegatingHandlerMapping(mappings);
	}
}

RespositoryRestHandlerMapping extends BasePathAwareHandlerMapping, so it's like there are two BasePathAwareHandlerMapping's being created, and getting registered.

My gut says this wasn't the intended goal

@spring-projects-issues
Copy link
Author

Istvan Ratkai commented

nope.

The controller is mapped twice because the RequestMappingHandlerMapping also maps it.
It maps all beans which are annotated by @Controller OR @RequestMapping at the class level.

Then the RepositoryRestHandlerMapping also maps it, because it maps all beans which are annotated by @RepositoryRestController

Try removing the class level @RequestMapping annotation and it will be mapped only once. (move the main path segments to method level annotations)

However...

if the controller also explicitly annotated by @BasePathAwareController, then it could be mapped 3rd time !

BasePathAwareHandlerMapping maps controller which are directly annotated by this annotation. (So if you annotate the controller ONLY with @RepositoryRestController - what is meta-annotated with @BasePathAwareController - then BasePathAwareHandlerMapping won't find it.)

You won't get mapping conflict exception, because 1st and 2nd mappings have different path, and 2nd and 3rd mappings have different MediaType (because @RepositoryRestController automatically adds produces=[application/hal+json || application/json]}


So...

You can easily avoid using @RepositoryRestController and @BasePathAwareController together. Actually you do NOT need @BasePathAwareController if you use the other one.

However removing the @RequestMapping from the class level is a bit inconvenient, but it seems that's the only workaround.

I assume that annotation is tested by the standard RequestMappingHandlerMapping because of convenient or historical reasons (You don't have to use @Controller if you use the @RequestMapping), so I hardly think that will change.
It would break some existing webmvc applications where the @Controller annotation is not used in the controller classes.
... also that filter cannot be refined, because it's in the mvc package and it doesn't import the data.rest package so they cannot exclude @RepositoryRestController in the filter...

But maybe a Spring-Guru can find a better solution...

@spring-projects-issues
Copy link
Author

Istvan Ratkai commented

Ouch...

my workaround in the previous won't work if you want to use ControllerLinkBuilder (what is broken currently), because it gets the path from the class level @RequestMapping annotation.
So even if that bug is fixed, we still get this other issue with the double-mapped controllers...

@spring-projects-issues
Copy link
Author

Martin Veverka commented

What about extending the RequestMaping Annotation:

@RequestMapping(path="postBallotJson", doNotExposeWithoutControllerAnnotation=true)

It is compatible with existing applications

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

The fact that @RequestMapping on the class level binds the controller to the standard MVC handler mapping is something we don't control and changing it in Spring MVC is a breaking change, too. We also don't control the annotation so that we can't actually do anything about that, I think

@spring-projects-issues
Copy link
Author

Martin Veverka commented

No, it will not be breaking change, if it is explicitly declared by new attribute of the RequestMapping annotation. The default behavior do not change, but I will be able to say in particular usage of the RequestMapping, that I do not expose this mapping in Spring MVC

@spring-projects-issues
Copy link
Author

Oliver Drotbohm commented

I guess you'd have to bring that up with the Spring MVC team then but I guess you'll get puss back as declaring an attribute in a Spring MVC annotation that MVc is supposed to disregard the annotation feels a bit weird

@spring-projects-issues
Copy link
Author

Doogiemuc commented

My current workaround:

@RepositoryRestController
@RequestMapping("${spring.data.rest.base-path}")

odrotbohm added a commit that referenced this issue Oct 6, 2021
…stMapping.

When we detected @BasePathAwareController and @RepositoryRestController instances, we now reject types that use @RequestMapping on the class level as doing so causes an inevitable registration of the controller with Spring MVC.

Fixes #1342, #1628, #1686, #1946.
odrotbohm added a commit that referenced this issue Oct 6, 2021
…stMapping.

When we detected @BasePathAwareController and @RepositoryRestController instances, we now reject types that use @RequestMapping on the class level as doing so causes an inevitable registration of the controller with Spring MVC.

Fixes #1342, #1628, #1686, #1946.
@odrotbohm
Copy link
Member

This should be resolved now. As of the next release we're going to reject type-level @RequestMapping annotations right away. The recommended workaround is merging the type-level settings into the method mappings for now. Spring 6 will switch to not considering type-level @RequestMappings as controllers which might give us the opportunity to allow that kind of usage again.

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

No branches or pull requests

2 participants