5

All requests and responses handled by our Spring Rest Controller has a Common section which has certain values:

{
    "common": {
        "requestId": "foo-bar-123",
        "otherKey1": "value1",
        "otherKey2": "value2",
        "otherKey3": "value3"
    },
    ...
}

Currently all my controller functions are reading the common and copying it into the response manually. I would like to move it into an interceptor of some sort.

I tried to do this using ControllerAdvice and ThreadLocal:

@ControllerAdvice
public class RequestResponseAdvice extends RequestBodyAdviceAdapter
    implements ResponseBodyAdvice<MyGenericPojo> {

  private ThreadLocal<Common> commonThreadLocal = new ThreadLocal<>();

  /* Request */

  @Override
  public boolean supports(
      MethodParameter methodParameter, Type type, Class<? extends HttpMessageConverter<?>> aClass) {
    return MyGenericPojo.class.isAssignableFrom(methodParameter.getParameterType());
  }

  @Override
  public Object afterBodyRead(
      Object body,
      HttpInputMessage inputMessage,
      MethodParameter parameter,
      Type targetType,
      Class<? extends HttpMessageConverter<?>> converterType) {
    var common = (MyGenericPojo)body.getCommon();
    if (common.getRequestId() == null) {
       common.setRequestId(generateNewRequestId()); 
    }
    commonThreadLocal(common);
    return body;
  }

  /* Response */

  @Override
  public boolean supports(
      MethodParameter returnType, Class<? extends HttpMessageConverter<?>> converterType) {
    return MyGenericPojo.class.isAssignableFrom(returnType.getParameterType());
  }

  @Override
  public MyGenericPojo beforeBodyWrite(
      MyGenericPojo body,
      MethodParameter returnType,
      MediaType selectedContentType,
      Class<? extends HttpMessageConverter<?>> selectedConverterType,
      ServerHttpRequest request,
      ServerHttpResponse response) {
    body.setCommon(commonThreadLocal.get());
    commonThreadLocal.remove();
    return body;
  }
}

This works when I test sending one request at a time. But, is it guaranteed that afterBodyRead and beforeBodyWrite is called in the same thread, when multiple requests are coming?

If not, or even otherwise, what is the best way of doing this?

daltonfury42
  • 3,103
  • 2
  • 30
  • 47
  • I dont see a need for `ThreadLocal`. You should be able to read the `metadata` inside `beforeBodyWrite` method. This can be read through `ServerHttpRequest request`. Have you tried it? – reflexdemon Mar 10 '20 at 09:57

4 Answers4

4

I think that there is no need of your own ThreadLocal you can use request attributes.

@Override
public Object afterBodyRead(
        Object body,
        HttpInputMessage inputMessage,
        MethodParameter parameter,
        Type targetType,
        Class<? extends HttpMessageConverter<?>> converterType) {

    var common = ((MyGenericPojo) body).getCommon();
    if (common.getRequestId() == null) {
        common.setRequestId(generateNewRequestId());
    }

    Optional.ofNullable((ServletRequestAttributes) RequestContextHolder.getRequestAttributes())
            .map(ServletRequestAttributes::getRequest)
            .ifPresent(request -> {request.setAttribute(Common.class.getName(), common);});

    return body;
}


@Override
public MyGenericPojo beforeBodyWrite(
        MyGenericPojo body,
        MethodParameter returnType,
        MediaType selectedContentType,
        Class<? extends HttpMessageConverter<?>> selectedConverterType,
        ServerHttpRequest request,
        ServerHttpResponse response) {

    Optional.ofNullable(RequestContextHolder.getRequestAttributes())
            .map(rc -> rc.getAttribute(Common.class.getName(), RequestAttributes.SCOPE_REQUEST))
            .ifPresent(o -> {
                Common common = (Common) o;
                body.setCommon(common);
            });

    return body;
}

EDIT

Optionals can be replaced with

RequestContextHolder.getRequestAttributes().setAttribute(Common.class.getName(),common,RequestAttributes.SCOPE_REQUEST);

RequestContextHolder.getRequestAttributes().getAttribute(Common.class.getName(),RequestAttributes.SCOPE_REQUEST);

EDIT 2

About thread safety

1) standard servlet-based Spring web application we have thread-per-request scenario. Request is processed by one of the worker threads through all the filters and routines. The processing chain will be executed by the very same thread from start to end . So afterBodyRead and beforeBodyWrite guaranteed to be executed by the very same thread for a given request.

2) Your RequestResponseAdvice by itself is stateless. We used RequestContextHolder.getRequestAttributes() which is ThreadLocal and declared as

private static final ThreadLocal<RequestAttributes> requestAttributesHolder =
        new NamedThreadLocal<>("Request attributes");

And ThreadLocal javadoc states:

his class provides thread-local variables. These variables differ from their normal counterparts in that each thread that accesses one (via its get or set method) has its own, independently initialized copy of the variable.

So I don't see any thread-safety issues into this sulotion.

Nonika
  • 2,490
  • 13
  • 15
  • Awesome! I just want to understand, why the `Optional` and the cast to `ServletRequestAttributes`? I can set a value in the RequestContextHolder by `RequestContextHolder.getRequestAttributes().setAttribute("key", "val", RequestAttributes.SCOPE_REQUEST);` and retrieve it by `RequestContextHolder.getRequestAttributes().getAttribute("key", RequestAttributes.SCOPE_REQUEST);` – daltonfury42 Mar 10 '20 at 13:06
  • Idea was compaining about getRequestAttributes() as its marked with @Nullable but in this case I don't think there can be a way that direct access to this method will result with NPE. so you can use it without null checks. – Nonika Mar 10 '20 at 14:26
  • exactly what I had commented earlier. You don't need `ThreadLocal` – reflexdemon Mar 10 '20 at 14:43
  • @daltonfury42 can you review my second answer. – Nonika Mar 10 '20 at 15:18
  • 1
    You don't need to handle `TreadLocal` yourself because [RequestContextHolder.getRequestAttributes()](https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/context/request/RequestContextHolder.java) uses `ThreadLocal` internally. So, in fact solution is still based on a `ThreadLocal`. – Eugene Khyst Mar 11 '20 at 08:44
  • @Nonika If you can add some explanation as to how this solution will work in a multi request, multi threaded situation, I'll be happy to award you the bounty. :) I don't want to use aspectJ right now, I like the RequestContextHolder solution more. – daltonfury42 Mar 13 '20 at 06:54
  • Thanks! I ended up splitting the advice into two, a RequestAdvice and a ResponseAdvice. That should be fine too, right? – daltonfury42 Mar 13 '20 at 10:28
  • Yes this should be ok – Nonika Mar 13 '20 at 10:30
1

Quick answer: RequestBodyAdvice and ResponseBodyAdvice are invoked within the same thread for one request.

You can debug the implementation at: ServletInvocableHandlerMethod#invokeAndHandle

The way you're doing it is not safe though:

  • ThreadLocal should be defined as static final, otherwise it's similar to any other class property
  • Exception thrown in body will skip invocation of ResponseBodyAdvice (hence the threadlocal data is not removed)

"More safe way": Make the request body supports any class (not just MyGenericPojo), in the afterBodyRead method:

  • First call ThreadLocal#remove
  • Check if type is MyGenericPojo then set the common data to threadlocal
Mạnh Quyết Nguyễn
  • 17,677
  • 1
  • 23
  • 51
0

Also I have already answered this thread, but I prefer another way to solve such kind of problems

I would use Aspect-s in this scenario.

I have written included this in one file but you should create proper separate classes.

@Aspect
@Component
public class CommonEnricher {

    // annotation to mark methods that should be intercepted
    @Retention(RetentionPolicy.RUNTIME)
    @Target(ElementType.METHOD)
    public @interface EnrichWithCommon {
    }


    @Configuration
    @EnableAspectJAutoProxy
    public static class CommonEnricherConfig {}

    // Around query to select methods annotiated with @EnrichWithCommon
    @Around("@annotation(com.example.CommonEnricher.EnrichWithCommon)")
    public Object enrich(ProceedingJoinPoint joinPoint) throws Throwable {
        MyGenericPojo myGenericPojo = (MyGenericPojo) joinPoint.getArgs()[0];

        var common = myGenericPojo.getCommon();
        if (common.getRequestId() == null) {
            common.setRequestId(UUID.randomUUID().toString());
        }

        //actual rest controller  method invocation
        MyGenericPojo res = (MyGenericPojo) joinPoint.proceed();

        //adding common to body
        res.setCommon(common);
        return res;
    }

    //example controller
    @RestController
    @RequestMapping("/")
    public static class MyRestController {

        @PostMapping("/test" )
        @EnrichWithCommon // mark method to intercept
        public MyGenericPojo test(@RequestBody  MyGenericPojo myGenericPojo) {
            return myGenericPojo;
        }
    }
}

We have here an annotation @EnrichWithCommon which marks endpoints where enrichment should happen.

kadir
  • 589
  • 1
  • 7
  • 29
Nonika
  • 2,490
  • 13
  • 15
-1

If it's only a meta data that you copy from the request to the response, you can do one of the followings:

1- store the meta in the request/response header,and just use filters to do the copy :

@WebFilter(filterName="MetaDatatFilter", urlPatterns ={"/*"})
public class MyFilter implements Filter{
    @Override
    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
        throws IOException, ServletException {

    HttpServletRequest httpServletRequest = (HttpServletRequest) request;
    HttpServletResponse httpServletResponse = (HttpServletResponse) response;        
    httpServletResponse.setHeader("metaData", httpServletRequest.getHeader("metaData"));        
}

}

2- move the work into the service layer where you can do the cope through a reusable common method, or have it run through AOP

public void copyMetaData(whatEverType request,whatEverType response) {
    response.setMeta(request.getMeta);

}
Suleiman Alrosan
  • 337
  • 2
  • 4
  • 14
  • The data is not complex to fit in the headers. It has to go into the body. Since the name "metadata" is a bit confusion, I have edited to make it "common". – daltonfury42 Mar 10 '20 at 02:31