Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
216f57a
refactor: simplify duplicate code
unknowntpo Oct 6, 2025
59c319d
minor: bypass checkstyle: visibilitymodifier
unknowntpo Oct 6, 2025
b544d5f
refactor: revert SuppressWarnings
unknowntpo Oct 6, 2025
05ffdc8
refactor: move creation of volBucketPair inside constructor of Proces…
unknowntpo Oct 7, 2025
20d4bae
refactor: no need to throw BadFormatException since we fall back to x…
unknowntpo Oct 8, 2025
79ea893
refactor: add writeErrorResponse to write error response according to…
unknowntpo Oct 9, 2025
5bf2702
refactor: extract enum: ResponseFormat
unknowntpo Oct 9, 2025
0d5f9cf
refactor: use try with resource to close writer
unknowntpo Oct 9, 2025
84e7baa
fix: fix wrong switch case usage
unknowntpo Oct 9, 2025
0ad52f7
Merge branch 'master' into HDDS-13258-refactor-HttpServletResponse
unknowntpo Oct 9, 2025
24bc3e8
refactor: no need to close writer, because servlet container handled …
unknowntpo Oct 9, 2025
a8b885e
refactor: introduce HttpServletUtils
unknowntpo Oct 14, 2025
ea772b2
fix: fix testGetPropertyWithCmd
unknowntpo Oct 15, 2025
8c3e71c
refactor(HttpServletUtils): pass response as params to writeErrorResp…
unknowntpo Oct 15, 2025
91c115a
refactor: HddsConfServlet: remove unused error class
unknowntpo Oct 15, 2025
e0ddbb8
refactor(TestHttpServletUtils): introduce parameterized test to testP…
unknowntpo Oct 15, 2025
49e706b
refactor(HttpServletUtils): add status code to writeErrorResponse
unknowntpo Oct 15, 2025
16ad021
refactor(HttpServletUtils): refactor and rename to getResponseFormat
unknowntpo Oct 15, 2025
93d5a34
fix writeErrorResponse
unknowntpo Oct 15, 2025
c3992da
fix format
unknowntpo Oct 15, 2025
8f57f9f
refactor(HttpServletUtils): revert writeResponse method
unknowntpo Oct 15, 2025
60e1cc5
revert writeResponse
unknowntpo Oct 16, 2025
6734b23
docs: remove unused comment
unknowntpo Oct 16, 2025
52c89b4
refactor(HttpServletUtils): introduce ResponseFormat.getContentType()
unknowntpo Oct 16, 2025
ff49170
minor tweaks
unknowntpo Oct 16, 2025
1006c1c
minor: add license
unknowntpo Oct 16, 2025
e72b084
minor: add license
unknowntpo Oct 16, 2025
6e0b35a
minor: fix checkstyle
unknowntpo Oct 16, 2025
8f357fd
refactor(HttpServletUtils): extract DOCUMENT_BUILDER_FACTORY
unknowntpo Oct 20, 2025
dd6efd0
Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/con…
unknowntpo Oct 20, 2025
092e5fa
Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/con…
unknowntpo Oct 20, 2025
5c6aa18
Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/uti…
unknowntpo Oct 20, 2025
d65d5fc
Update hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/uti…
unknowntpo Oct 20, 2025
ae742f1
refactor(HddsConfServlet): explicitly throw BadFormatException
unknowntpo Oct 25, 2025
656fb87
refactor(HddsConfServlet): throw internal server error for BadFormatE…
unknowntpo Oct 25, 2025
e928eee
refactor(HttpServletUtils): add writeResponse method
unknowntpo Oct 27, 2025
c7c8567
remove VisibleForTesting
unknowntpo Oct 27, 2025
d42479c
minor: manually tweak indentation of `writeResponse`
unknowntpo Oct 27, 2025
d7e17f5
remove unused import
unknowntpo Oct 27, 2025
20a7bd3
refactor(HttpServletUtils.writeErrorResponse): explicitly throw excep…
unknowntpo Oct 27, 2025
1443f96
refactor(HttpServletUtils.writeErrorResponse): throw IOException, whi…
unknowntpo Oct 27, 2025
32cc44b
Merge branch 'master' into HDDS-13258-refactor-HttpServletResponse
peterxcli Nov 7, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

package org.apache.hadoop.hdds.conf;

import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
import java.io.Writer;
import java.util.HashMap;
Expand All @@ -27,23 +26,26 @@
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.ws.rs.core.HttpHeaders;
import org.apache.hadoop.conf.ConfServlet.BadFormatException;
import org.apache.hadoop.hdds.annotation.InterfaceAudience;
import org.apache.hadoop.hdds.annotation.InterfaceStability;
import org.apache.hadoop.hdds.server.JsonUtils;
import org.apache.hadoop.hdds.server.http.HttpServer2;
import org.apache.hadoop.hdds.utils.HttpServletUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* A servlet to print out the running configuration data.
*/
@InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"})
@InterfaceStability.Unstable
public class HddsConfServlet extends HttpServlet {
private static final Logger LOG =
LoggerFactory.getLogger(HddsConfServlet.class);

private static final long serialVersionUID = 1L;

protected static final String FORMAT_JSON = "json";
protected static final String FORMAT_XML = "xml";
private static final String COMMAND = "cmd";
private static final OzoneConfiguration OZONE_CONFIG =
new OzoneConfiguration();
Expand All @@ -55,7 +57,7 @@ public class HddsConfServlet extends HttpServlet {
private OzoneConfiguration getConfFromContext() {
OzoneConfiguration conf =
(OzoneConfiguration) getServletContext().getAttribute(
HttpServer2.CONF_CONTEXT_ATTRIBUTE);
HttpServer2.CONF_CONTEXT_ATTRIBUTE);
assert conf != null;
return conf;
}
Expand All @@ -69,75 +71,47 @@ public void doGet(HttpServletRequest request, HttpServletResponse response)
return;
}

String format = parseAcceptHeader(request);
if (FORMAT_XML.equals(format)) {
response.setContentType("text/xml; charset=utf-8");
} else if (FORMAT_JSON.equals(format)) {
response.setContentType("application/json; charset=utf-8");
HttpServletUtils.ResponseFormat format = HttpServletUtils.getResponseFormat(request);
if (format == HttpServletUtils.ResponseFormat.UNSPECIFIED) {
// use XML as default response format
format = HttpServletUtils.ResponseFormat.XML;
}

String name = request.getParameter("name");
Writer out = response.getWriter();
String cmd = request.getParameter(COMMAND);

processCommand(cmd, format, request, response, out, name);
out.close();
processCommand(cmd, format, request, response, name);
}

private void processCommand(String cmd, String format,
HttpServletRequest request, HttpServletResponse response, Writer out,
String name)
private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, HttpServletRequest request,
HttpServletResponse response, String name)
throws IOException {
try {
if (cmd == null) {
writeResponse(getConfFromContext(), out, format, name);
HttpServletUtils.writeResponse(response, format, (out) -> {
switch (format) {
case JSON:
OzoneConfiguration.dumpConfiguration(getConfFromContext(), name, out);
break;
case XML:
getConfFromContext().writeXml(name, out);
break;
default:
throw new BadFormatException("Bad format: " + format);
}
}, IllegalArgumentException.class);
} else {
processConfigTagRequest(request, cmd, out);
processConfigTagRequest(request, cmd, response);
}
} catch (BadFormatException bfe) {
response.sendError(HttpServletResponse.SC_BAD_REQUEST, bfe.getMessage());
} catch (IllegalArgumentException iae) {
response.sendError(HttpServletResponse.SC_NOT_FOUND, iae.getMessage());
}
}

@VisibleForTesting
static String parseAcceptHeader(HttpServletRequest request) {
String format = request.getHeader(HttpHeaders.ACCEPT);
return format != null && format.contains(FORMAT_JSON) ?
FORMAT_JSON : FORMAT_XML;
}

/**
* Guts of the servlet - extracted for easy testing.
*/
static void writeResponse(OzoneConfiguration conf,
Writer out, String format, String propertyName)
throws IOException, IllegalArgumentException, BadFormatException {
if (FORMAT_JSON.equals(format)) {
OzoneConfiguration.dumpConfiguration(conf, propertyName, out);
} else if (FORMAT_XML.equals(format)) {
conf.writeXml(propertyName, out);
} else {
throw new BadFormatException("Bad format: " + format);
}
}

/**
* Exception for signal bad content type.
*/
public static class BadFormatException extends Exception {

private static final long serialVersionUID = 1L;

public BadFormatException(String msg) {
super(msg);
HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_NOT_FOUND, iae.getMessage(), format, response);
}
}

private void processConfigTagRequest(HttpServletRequest request, String cmd,
Writer out) throws IOException {
private void processConfigTagRequest(HttpServletRequest request, String cmd, HttpServletResponse response)
throws IOException {
OzoneConfiguration config = getOzoneConfig();
Writer out = response.getWriter();

switch (cmd) {
case "getOzoneTags":
Expand All @@ -147,7 +121,7 @@ private void processConfigTagRequest(HttpServletRequest request, String cmd,
String tags = request.getParameter("tags");
if (tags == null || tags.isEmpty()) {
throw new IllegalArgumentException("The tags parameter should be set" +
" when using the getPropertyByTag command.");
" when using the getPropertyByTag command.");
}
Map<String, Properties> propMap = new HashMap<>();

Expand All @@ -162,7 +136,6 @@ private void processConfigTagRequest(HttpServletRequest request, String cmd,
default:
throw new IllegalArgumentException(cmd + " is not a valid command.");
}

}

private static OzoneConfiguration getOzoneConfig() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hadoop.hdds.utils;

import java.io.IOException;
import java.io.PrintWriter;
import java.io.Serializable;
import java.io.Writer;
import java.util.HashMap;
import java.util.Map;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.ws.rs.core.HttpHeaders;
import javax.ws.rs.core.MediaType;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerException;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.stream.StreamResult;
import org.apache.hadoop.hdds.server.JsonUtils;
import org.apache.hadoop.util.XMLUtils;
import org.apache.ratis.util.MemoizedCheckedSupplier;
import org.apache.ratis.util.function.CheckedSupplier;
import org.w3c.dom.Document;
import org.w3c.dom.Element;

/**
* Utility class for HTTP servlet operations.
* Provides methods for parsing request headers and writing responses.
*/
public final class HttpServletUtils implements Serializable {

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

private HttpServletUtils() {
// Utility class, prevent instantiation
}

/**
* Get the response format from request header.
*
* @param request the HTTP servlet request
* @return {@link ResponseFormat#JSON} if Accept header contains "application/json",
* otherwise {@link ResponseFormat#XML} (default for backwards compatibility)
* @see HttpHeaders#ACCEPT
*/
public static ResponseFormat getResponseFormat(HttpServletRequest request) throws IllegalArgumentException {
String format = request.getHeader(HttpHeaders.ACCEPT);
if (format == null) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return ResponseFormat.UNSPECIFIED;
}
return format.contains(ResponseFormat.JSON.getValue()) ?
ResponseFormat.JSON : ResponseFormat.XML;
}

/**
* Write error response according to the specified format.
*
* @param errorMessage the error message
* @param format the response format
* @param response the response
*/
public static void writeErrorResponse(int status, String errorMessage, ResponseFormat format,
HttpServletResponse response)
throws IOException {
response.setStatus(status);
PrintWriter writer = response.getWriter();
switch (format) {
case JSON:
Map<String, String> errorMap = new HashMap<>();
errorMap.put("error", errorMessage);
writer.write(JsonUtils.toJsonString(errorMap));
break;
case XML:
writeXmlError(errorMessage, writer);
break;
default:
throw new IOException("Unsupported response format for error response: " + format,
new IllegalArgumentException("Bad format: " + format));
}
Comment on lines +94 to +100
Copy link
Copy Markdown
Member

@peterxcli peterxcli Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto (#9126 (comment))

Suggested change
case XML:
default:
writeXmlError(errorMessage, writer);
break;
}
case XML:
writeXmlError(errorMessage, writer);
break;
default:
throw ....;
}

Copy link
Copy Markdown
Contributor Author

@unknowntpo unknowntpo Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

private static void writeXmlError(String errorMessage, Writer out) throws IOException {
try {
DocumentBuilder builder = DOCUMENT_BUILDER_FACTORY.get().newDocumentBuilder();
Document doc = builder.newDocument();

Element root = doc.createElement("error");
root.setTextContent(errorMessage);
doc.appendChild(root);

TransformerFactory transformerFactory = TransformerFactory.newInstance();
Transformer transformer = transformerFactory.newTransformer();
transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-8");
transformer.setOutputProperty(OutputKeys.STANDALONE, "no");

DOMSource source = new DOMSource(doc);
StreamResult result = new StreamResult(out);
transformer.transform(source, result);
} catch (ParserConfigurationException | TransformerException e) {
throw new IOException("Failed to write XML error response", e);
}
}

/**
* Write response according to the specified format.
* The caller provides a callback to write the content.
*
* @param response the HTTP servlet response
* @param format the response format
* @param contentWriter callback to write content to the writer
* @param exceptionClass class of exception to propagate from contentWriter
* @param <E> the type of exception that may be thrown by contentWriter
* @throws IOException if an I/O error occurs
* @throws E if contentWriter throws an exception of type E
*/
public static <E extends Exception> void writeResponse(HttpServletResponse response, ResponseFormat format,
CheckedConsumer<Writer> contentWriter, Class<E> exceptionClass) throws IOException, E {
response.setContentType(format.getContentType());
Writer out = response.getWriter();
try {
contentWriter.accept(out);
} catch (IOException e) {
// Always rethrow IOException as-is
throw e;
} catch (Exception e) {
// If exception matches the generic type, throw it
if (exceptionClass.isInstance(e)) {
throw exceptionClass.cast(e);
}
// Otherwise wrap in IOException
throw new IOException("Failed to write response", e);
}
}

/**
* 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;
}
Comment on lines +156 to +164
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Response format enumeration for HTTP responses.
* Supports JSON, XML, and UNSPECIFIED formats.
*/
public enum ResponseFormat {
UNSPECIFIED("unspecified"),
JSON("json"),
XML("xml");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

private final String value;

ResponseFormat(String value) {
this.value = value;
}

/**
* Get the string value of this response format.
*
* @return the format value (e.g., "json", "xml", "unspecified")
*/
public String getValue() {
return value;
}

@Override
public String toString() {
return value;
}

/**
* Get Content-Type header value with UTF-8 charset for this format.
*
* @return Content-Type string (e.g., "application/json;charset=utf-8"),
* or null if UNSPECIFIED
*/
public String getContentType() {
switch (this) {
case JSON:
return MediaType.APPLICATION_JSON_TYPE.withCharset("utf-8").toString();
case XML:
return MediaType.APPLICATION_XML_TYPE.withCharset("utf-8").toString();
default:
return null;
}
}
}
}
Loading