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

Allow PayloadArgumentResolver to only apply to @Payload annotated parameters [SPR-14937] #19504

Closed
spring-projects-issues opened this issue Nov 23, 2016 · 4 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Nov 23, 2016

Jens Zastrow opened SPR-14937 and commented

PayloadArgumentResolver should only be activated when @Payload annotation actually added to a method parameter.

Example:

@SqsListener(...)
  public void handleMessage(@Header("SentTimestamp") Long t, @Payload Message m)

In our case we configured a custom PayloadArgumentResolver to a QueueMessageHandlerFactory with results it beeing added as first argument resolver.
Since it always tries to resolve it breaks any other default resolver e.g. HeaderArgumentResolver

As workaround all default resolvers must also be added before the custom PayloadArgumentResolver.

More or less copy/paste from org.springframework.cloud.aws.messaging.listener.QueueMessageHandler.initArgumentResolvers()

factory.setArgumentResolvers(
      asList(
        new HeaderMethodArgumentResolver(null, null),
        new HeadersMethodArgumentResolver(),
        new NotificationSubjectArgumentResolver(),
        new AcknowledgmentHandlerMethodArgumentResolver("Acknowledgment"),
        new NotificationMessageArgumentResolver(compositeMessageConverter),
        new PayloadArgumentResolver(compositeMessageConverter)
        )
    );

Issue Links:

  • #19217 Add RedirectAttributesMethodArgumentResolver to the default argument resolvers in ExceptionHandlerExceptionResolver
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2016

Juergen Hoeller commented

I'm afraid this is by design: PayloadArgumentResolver is meant to treat unresolved non-annotated parameters as a payload type. Its javadoc even says:

This HandlerMethodArgumentResolver should be ordered last as it supports all types and does not require the Payload annotation.

So I guess you'd like to have a PayloadArgumentResolver variant which only applies to actual @Payload annotated parameters for custom scenarios? You could override supportsParameter yourself and narrow it that day... but I guess we could also provide such a subclass out of the box.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2016

Jens Zastrow commented

The most common use case for us is to configure a Jackson2 ObjectMapper to be used.
Maybe allow to override/modify the messageConverters used by the default PayloadArgumentResolver, to prevent to redefine everything from scratch.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 23, 2016

Rossen Stoyanchev commented

In the Spring Framework for STOMP messaging we add custom resolvers after built-in ones but before the payload resolver so that should work as expected. In Spring Cloud AWS the custom resolvers are always first. I would argue this is something Spring Cloud AWS can improve on.

I suppose we could also make an improvement on our side to add a boolean flag whether to require the annotation or not. We do that on the Spring MVC side (for example) for @RequestParam and @ModelAttribute arguments for which we try with annotations first and then after custom resolves without. Given that flag you can at least create a custom PayloadArgumentResolver that requires the annotation.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 28, 2016

Rossen Stoyanchev commented

The PayloadArgumentResolver now has an additional flag useDefaultResolution.

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

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.