Spring RequestInfo combine not working if either info is null

Affects: 5.3.X

Motivation

I am currently trying to implement a feature, which allows to use nested RequestMapping classes to assemble the path:

@RequestMapping("/api/v1")
class ApiV1 {}

@RequestMapping("/entity")
@RestController
class EntityController extends ApiV1 {}

so that endpoints defined in EntityController get path /api/v1/entity...

If you do not like what I am trying to accomplish here, move to section "The Bug", as in my opinion this needs to get fixed either way.

My Approaches

I am trying to accomplish this by providing my own implementation of RequestMappingHandlerMapping.

Override `resolveEmbeddedValuesInPatterns`

My initial thought was overriding the method which is assembling the path: RequestMappingHandlerMapping.resolveEmbeddedValuesInPatterns(String[] patterns

However, the only argument is the String[] patterns and I do not have the handler class available (could this be changed?)

Override `getMappingForMethod`

My next attempt was overriding method getMappingForMethod(Method method, Class<?> handlerType). My implementation would first call super.getMappingForMethod to receive a RequestMappingInfo, then iterate through the handlerType's superClasses and assembles the path according to the found @RequestMapping annotations:

(Simplified, some error checking etc. is happening here as well)

protected RequestMappingInfo getMappingForMethod(Method method, Class<?> handlerType) {
    RequestMappingInfo methodMapping = super.getMappingForMethod(method, handlerType);
    List<String> superclassUrlPatterns = new ArrayList<String>();
    for (Class<?> clazz = handlerType; clazz != Object.class; clazz = clazz.getSuperclass()) {
      // assemble url from found annotations 
          superclassUrlPatterns.add(...)
    }
    RequestMappingInfo superclassRequestMappingInfo = RequestMappingInfo
          .paths(String.join("", superclassUrlPatterns))
          .build();

   return superclassRequestMappingInfo.combine(methodMapping);
   return methodMapping;
  }

As you can see, as I already have a methodMapping (which I cannot modify) from the super call, I need to assemble a new one (superclassRequestMappingInfo) and combine these two.

The Bug

Combining does not work, as I am running into an assertion error. This is caused by some if clauses which will only combine the two infos, if both are not null. In my opinion, combine should be done even if only one of both is not null. This can be safely done, as the called method is doing additional checks.

The fix

Update: I locally tried to just change the && to ||, but this will fail later, due to other being null. What is working for me now is this (not very pretty) code:

PathPatternsRequestCondition pathPatterns = null;
if(this.pathPatternsCondition != null && other.pathPatternsCondition == null){
    pathPatterns = this.pathPatternsCondition;
} else if(this.pathPatternsCondition == null && other.pathPatternsCondition != null){
    pathPatterns = other.pathPatternsCondition;
} else if(this.pathPatternsCondition != null && other.pathPatternsCondition != null){
    pathPatterns = this.pathPatternsCondition.combine(other.pathPatternsCondition);
}

PatternsRequestCondition patterns = null;
if(this.patternsCondition != null && other.patternsCondition == null){
    patterns = this.patternsCondition;
} else if(this.patternsCondition == null && other.patternsCondition != null){
    patterns = other.patternsCondition;
} else if(this.patternsCondition != null && other.patternsCondition != null){
    patterns = this.patternsCondition.combine(other.patternsCondition);
}

Notes

While I believe the mentioned if statements are a bug alone, my preferred way to accomplish my task would rather be to override resolveEmbeddedValuesInPatterns and have the handler included as a parameter. If this is not something you'd consider, I can proceed with my 2nd approach as soon as the bug got fixed.

Comment From: ericlacher

I gave up on my approach, as extending classes does not seem to be a good way to resolve nested paths. Nevertheless, I think the combine method needs a fix.

Comment From: rstoyanchev

If you do not like what I am trying to accomplish here, move to section "The Bug", as in my opinion this needs to get fixed either way.

Preferences aside, I'm not sure it's a good idea to alter semantics this way on existing annotations. It would be more obvious to use meta annotations, e.g. @ApiMapping or something similar. This would make it more clear, but I would still consider alternatives, e.g. we support a pathPrefix with a Class predicate or possibly placeholder for the API prefix in the request mapping.

As for the checks that both mappings should be PathPattern based or String pattern based, this is an illegal state check and it is their by design. A given HandlerMapping should be using one strategy or the other, not both.

Comment From: spring-projects-issues

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.