HDDS-13258. Refactor HttpServletResponse (Part1 - HddsConfServlet) by unknowntpo · Pull Request #9126 · apache/ozone

unknowntpo · 2025-10-09T03:25:07Z

This PR:

Send error response according to specified accept format, e.g. JSON, or XML, if no format is specified, fall back to XML.
extract enum ResponseFormat for readability.

What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13258

How was this patch tested?
UT: TestHddsConfServlet

…sedKeyInfo

…ml if accept header has invalid value

… specified format

…this

unknowntpo · 2025-10-09T04:17:29Z

@peterxcli Would you like to review this PR ?

adoroszlai

Thanks @unknowntpo for the patch.

I would prefer ResponseFormat and related static methods to be added/moved outside of HddsConfServlet (and matching new test class).

adoroszlai · 2025-10-09T07:18:09Z

+  private void processCommand(String cmd, ResponseFormat format,
+                              HttpServletRequest request, HttpServletResponse response, Writer out,
+                              String name)


nit: Please do not format method signature like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters.

adoroszlai · 2025-10-09T07:19:31Z

-      Writer out) throws IOException {
+                                       Writer out) throws IOException {


Please do not format method signature like this.

adoroszlai · 2025-10-09T08:43:18Z

+    switch (format) {
+    case JSON:
      response.setContentType("application/json; charset=utf-8");
+      break;
+    case XML:
+    default:
+      response.setContentType("text/xml; charset=utf-8");
+      break;
    }


I suggest adding getContentType() instance method in ResponseFormat, returning application/json and text/xml for JSON and XML respectively. Then we can change this to:

Suggested change

switch (format) {

case JSON:

response.setContentType("application/json; charset=utf-8");

break;

case XML:

default:

response.setContentType("text/xml; charset=utf-8");

break;

}

response.setContentType(format.getContentType() + "; charset=utf-8");

unknowntpo · 2025-10-10T09:26:15Z

Thanks @unknowntpo for the patch.

I would prefer ResponseFormat and related static methods to be added/moved outside of HddsConfServlet (and matching new test class).

@adoroszlai Do you want to provide a consistent way to deal with HTTP response for all child class of HTTPServlet ?
We can make a HTTPServletUtils class placed at org.apache.hadoop.hdds.utils. What do you think ?

adoroszlai · 2025-10-13T14:16:30Z

We can make a HTTPServletUtils class placed at org.apache.hadoop.hdds.utils. What do you think ?

Sounds good.

…onse

…arseHeaders

unknowntpo · 2025-10-19T00:23:10Z

@adoroszlai Refactoring is done, would you like to review it again ?

adoroszlai

Thanks @unknowntpo for updating the patch, LGTM.

adoroszlai · 2025-10-20T12:19:50Z

+
+  private static void writeXmlError(String errorMessage, Writer out) throws IOException {
+    try {
+      DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory();


I think DocumentBuilderFactory should be stored for reuse.

Once an application has obtained a reference to a DocumentBuilderFactory it can use the factory to configure and obtain parser instances. doc

Something like:

private static final CheckedSupplier<DocumentBuilderFactory, ParserConfigurationException> DOCUMENT_BUILDER_FACTORY = MemoizedCheckedSupplier.valueOf(XMLUtils::newSecureDocumentBuilderFactory);

and

DocumentBuilder builder = DOCUMENT_BUILDER_FACTORY.get().newDocumentBuilder();

…f/HddsConfServlet.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

…ls/HttpServletUtils.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

…ls/TestHttpServletUtils.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

unknowntpo · 2025-10-20T23:33:50Z

@adoroszlai Sorry, that indentation problem happens again.
Actually, I indent my file using Intellij without additional configuration.
Is there any guideline of indentation in Apache Ozone ?

adoroszlai · 2025-10-21T05:29:05Z

Is there any guideline of indentation in Apache Ozone ?

Continuation indent: 4
turn off Align when multiline

adoroszlai · 2025-10-22T17:57:09Z

@peterxcli please take a look

peterxcli · 2025-10-23T05:18:44Z

+    case XML:
+    default:
+      writeXmlError(errorMessage, writer);
+      break;
+    }


ditto (#9126 (comment))

Suggested change

case XML:

default:

writeXmlError(errorMessage, writer);

break;

}

case XML:

writeXmlError(errorMessage, writer);

break;

default:

throw ....;

}

I throw an IOException, whose cause is IllegalArgumentException to denote that it happened during writing response to I/O, and caller passed in illegal arguments.

…xception

…tion for unspecified format

…ch cause is IllegalArgumentException

chungen0126

Thanks @unknowntpo for the patch. Left some comments.

chungen0126 · 2025-11-07T08:52:33Z

+   */
+  public static ResponseFormat getResponseFormat(HttpServletRequest request) throws IllegalArgumentException {
+    String format = request.getHeader(HttpHeaders.ACCEPT);
+    if (format == null) {


My suggestion is to use XML as the fallback when the format is null. For any format other than XML or JSON, we should set it to UNSPECIFIED.

chungen0126 · 2025-11-07T09:01:02Z

+  public enum ResponseFormat {
+    UNSPECIFIED("unspecified"),
+    JSON("json"),
+    XML("xml");


I suggest we use the MediaType constants (e.g., MediaType.APPLICATION_JSON, MediaType.APPLICATION_XML) instead of string literals like "json". This is more precise as it directly uses the values that will be set in the HTTP headers, and it also avoids 'magic strings'.

peterxcli

LGTM! Thanks very much for this improvement!

peterxcli · 2025-11-07T11:16:57Z

lets merge this after green CI!

peterxcli · 2025-11-07T15:12:37Z

Thanks @unknowntpo for this patch, @adoroszlai and @chungen0126 for reviewing!

adoroszlai · 2025-11-07T15:14:55Z

+  /**
+   * Functional interface for operations that accept a parameter and may throw exceptions.
+   *
+   * @param <T> the type of the input to the operation
+   */
+  @FunctionalInterface
+  public interface CheckedConsumer<T> {
+    void accept(T t) throws Exception;
+  }


In part 2, I think we should reuse CheckedConsumer from org.apache.ratis.util.function instead of defining new interface.

peterxcli · 2025-11-07T15:28:03Z

@chungen0126 Sorry I didnt notice that you left some comments

HDDS-13258. Refactor HttpServletResponse (Part1 - HddsConfServlet) #9126 (comment)
HDDS-13258. Refactor HttpServletResponse (Part1 - HddsConfServlet) #9126 (comment)

@unknowntpo please address them in the part 2 if you think they make sense to you, thanks!

unknowntpo added 10 commits October 6, 2025 19:26

refactor: simplify duplicate code

216f57a

minor: bypass checkstyle: visibilitymodifier

59c319d

refactor: revert SuppressWarnings

b544d5f

refactor: move creation of volBucketPair inside constructor of Proces…

05ffdc8

…sedKeyInfo

refactor: no need to throw BadFormatException since we fall back to x…

20d4bae

…ml if accept header has invalid value

refactor: add writeErrorResponse to write error response according to…

79ea893

… specified format

refactor: extract enum: ResponseFormat

5bf2702

refactor: use try with resource to close writer

0d5f9cf

fix: fix wrong switch case usage

84e7baa

Merge branch 'master' into HDDS-13258-refactor-HttpServletResponse

0ad52f7

unknowntpo changed the title ~~Hdds 13258 refactor http servlet response~~ HDDS-13258. Refactor HttpServletResponse (Part1) Oct 9, 2025

unknowntpo marked this pull request as ready for review October 9, 2025 03:34

unknowntpo marked this pull request as draft October 9, 2025 03:35

refactor: no need to close writer, because servlet container handled …

24bc3e8

…this

unknowntpo changed the title ~~HDDS-13258. Refactor HttpServletResponse (Part1)~~ HDDS-13258. Refactor HttpServletResponse (Part1 - HddsConfServlet) Oct 9, 2025

unknowntpo marked this pull request as ready for review October 9, 2025 04:17

unknowntpo closed this Oct 9, 2025

unknowntpo reopened this Oct 9, 2025

adoroszlai reviewed Oct 9, 2025

View reviewed changes

adoroszlai requested a review from peterxcli October 9, 2025 08:48

unknowntpo added 7 commits October 15, 2025 07:51

refactor: introduce HttpServletUtils

a8b885e

fix: fix testGetPropertyWithCmd

ea772b2

refactor(HttpServletUtils): pass response as params to writeErrorResp…

8c3e71c

…onse

refactor: HddsConfServlet: remove unused error class

91c115a

refactor(TestHttpServletUtils): introduce parameterized test to testP…

e0ddbb8

…arseHeaders

refactor(HttpServletUtils): add status code to writeErrorResponse

49e706b

refactor(HttpServletUtils): refactor and rename to getResponseFormat

16ad021

adoroszlai self-requested a review October 19, 2025 16:36

adoroszlai reviewed Oct 20, 2025

View reviewed changes

refactor(HttpServletUtils): extract DOCUMENT_BUILDER_FACTORY

8f357fd

adoroszlai reviewed Oct 20, 2025

View reviewed changes

unknowntpo and others added 4 commits October 21, 2025 07:31

Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/con…

dd6efd0

…f/HddsConfServlet.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/con…

092e5fa

…f/HddsConfServlet.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/uti…

5c6aa18

…ls/HttpServletUtils.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

Update hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/uti…

d65d5fc

…ls/TestHttpServletUtils.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>

peterxcli reviewed Oct 23, 2025

View reviewed changes

unknowntpo added 8 commits October 25, 2025 21:01

refactor(HddsConfServlet): explicitly throw BadFormatException

ae742f1

refactor(HddsConfServlet): throw internal server error for BadFormatE…

656fb87

…xception

refactor(HttpServletUtils): add writeResponse method

e928eee

remove VisibleForTesting

c7c8567

minor: manually tweak indentation of writeResponse

d42479c

remove unused import

d7e17f5

refactor(HttpServletUtils.writeErrorResponse): explicitly throw excep…

20a7bd3

…tion for unspecified format

refactor(HttpServletUtils.writeErrorResponse): throw IOException, whi…

1443f96

…ch cause is IllegalArgumentException

chungen0126 requested a review from peterxcli November 7, 2025 03:08

chungen0126 reviewed Nov 7, 2025

View reviewed changes

Merge branch 'master' into HDDS-13258-refactor-HttpServletResponse

32cc44b

peterxcli approved these changes Nov 7, 2025

View reviewed changes

peterxcli merged commit 2e323b4 into apache:master Nov 7, 2025
83 of 84 checks passed

adoroszlai reviewed Nov 7, 2025

View reviewed changes

		Writer out) throws IOException {
		Writer out) throws IOException {

Conversation

unknowntpo commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unknowntpo commented Oct 9, 2025

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unknowntpo commented Oct 10, 2025

Uh oh!

adoroszlai commented Oct 13, 2025

Uh oh!

unknowntpo commented Oct 19, 2025

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

unknowntpo commented Oct 20, 2025

Uh oh!

adoroszlai commented Oct 21, 2025

Uh oh!

adoroszlai commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peterxcli Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unknowntpo Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chungen0126 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterxcli left a comment

Choose a reason for hiding this comment

Uh oh!

peterxcli commented Nov 7, 2025

Uh oh!

Uh oh!

peterxcli commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterxcli commented Nov 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

unknowntpo commented Oct 9, 2025 •

edited

Loading

peterxcli Oct 23, 2025 •

edited

Loading

unknowntpo Oct 27, 2025 •

edited

Loading