R39.0.0.beta1 with openapi fix WFLY-21302#1117
Conversation
rhusar
changed the title
|
Lets go with #1118 |
|
please update this one instead, otherwise will need to wait or manually close all pending tests |
|
once in this route, till the end :) |
…th changes in WF Signed-off-by: Radoslav Husar <radosoft@gmail.com>
rhusar
force-pushed
the
r39.0.0.Beta1-with-openapi
branch
from
0c320ef to
0abd619
Compare
|
Awesome! |
| @@ -47,9 +48,8 @@ public void testOpenAPIContextIsAvailable() throws IOException, InterruptedExcep | |||
| final HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | |||
There was a problem hiding this comment.
This is not portable. If the OpenAPI request does not define a specific encoding, via the Accept-Charset (or Accept) header, then it will use UTF-8 was specified in the request. Since the above request does include this header, the response may differ from the default charset of the test client.
Either:
- Specify an Accept-Charset or Accept header in the request (using the default charset of the test client)
- Read the body using default encoding of the OpenAPI endpoint, i.e. HttpResponse.BodyHandlers.ofString(StandardCharsets.UTF_8)
There was a problem hiding this comment.
@pferraro The "response may differ from the default charset of the test client" statement is false.
The no-argument version of HttpResponse.BodyHandlers.ofString() does NOT use "the default charset of the test client" (not to be confused with Charset.defaultCharset()).
Instead, it
- Uses the charset specified in the response's Content-Type header if present
- Falls back to UTF-8 if no charset is specified in the response
Also, this same code is used in most of the quickstarts here, so if this were to be problematic, it could affect more of the QSs.
There was a problem hiding this comment.
Ah - you are correct sir ... even if your javadoc reference is old :)
There was a problem hiding this comment.
Actually, now I'm questioning how we actually handle this stuff...
According to:
https://www.rfc-editor.org/rfc/rfc8259#section-8.1
https://yaml.org/spec/1.2.2/#52-character-encodings
... we should actually return a 406 response if the client requests anything other than unicode (specifically, UTF-8, for application/json).
I was not very familiar with these formats when I wrote this stuff, and thus treated them as text formats.
| String[] bodyLines = response.body().split("\n"); | ||
| assertTrue(bodyLines[1].startsWith("openapi:")); | ||
| assertTrue("Document does not contain \"openapi:\" field", Arrays.stream(bodyLines).anyMatch(line -> line.startsWith("openapi:"))); |
There was a problem hiding this comment.
Better, but still more fragile than necessary, as it assumes no root indentation. Why not just parse the content into a YAML model and use that for any test assertions?
There was a problem hiding this comment.
The scope of the test is to verify presence of non-dummy /openapi endopoint, i.e. to help a user quickly realize the mistake of using a server profile with absent openapi subsystem, so as such does not attempt to do anything more. Moreover, these tests are asking users to run in isolation, so there is no root indentation at play. Nevertheless, it would not hurt to demostrate this while making this less fragile in the process. We could fix this for 39 Final. 👍🏼
There was a problem hiding this comment.
Moreover, these tests are asking users to run in isolation, so there is no root indentation at play.
That is controlled by the implementation - so, still a superfluous test assumption (but also not a big deal).
There was a problem hiding this comment.
@pferraro Can you open a Jira please? Feel free to assign to me to fix this since I caused it!
3 participants
Includes #1116
https://issues.redhat.com/browse/WFLY-21302