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

Update body related options in ClientRequest [SPR-15234] #19799

Closed
spring-projects-issues opened this issue Feb 8, 2017 · 4 comments
Closed

Update body related options in ClientRequest [SPR-15234] #19799

spring-projects-issues opened this issue Feb 8, 2017 · 4 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Feb 8, 2017

Rossen Stoyanchev opened SPR-15234 and commented

Currently if an ExchangeFilterFunction wants to enrich the request with a header it uses ClientRequest.from(request) which copies the HTTP method, URL, headers, and cookies from the current request. The body however must be accessed and passed explicitly to the builder. This can be seen in the built-in basicAuthentication filter.

I can see that this is required for the Builder to work which has a separate build() (i.e. no body) vs body methods returning a ClientRequest. However now that WebClient provides all the necessary convenience to perform an exchange perhaps we can update the builder so that body methods return Builder and only build() actually builds the request instance. Then the from(ClientRequest) method could also copy the body.


Affects: 5.0 M4

Referenced from: commits 45770d7

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 10, 2017

Arjen Poutsma commented

I took a stab at this, but I'm not that happy with the outcome. See poutsma@10b0b2b
Note that this branch does not even compile at the time of writing this. Here is some explanation:

If we do not make the build method the "end" of the method call chain, we cannot determine the parameterisation of the ClientRequest by whatever was passed into build (as we currently do). Instead, we need to parameterise the ClientRequest .Builder itself, but that results in some issues. For instance, having the body(S publisher, Class<T> elementClass) variant is not possible anymore. See my comments here, here, and here.

I see three options for resolving this:

  1. We continue down this path, and figure out a way to resolve the issues above.
  2. We drop the parameterisation of ClientRequest
  3. We keep things as they currently are, rename ClientRequest.from so that it becomes apparent that it does not copy the body inserter. Possible candidate: withoutBodyFrom?

Of these options, my favourite is number 3. Option 1 would require a complete ClientRequest overhaul, probably making it less similar to its server-side counterparts.
I don't think this particular minor issue warrants such an act.

As for option 2, I think that the ClientRequest parameterisation actually conveys useful information to the user: ClientRequest<Void> shows that you're sending no body, ClientRequest<String> shows that you're sending a String, etc. Granted, with the new WebClient users are not expected to use ClientRequest directly that often, but I still think that the parameterisation is important enough to keep around, for users who want to use ExchangeFunction directly.

So that leaves us with option 3. Copying an entire request is not that common of a use case (it typically only occurs in filters), especially compared to creating regular requests. So I don't mind the fact that creating a complete copy is slightly inconvenient, especially if we rename the method so that it becomes clearer that the body isn't copied.

What do you think, Rossen Stoyanchev?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 10, 2017

Rossen Stoyanchev commented

Following the WebClient refactoring, the most likely place to use a ClientRequest is a filter so it would be useful for from(request) to work intuitively. Right now the only way to understand why it skips the body is to understand how the Builder works and changing the name only makes it more clear what it does, it does not make it any more intuitive.

Do we need the generic type? As far as I can see nothing relies on it and SeverRequest is not parmaterized either.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 13, 2017

Arjen Poutsma commented

Do we need the generic type? As far as I can see nothing relies on it and SeverRequest is not parmaterized either.

The counterpart to ClientRequest on the serverside would be ServerResponse, as it is the ServerResponse that is created by the user and written to an output stream. That said, I forgot that we recently did remove the parameterisation in ServerResponse, in favour of two specific ServerResponse subtypes: RenderingResponse and EntityResponse.

With that situation, I see no objection to dropping the parameterisation on ClientRequest as well, so I am working on that.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Feb 13, 2017

Arjen Poutsma commented

Done. See 45770d7

@spring-projects-issues spring-projects-issues added type: enhancement in: web labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 5.0 M5 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants