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

Make FormHttpMessageWriter easier to extend [SPR-16855] #21395

Closed
spring-issuemaster opened this issue May 22, 2018 · 8 comments
Closed

Make FormHttpMessageWriter easier to extend [SPR-16855] #21395

spring-issuemaster opened this issue May 22, 2018 · 8 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented May 22, 2018

GanMing opened SPR-16855 and commented

I intend to use FormHttpMessageWriter on WebClient, mainly because I think the core function of FormHttpMessageWriter is serializeForm, if you declare it as private, you completely lose the possibility of inheritance. For example, if I need a function similar to the FormHttpMessageWriter, just want to add a verification signature after the Writer, then I need to completely copy the entire class, and then simply modify a line or two in the inside, these practices need to endure the exact same Code, IDEA duplicate code warning. In fact, if serializeForm is turned into protected, it will not expand the scope too much, and at the same time, it will simply avoid copying the content and make the code look more beautiful.

 

for example:

	class SignFormHttpMessageWriter extends FormHttpMessageWriter {
		private final Function<StringBuilder,StringBuilder> signer;

		public SignFormHttpMessageWriter(Function<StringBuilder, StringBuilder> signer) {
			this.signer = signer;
		}

		@Override
		protected StringBuilder serializeForm(MultiValueMap<String, String> form, Charset charset) {
			return signer.apply(super.serializeForm(form, charset));
		}
	}


		FormHttpMessageWriter writer = new SignFormHttpMessageWriter(sb->{
			String sign = sha256(sb.toString());
			return sb.append("&sign=").append(sign);
		});
		WebClient client = WebClient.builder().build();
		MultiValueMap<String, String> data = new LinkedMultiValueMap<>();
		//fill the data ...
		client.post().body((o, c) ->
			writer.write(Mono.just(data),
					null,
					MediaType.APPLICATION_FORM_URLENCODED,
					o,c.hints())
		);

Affects: 5.0.6

Referenced from: pull request #1833, and commits 0b61c74

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 22, 2018

Rossen Stoyanchev commented

I suppose a protected method that lets you modify the MultiValueMap before it is serialized would be good enough? In other words you're not trying to change how it's serialized, but only to add an extra parameter. Can you confirm?

Also we could update ClientCodecConfigurer to allow replacing the FormHttpMessageWriter registered by default. So you could then:

ExchangeStrategies strategies = ExchangeStrategies.builder()
        .codecs(configurer -> {
            // register custom form writer here...
        })
        .build();

WebClient webClient = WebClient.builder()
        .exchangeStrategies(strategies)
        .build(); 
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 22, 2018

GanMing commented

In fact, it is not feasible to simply modify the MultiValueMap. Usually this type of requirement is because the value of this extra parameter depends on the result of the serialization. If you don't do this, we need to predict the serialization result in advance when changing the MultiValueMap value, which will make the code very unreliable.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 22, 2018

GanMing commented

And if you have already built () a WebClient instance, then using mutate() at this time will increase the cost of creating a new instance, and in most cases, some of the implemented components,, webClient is a singleton. So I think it's more appropriate to use higher-order functions to handle these situations when executing.

For example, this WebClient is used for access of various third-party services. The earliest design of the component did not adapt an encoder or decoder for each third-party service. Most third-party services use the default encoders and decoders to work well, but there are always some third-party services that cannot be satisfied. A better approach is to specify different BodyInserters for these services.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 23, 2018

Rossen Stoyanchev commented

XYUU, okay it makes sense that you want to sign the serialized content.

Rob Winch I'd be curious to hear if there are any similar requirements in Spring Security? Specifically I am wondering about the option to use a filter, but at the moment, unless I'm missing something, I am not seeing an easy option for a client-side filter to rewrite request content. It would have to decorate the BodyInserter and ReactiveHttpOutputMessage, it seems like there should be an easier option.

 

 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 23, 2018

Rob Winch commented

There could be the need for something like this in the future to implement something like AWS Signatures. If we did something like that we would need the ability to create a canonical request from the HTTP Method, URI, Query String, Headers, and body.

For this use case would it be possible to delegate to FormHttpMessageWriter and write to a ReactiveHttpOutputMessage that produces a String that could then be mapped to a signed String that is written to the original ReactiveHttpOutputMessage?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 23, 2018

Rossen Stoyanchev commented

That's a slightly simpler variation, but still requires an implementation of ReactiveHttpOutputMessage and then some more boilerplate.

Logically what's needed is a way to provide a function that accepts Flux<DataBuffer>, operates on it, and returns the resulting Flux. This could be exposed on ClientRequest.Builder for example, which in a similar vein accepts a Consumer for modifying the underlying headers.

Arjen Poutsma, what do you think?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented May 24, 2018

Arjen Poutsma commented

I might be misunderstanding the issue, but isn't this something that can be accomplished using a ExchangeFilterFuntion? Modifying the header and body contents sounds like a good fit.

We could expose a new filter in the utility class ExchangeFilterFunctions that operates on a given Function<Flux<DataBuffer>, Flux<DataBuffer>> to change the body and/or Function<HttpHeaders, HttpHeaders> to change the headers.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 13, 2018

Rossen Stoyanchev commented

For now I've made the serializeForm method in FormHttpMessageWriter protected, as requested, and added a similar protected method in FormHttpMessageConverter.

Since I also had a closer look at a filter for re-writing the body, I'll leave some notes here. I think such a filter could fit in ExchangeFilterFunction, e.g. ofRequestBodyProcessor next to the existing ofRequestProcessor. As for the actual function type, it makes little sense to re-write individual chunks, so such a re-wrtie filter would be most useful if it aggregates via DataBufferUtils#join first, and then passes the resulting DataBuffer into the processor. In turn this the Content-Length might need to be updated, when present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.