Fix Spring encoding of + character #16732
Conversation
|
Tagging according to the technical committee list. @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08) |
blaghed
force-pushed
the
feature/uri_encoding
branch
from
f55b005
to
c6ca183
Compare
There was a problem hiding this comment.
Hi @blaghed, thank you for the PR!
I've added a few small comments below.
In addition, can you please show what problem your PR fixes? So someone can add a test case for that later. Or you could even add a test case yourself if you want. :)
PS: Ok, I noticed now that the problem is described in #16733. However, I can't reproduce it. Using the files from this gist and the latest commit on the master branch of OpenAPI Generator, I get output like this (note the encoded "+" sign: "url": "https://httpbin.org/anything?myParam=2023-10-07T17%3A34%3A02.6676887%2B02%3A00").
| @@ -682,31 +659,19 @@ public class ApiClient{{#jsr310}} extends JavaTimeFormatter{{/jsr310}} { | |||
| * @param returnType The return type into which to deserialize the response | |||
| * @return ResponseEntity<T> The response of the chosen type | |||
| */ | |||
| public <T> ResponseEntity<T> invokeAPI(String path, HttpMethod method, Map<String, Object> pathParams, MultiValueMap<String, String> queryParams, Object body, HttpHeaders headerParams, MultiValueMap<String, String> cookieParams, MultiValueMap<String, Object> formParams, List<MediaType> accept, MediaType contentType, String[] authNames, ParameterizedTypeReference<T> returnType) throws RestClientException { | |||
There was a problem hiding this comment.
This method can still throw RestClientException - see the last line of this method. Although it's not necessary to declare this, because it's derived from RuntimeException, it might still be a useful info.
So I'd recommend to restore the "throws RestClientException", although it's not really important.
| public String generateQueryUri(MultiValueMap<String, String> queryParams, Map<String, Object> uriParams) { | ||
| StringBuilder queryBuilder = new StringBuilder(); | ||
| public MultiValueMap<String, String> processQueryParams(MultiValueMap<String, String> queryParams, Map<String, Object> uriParams) { | ||
| final MultiValueMap<String, String> result = new LinkedMultiValueMap<>(); | ||
| queryParams.forEach((name, values) -> { |
There was a problem hiding this comment.
It seems we need to protect against the case that queryParams == null.
There was a problem hiding this comment.
PS: This also applies to the other changed file.
| public String expandPath(String pathTemplate, Map<String, Object> variables) { | ||
| return restTemplate.getUriTemplateHandler().expand(pathTemplate, variables).toString(); | ||
| } | ||
|
|
||
| /** | ||
| * Include queryParams in uriParams taking into account the paramName | ||
| * | ||
| * @param queryParams The query parameters | ||
| * @param uriParams The path parameters | ||
| * return templatized query string | ||
| */ |
There was a problem hiding this comment.
Please update this JavaDoc comment like you did in the other file.
|
Hi @martin-mfg , First off, thank you very much for having a look. I'll be sure to address your code comments shortly. As for this:
I decided to create a totally blank project and have only exactly the code you put in gist, and it behaved exactly as you said. So, manually debugging, I found this as the reason why I could see totally different results: public ApiClient() {
this.restTemplate = buildRestTemplate();
init();
}
@Autowired
public ApiClient(RestTemplate restTemplate) {
this.restTemplate = restTemplate;
init();
}
protected RestTemplate buildRestTemplate() {
RestTemplate restTemplate = new RestTemplate();
// This allows us to read the response more than once - Necessary for debugging.
restTemplate.setRequestFactory(new BufferingClientHttpRequestFactory(restTemplate.getRequestFactory()));
// disable default URL encoding
DefaultUriBuilderFactory uriBuilderFactory = new DefaultUriBuilderFactory();
uriBuilderFactory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.VALUES_ONLY);
restTemplate.setUriTemplateHandler(uriBuilderFactory);
return restTemplate;
}The important line above is: uriBuilderFactory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.VALUES_ONLY);So, if this is set on the In fact, you can also easily reproduce it simply by adding in an empty public static void main(String[] args) {
DefaultApi apiInstance = new DefaultApi(new ApiClient(new RestTemplate()));
String result = apiInstance.anythingGet(OffsetDateTime.now());
System.out.println(result);
}It could be that perhaps the expectation from your side is that we always set the |
I would love to add a test! |
The question how the implementation should behave by default I'd leave to @wing328.
Now that I have a better understanding of the problem that's adressed in this PR, I no longer think a test is necessary. At least it's not a common practice so far in this repo to have extensive unit tests for the generated code. |
|
@martin-mfg , so... anything else I can do to assist in merging these changes in? |
|
I don't have the rights to merge PRs. Please ask the project maintainer @wing328 to merge. (And to decide what the default behaviour should be.) Outside of GitHub you can also try to reach him on Slack. |
|
Hi @blaghed @martin-mfg @wing328 I have tested these changes and API compatibility with spring-web 6 Observation API is lost (see this PR). Please do not merge this PR until it is fixed. |
|
On the other hand, an error is occurring when the query param has a special character, for example this case: $filter paths:
/v1.0/users:
description: Get users by filter
summary: Get users by filter
get:
summary: Get users by filter
description: Get users by filter
operationId: getUsers
parameters:
- name: $filter #<-- see $
in: query
required: true
description: The filter to search users
schema:
type: string
responses:
200:
description: The requested users
content:
application/json:
schema:
$ref: '#/components/schemas/UsersResponse'
components:
schemas:
UsersResponse:
description: Represents the users response
properties:
value:
type: array
minItems: 0
nullable: false
items:
$ref: '#/components/schemas/User'
User:
type: object
properties:
id:
description: The identifier of the user
type: stringI think the problem is this line in the final String encodedName = URLEncoder.encode(name, StandardCharsets.UTF_8);I think thath we should delegate the encoding form to RestTemplate, it should not be done in the ApiClient. (see URI Encoding ) |
|
Hi @jorgerod , I've just tried the example above with the changes in this PR and it generated the following method for me without error: getUsers(String $filter)Did you mean that the error happens elsewhere? As for the Observability topic you mentioned, I have no clue why this PR in particular would impact that at all. What this does is just change the encoding, and not how the request is actually made. |
|
Hi @blaghed
The issue of observability is very well explained in this issue: I have downloaded the demo of this issue and with the modifications in the ApiClient of this PR there is a regression failure in the metrics. With openapi-generator 7.1.0, uri is filled:
Regarding this issue, there are two causalities.
|
|
Hi @jorgerod , Thank you for explaining, now it is clear. RequestEntity.method(HttpMethod method, URI url)
RequestEntity.method(HttpMethod method, String uriTemplate, Object... uriVariables)
RequestEntity.method(HttpMethod method, String uriTemplate, Map<String, ?> uriVariables)It seems that these overload methods are not working equally in the Observability layer. Likewise other methods from Is this an error in how I try to use it, or a bug in Spring, though? Edit: Continuing on this topic, is the WebClient approach approach ok with you? |
|
Hi @jorgerod , Decided to test WebClient and the new RestClient, and noticed that the behaviour is exactly the same among them. With both Many thanks for looking at this. |
|
Hi @blaghed Honestly, after checking it a lot, I think there is no bug. For the case of an own resttemplate, what you have to do is to configure the uriTemplateHandler correctly. For example: @Bean
public RestTemplate restClient(RestTemplateBuilder builder) {
RestTemplate restTemplate= builder.build();
// Config uriTemplateHandler
DefaultUriBuilderFactory uriBuilderFactory = new DefaultUriBuilderFactory();
uriBuilderFactory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.VALUES_ONLY);
restTemplate.setUriTemplateHandler(uriBuilderFactory);
return restTemplate;
}
@Bean
public PetApi petApi(RestTemplate restClient, @Value("${server.port}") String port) {
ApiClient apiClient = new ApiClient(restClient);
apiClient.setBasePath("http://localhost:" + port + "/app-rest");
PetApi petApi = new PetApi(apiClient);
return petApi;
}With this configuration, the |
|
Hi @jorgerod , Yes, that would solve the problem for the OpenApi Generator use cases. So, yes, this is a bug. And yes, there are ways to work around it in order to mitigate its impact, so it is not a world-breaking bug. Note that the proposed code here is a solution I am already using via the template override mechanisms, and haven't encountered issues with it so far, including the ones mentioned by you here. Cheers |
Sure, that's why if you look at the example I put in the previous comment, I create a In addition, in the default configuration of the ApiClient, a RestTemplate is being created with the encoding VALUES_ONLY In fact, according to spring documentation:
In my opinion, what they mean is that the responsible for doing the expanded and encoding URI templates is the Anyway, as the PR is currently, we would be breaking the observability in spring and could even cause an OOM because the url's would not be templatized. The key is here (RestTemplate.java line 871): @Nullable
@SuppressWarnings("try")
protected <T> T doExecute(URI url, @Nullable String uriTemplate, @Nullable HttpMethod method, @Nullable RequestCallback requestCallback,
@Nullable ResponseExtractor<T> responseExtractor) throws RestClientException {
Assert.notNull(url, "url is required");
Assert.notNull(method, "HttpMethod is required");
ClientHttpRequest request;
try {
request = createRequest(url, method);
}
catch (IOException ex) {
ResourceAccessException exception = createResourceAccessException(url, method, ex);
throw exception;
}
ClientRequestObservationContext observationContext = new ClientRequestObservationContext(request);
// Observation in spring needs an uriTemplate
observationContext.setUriTemplate(uriTemplate);
Observation observation = ClientHttpObservationDocumentation.HTTP_CLIENT_EXCHANGES.observation(this.observationConvention,
DEFAULT_OBSERVATION_CONVENTION, () -> observationContext, this.observationRegistry).start();
//...This is why the In short, I still think that this PR is not correct. How do you see it @wing328 @cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch |
|
I'am using the FeignClient and not the WebClient/RestTemplate. There the + is handled correctly. |
|
@MelleD , thanks for the suggestion, but I am trying to fix the Client side of the call and on the query param value, not on the URI itself. The fact that this PR impacts the URI Templating was just a side-effect of trying to get it fixed. @jorgerod , fair enough, I can understand not liking the PR proposal, as long as we agree there is a bug and try to fix it. So, being constructive here, how would you feel about changing this instead, then? Suggestion: if (value != null) {
String parsedValue = value.toString();
if (restTemplate.getUriTemplateHandler() instanceof DefaultUriBuilderFactory templateHandler) {
if (templateHandler.getEncodingMode() == DefaultUriBuilderFactory.EncodingMode.URI_COMPONENT) {
parsedValue = URLEncoder.encode(parsedValue, StandardCharsets.UTF_8);
}
}
String templatizedKey = encodedName + valueItemCounter++;
uriParams.put(templatizedKey, parsedValue);
queryBuilder.append('=').append("{").append(templatizedKey).append("}");
}Alternatively, if we avoid the Otherwise, I've no clue where else to fix this properly.
You say this is incorrect, so ¯\_(ツ)_/¯ Edit: |
|
@blaghed yes i know, but maybe you should look into other clients how they solved it. And yes you could also encode the raw uri with an interceptor (what I know from the RestTemplate) like here |
|
Sorry for the delay in answering but I'm quite busy.
I think it is the right thing to do and it already exists (I add the example again). You have to remember that by default, // The user defines a resttemplate that will be used for the ApiClient with the configuration he wants, in this case VALUES_ONLY
@Bean
public RestTemplate myRestTemplate(RestTemplateBuilder builder) {
RestTemplate restTemplate= builder.build();
// Config uriTemplateHandler
DefaultUriBuilderFactory uriBuilderFactory = new DefaultUriBuilderFactory();
uriBuilderFactory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.VALUES_ONLY);
restTemplate.setUriTemplateHandler(uriBuilderFactory);
return restTemplate;
}
// Uses the resttemplate defined above and will correctly encode `+`.
@Bean
public PetApi petApi(RestTemplate myRestTemplate) {
ApiClient apiClient = new ApiClient(restClient);
PetApi petApi = new PetApi(apiClient);
return petApi;
}In this way, the encoding works perfectly and does not break Spring's observability. |
|
Hi @jorgerod ,
No worries.
It is using the default RestTemplate, with no qualifier.
It was removed at same point, then it was added again based on the "generateClientAsBean" flag, which (as far as I can tell) is enabled by default. {{#generateClientAsBean}}
@Component("{{invokerPackage}}.ApiClient")
{{/generateClientAsBean}}
public class ApiClient{{#jsr310}} extends JavaTimeFormatter{{/jsr310}} {
Like I mentioned, that is a workaround. |
|
Hi @blaghed
Maybe you could add a property to configure the encoding of the resttemplate that ApiClient uses.
By default, ApiClient does not have
I have already stated my point of view several times. RestTemplate provides the mechanisms to do the encoding correctly. There are millions of projects using RestTemplate (without ApiClient) and they work transparently, without doing the encoding... And what we can't do is to add bugs. Sorry |
|
Yes, fixing a bug by adding another is not the intent here :) But, if I understand right, your opinion here is that nothing at all should be done for this, right? |
|
Correct. In my opinion, if resttemplate is configured properly, I don't think there is any bug. I am very sorry |
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.For Windows users, please run the script in Git BASH.
master(upcoming 7.1.0 minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)