Implement method HEAD in ApiListener #6062

Yale96 · 2023-12-29T12:04:44Z

No description provided.

jkosternl · 2024-01-08T09:15:24Z

@Yale96, can you check (or perhaps add a check) that no real content data is received if the HEAD method is used? Maybe we need to add something to prevent that (or that it is used to by pass security etc).

tnleeuw · 2024-01-08T11:10:27Z

@Yale96, can you check (or perhaps add a check) that no real content data is received if the HEAD method is used? Maybe we need to add something to prevent that (or that it is used to by pass security etc).

Indeed please add a test for this. I see nothing in code now to prevent data from being returned.

core/src/main/java/org/frankframework/http/rest/ApiListener.java

jkosternl

As discussed in daily.

jkosternl

Looking good. Just a few minor remarks to consider.

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

core/src/test/java/org/frankframework/http/rest/ApiListenerTest.java

tnleeuw

In addition to the comments on the PR also on all affected files change the copyright-year to include the year 2024! :-)

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

core/src/test/java/org/frankframework/http/rest/ApiListenerTest.java

core/src/main/java/org/frankframework/http/rest/ApiListenerServlet.java

core/src/main/java/org/frankframework/http/rest/ApiListener.java

tnleeuw · 2024-01-17T10:06:26Z

core/src/main/java/org/frankframework/http/rest/ApiListenerServlet.java

+				if (result == null && method == ApiListener.HttpMethod.HEAD) {
+					MimeType contentType = determineContentType(messageContext, listener, null);
+					response.setContentType(contentType.toString());
+				}
+


Two comments on this:

result == null is not the same as Message.isEmpty(result)

I think that since both blocks have same content you can merge the if-conditions:
if (!Message.isEmpty(result) || method == ApiListener.HttpMethod.HEAD))

tnleeuw · 2024-01-17T10:41:45Z

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

@@ -1198,8 +1225,11 @@ public void testRequestWithAccept(Methods method) throws Exception {

 		// Assert
 		assertEquals(200, result.getStatus());
-		Message input = requestMessage;
-		assertEquals("application/xml", input.getContext().get("Header.accept"));
+		if (method != Methods.HEAD) {


Why is the exception on method HEAD needed here?

That's not needed anymore indeed. I will remove it

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

core/src/main/java/org/frankframework/http/rest/ApiListener.java

tnleeuw · 2024-01-19T09:29:40Z

core/src/main/java/org/frankframework/http/rest/ApiListenerServlet.java

@@ -579,9 +579,11 @@ else if(segment.startsWith("{") && segment.endsWith("}")) {
 				 */
 				response.addHeader("Allow", (String) messageContext.get("allowedMethods"));

-				if (!Message.isEmpty(result)) {
+				if (!Message.isEmpty(result) && result.size() != -1) {


This is not the correct change because you now make setting content-type dependent on result-size being known.
What I had suggested earlier was to combine (!Message.isEmpty(result) || method == ApiListener.HttpMethod.HEAD)

Checking for size can be done in a nested if. And when setting content-length on the response, you should not need to also add a header content-length because the HttpResponse implementation should do this for you.

tnleeuw · 2024-01-19T09:30:30Z

core/src/main/java/org/frankframework/http/rest/ApiListenerServlet.java

+				} else {
+					response.setContentLength(0);
+					response.addHeader("content-length", "0");


Now you always set content-length to 0 when method = HEAD?
I thought that that goes against the intention?

tnleeuw · 2024-01-19T09:39:46Z

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

+		Response result = service(createRequest(uri, Methods.HEAD, null, headers));
+
+		// Assert
+		assertEquals(200, result.getStatus());


I do not see a check here that the content of the response is empty. That check is perhaps the most important check to validate if HEAD did what it is supposed to do.

tnleeuw · 2024-01-19T09:41:21Z

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

+		assertEquals("OPTIONS, HEAD", result.getHeader("Allow"));
+		assertNull(result.getErrorMessage());
+		assertFalse(result.containsHeader("etag"));
+		assertEquals("0", result.getHeader("content-length"));


The length of the response should not have been 0 as the response-content that would have been returned by GET would have been {"tralalalallala"}.

tnleeuw · 2024-01-19T09:45:50Z

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

+		assertNull(result.getErrorMessage());
+		assertFalse(result.containsHeader("etag"));
+		assertEquals("0", result.getHeader("content-length"));
+		assertEquals("*/*", result.getHeader("content-type"));


I'm surprised that the content-type header is */* and not JSON. Can you please set the mimetype on the repeatableMessage to JSON? Or otherwise the produces on the ApiListenerBuilder?

tnleeuw · 2024-01-19T09:47:53Z

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java

+	public void apiListenerWithHeadMethodCall() throws Exception {
+		// Arrange
+		String uri = "/apiListenerWithHeadMethodCall";
+		Message repeatableMessage = Message.asMessage(new Message("{\"tralalalallala\":true}").asByteArray());


After applying other comments to this test, I would like a copy of this test where this repeatableMessage is constructed with an empty string, but the size set to something like 20 by setting the corresponding key in the MessageContext. Then the content-length returned by HEAD should also be 20.

Co-authored-by: J. Koster <j.koster@gmx.com>

nielsm5 · 2024-01-24T12:59:03Z

core/src/main/java/org/frankframework/http/rest/ApiListenerServlet.java

+					/*
+					 * Finalize the pipeline and write the result to the response
+					 */
+					final boolean outputWritten = writeToResponseStream(response, result);


Sluiten we deze wel?

nielsm5

I've made some changes in #6148 so one of us needs to merge those :)

…442-Implement-method-head-in-ApiListener # Conflicts: # core/src/main/java/org/frankframework/http/rest/ApiListener.java

nielsm5 · 2024-01-24T14:27:46Z

core/src/main/java/org/frankframework/http/rest/ApiListener.java

@@ -240,6 +226,7 @@ public boolean accepts(@Nullable String acceptHeader) {
 	 * HTTP method to listen to
 	 * @ff.default GET
 	 */
+	@Deprecated


Suggested change

@Deprecated

Deze moet hier denk ik niet?

sonarcloud · 2024-01-24T14:53:10Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
95.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

core/src/main/java/org/frankframework/http/rest/ApiListenerServlet.java

@@ -577,15 +577,18 @@
 				/*
 				 * Add headers
 				 */
-				response.addHeader("Allow", (String) messageContext.get("allowedMethods"));
+				response.addHeader("Allow", (String) pipelineSession.get("allowedMethods"));


5442 - implement method HEAD in ApiListener

680ebd9

Yale96 added the 7.9 label Dec 29, 2023

Yale96 added this to the 7.9.1 milestone Dec 29, 2023

Yale96 requested review from tnleeuw, nielsm5 and jkosternl December 29, 2023 12:04

Yale96 self-assigned this Dec 29, 2023

jkosternl removed the 7.9 label Jan 8, 2024

jkosternl removed this from the 7.9.1 milestone Jan 8, 2024

jkosternl linked an issue Jan 8, 2024 that may be closed by this pull request

Implement http method HEAD #5442

Closed

5442 - Get empty message body with HEAD method

1bc67f6

jkosternl requested changes Jan 9, 2024

View reviewed changes

core/src/main/java/org/frankframework/http/rest/ApiListener.java Outdated Show resolved Hide resolved

5442 - added extra tests in ApiListenerServletTest.java

0ba54a5

Yale96 requested a review from jkosternl January 9, 2024 12:31

5442 - added extra tests in ApiListenerServletTest.java

be4f220

tnleeuw approved these changes Jan 9, 2024

View reviewed changes

jkosternl approved these changes Jan 10, 2024

View reviewed changes

jkosternl requested changes Jan 10, 2024

View reviewed changes

5442 - resolved comments on MR

e449c4b

Yale96 requested review from jkosternl and tnleeuw January 10, 2024 13:57

jkosternl removed the request for review from nielsm5 January 10, 2024 16:31

jkosternl approved these changes Jan 10, 2024

View reviewed changes

core/src/test/java/org/frankframework/http/rest/ApiListenerServletTest.java Outdated Show resolved Hide resolved

core/src/test/java/org/frankframework/http/rest/ApiListenerTest.java Outdated Show resolved Hide resolved

5442 - resolved comments on MR

be82d90

Yale96 requested a review from jkosternl January 11, 2024 06:13

jkosternl approved these changes Jan 11, 2024

View reviewed changes

tnleeuw requested changes Jan 11, 2024

View reviewed changes

5442 - resolved first couple of comments

512b20c

5442 - resolved first comments on MR

49e5cfd

tnleeuw requested changes Jan 17, 2024

View reviewed changes

Yale96 requested review from tnleeuw and jkosternl January 18, 2024 12:13

Yale96 added 3 commits January 18, 2024 13:14

5442 - Resolved comments on MR

2637270

5442 - removed exception for head method in tests

ae88453

5442 - removed override method ApiListener

e8a4c86

tnleeuw requested changes Jan 19, 2024

View reviewed changes

Yale96 requested a review from tnleeuw January 19, 2024 10:40

5442 - resolved comments on MR

1c2c77d

tnleeuw approved these changes Jan 19, 2024

View reviewed changes

jkosternl approved these changes Jan 22, 2024

View reviewed changes

tnleeuw requested a review from nielsm5 January 22, 2024 11:22

nielsm5 and others added 2 commits January 23, 2024 11:07

Cleanup ApiListener code

6ed105a

Apply suggestions from code review

9d54edb

Co-authored-by: J. Koster <j.koster@gmx.com>

nielsm5 reviewed Jan 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/chore/cleanupApiListener' into 5…

d129048

…442-Implement-method-head-in-ApiListener # Conflicts: # core/src/main/java/org/frankframework/http/rest/ApiListener.java

nielsm5 changed the title ~~5442 - implement method HEAD in ApiListener~~ Implement method HEAD in ApiListener Jan 24, 2024

Yale96 added 2 commits January 24, 2024 14:51

Merge branch 'master' into 5442-Implement-method-head-in-ApiListener

1cf99ce

5939 - Fix trailing spaces

f0ffaab

nielsm5 reviewed Jan 24, 2024

View reviewed changes

Minor refactor

8967f46

nielsm5 requested a review from tnleeuw January 24, 2024 14:36

github-advanced-security bot found potential problems Jan 24, 2024

View reviewed changes

nielsm5 approved these changes Jan 24, 2024

View reviewed changes

tnleeuw approved these changes Jan 24, 2024

View reviewed changes

nielsm5 merged commit 9bd72ea into master Jan 24, 2024
15 checks passed

nielsm5 deleted the 5442-Implement-method-head-in-ApiListener branch January 24, 2024 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement method HEAD in ApiListener #6062

Implement method HEAD in ApiListener #6062

Yale96 commented Dec 29, 2023

jkosternl commented Jan 8, 2024

tnleeuw commented Jan 8, 2024

jkosternl left a comment

jkosternl left a comment

tnleeuw left a comment

tnleeuw Jan 17, 2024

tnleeuw Jan 17, 2024

Yale96 Jan 19, 2024

tnleeuw Jan 19, 2024

tnleeuw Jan 19, 2024

tnleeuw Jan 19, 2024

tnleeuw Jan 19, 2024

tnleeuw Jan 19, 2024

tnleeuw Jan 19, 2024

nielsm5 Jan 24, 2024

nielsm5 left a comment

nielsm5 Jan 24, 2024 •

edited

Loading

sonarcloud bot commented Jan 24, 2024

Implement method HEAD in ApiListener #6062

Implement method HEAD in ApiListener #6062

Conversation

Yale96 commented Dec 29, 2023

jkosternl commented Jan 8, 2024

tnleeuw commented Jan 8, 2024

jkosternl left a comment

Choose a reason for hiding this comment

jkosternl left a comment

Choose a reason for hiding this comment

tnleeuw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsm5 left a comment

Choose a reason for hiding this comment

nielsm5 Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Jan 24, 2024

Quality Gate passed

nielsm5 Jan 24, 2024 •

edited

Loading