From 216f57a65ed25ef4eb881107891bfe93e779429b Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 6 Oct 2025 19:26:43 +0800 Subject: [PATCH 01/40] refactor: simplify duplicate code --- .../key/OMDirectoriesPurgeRequestWithFSO.java | 94 ++++++++++++------- 1 file changed, 59 insertions(+), 35 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java index e30a66aa124e..c1b05067671f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java @@ -85,7 +85,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut purgeDirsRequest.getSnapshotTableKey() : null; List purgeRequests = - purgeDirsRequest.getDeletedPathList(); + purgeDirsRequest.getDeletedPathList(); Map, OmBucketInfo> volBucketInfoMap = new HashMap<>(); OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl) ozoneManager.getMetadataManager(); Map openKeyInfoMap = new HashMap<>(); @@ -133,56 +133,38 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut for (OzoneManagerProtocolProtos.PurgePathRequest path : purgeRequests) { for (OzoneManagerProtocolProtos.KeyInfo key : path.getMarkDeletedSubDirsList()) { - OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key); - - String pathKey = omMetadataManager.getOzonePathKey(path.getVolumeId(), - path.getBucketId(), keyInfo.getParentObjectID(), keyInfo.getFileName()); - String deleteKey = omMetadataManager.getOzoneDeletePathKey( - keyInfo.getObjectID(), pathKey); - - subDirNames.add(deleteKey); - - String volumeName = keyInfo.getVolumeName(); - String bucketName = keyInfo.getBucketName(); - Pair volBucketPair = Pair.of(volumeName, bucketName); + ProcessedKeyInfo processed = processDeleteKey(key, path, omMetadataManager); + subDirNames.add(processed.deleteKey); omMetrics.decNumKeys(); OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager, - volumeName, bucketName); + processed.volumeName, processed.bucketName); // bucketInfo can be null in case of delete volume or bucket // or key does not belong to bucket as bucket is recreated if (null != omBucketInfo && omBucketInfo.getObjectID() == path.getBucketId()) { omBucketInfo.incrUsedNamespace(-1L); String ozoneDbKey = omMetadataManager.getOzonePathKey(path.getVolumeId(), - path.getBucketId(), keyInfo.getParentObjectID(), keyInfo.getFileName()); + path.getBucketId(), processed.keyInfo.getParentObjectID(), + processed.keyInfo.getFileName()); omMetadataManager.getDirectoryTable().addCacheEntry(new CacheKey<>(ozoneDbKey), CacheValue.get(context.getIndex())); - volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo); + volBucketInfoMap.putIfAbsent(processed.volBucketPair, omBucketInfo); } } for (OzoneManagerProtocolProtos.KeyInfo key : path.getDeletedSubFilesList()) { - OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key); - - String pathKey = omMetadataManager.getOzonePathKey(path.getVolumeId(), - path.getBucketId(), keyInfo.getParentObjectID(), keyInfo.getFileName()); - String deleteKey = omMetadataManager.getOzoneDeletePathKey( - keyInfo.getObjectID(), pathKey); - subFileNames.add(deleteKey); - - String volumeName = keyInfo.getVolumeName(); - String bucketName = keyInfo.getBucketName(); - Pair volBucketPair = Pair.of(volumeName, bucketName); + ProcessedKeyInfo processed = processDeleteKey(key, path, omMetadataManager); + subFileNames.add(processed.deleteKey); // If omKeyInfo has hsync metadata, delete its corresponding open key as well String dbOpenKey; - String hsyncClientId = keyInfo.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); + String hsyncClientId = processed.keyInfo.getMetadata().get(OzoneConsts.HSYNC_CLIENT_ID); if (hsyncClientId != null) { - long parentId = keyInfo.getParentObjectID(); + long parentId = processed.keyInfo.getParentObjectID(); dbOpenKey = omMetadataManager.getOpenFileName(path.getVolumeId(), path.getBucketId(), - parentId, keyInfo.getFileName(), hsyncClientId); + parentId, processed.keyInfo.getFileName(), hsyncClientId); OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(getBucketLayout()).get(dbOpenKey); if (openKeyInfo != null) { openKeyInfo.getMetadata().put(DELETED_HSYNC_KEY, "true"); @@ -193,18 +175,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut omMetrics.decNumKeys(); numSubFilesMoved++; OmBucketInfo omBucketInfo = getBucketInfo(omMetadataManager, - volumeName, bucketName); + processed.volumeName, processed.bucketName); // bucketInfo can be null in case of delete volume or bucket // or key does not belong to bucket as bucket is recreated if (null != omBucketInfo && omBucketInfo.getObjectID() == path.getBucketId()) { - omBucketInfo.incrUsedBytes(-sumBlockLengths(keyInfo)); + omBucketInfo.incrUsedBytes(-sumBlockLengths(processed.keyInfo)); omBucketInfo.incrUsedNamespace(-1L); String ozoneDbKey = omMetadataManager.getOzonePathKey(path.getVolumeId(), - path.getBucketId(), keyInfo.getParentObjectID(), keyInfo.getFileName()); + path.getBucketId(), processed.keyInfo.getParentObjectID(), + processed.keyInfo.getFileName()); omMetadataManager.getFileTable().addCacheEntry(new CacheKey<>(ozoneDbKey), CacheValue.get(context.getIndex())); - volBucketInfoMap.putIfAbsent(volBucketPair, omBucketInfo); + volBucketInfoMap.putIfAbsent(processed.volBucketPair, omBucketInfo); } } if (path.hasDeletedDir()) { @@ -212,7 +195,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut numDirsDeleted++; } } - + // Remove deletedDirNames from subDirNames to avoid duplication subDirNames.removeAll(deletedDirNames); numSubDirMoved = subDirNames.size(); @@ -254,6 +237,47 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut getBucketLayout(), volBucketInfoMap, fromSnapshotInfo, openKeyInfoMap); } + /** + * Helper class to hold processed key information. + */ + private static class ProcessedKeyInfo { + final OmKeyInfo keyInfo; + final String deleteKey; + final String volumeName; + final String bucketName; + final Pair volBucketPair; + + ProcessedKeyInfo(OmKeyInfo keyInfo, String deleteKey, String volumeName, String bucketName, + Pair volBucketPair) { + this.keyInfo = keyInfo; + this.deleteKey = deleteKey; + this.volumeName = volumeName; + this.bucketName = bucketName; + this.volBucketPair = volBucketPair; + } + } + + /** + * Process delete key info. + * Returns ProcessedKeyInfo containing all the processed information. + */ + private ProcessedKeyInfo processDeleteKey(OzoneManagerProtocolProtos.KeyInfo key, + OzoneManagerProtocolProtos.PurgePathRequest path, + OmMetadataManagerImpl omMetadataManager) { + OmKeyInfo keyInfo = OmKeyInfo.getFromProtobuf(key); + + String pathKey = omMetadataManager.getOzonePathKey(path.getVolumeId(), + path.getBucketId(), keyInfo.getParentObjectID(), keyInfo.getFileName()); + String deleteKey = omMetadataManager.getOzoneDeletePathKey( + keyInfo.getObjectID(), pathKey); + + String volumeName = keyInfo.getVolumeName(); + String bucketName = keyInfo.getBucketName(); + Pair volBucketPair = Pair.of(volumeName, bucketName); + + return new ProcessedKeyInfo(keyInfo, deleteKey, volumeName, bucketName, volBucketPair); + } + private List getBucketLockKeySet(PurgeDirectoriesRequest purgeDirsRequest) { if (!purgeDirsRequest.getBucketNameInfosList().isEmpty()) { return purgeDirsRequest.getBucketNameInfosList().stream() From 59c319d452888495cd0cbae997a6dd23a36d3c0a Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 6 Oct 2025 20:02:25 +0800 Subject: [PATCH 02/40] minor: bypass checkstyle: visibilitymodifier --- .../ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java index c1b05067671f..259576774492 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java @@ -240,6 +240,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut /** * Helper class to hold processed key information. */ + @SuppressWarnings("visibilitymodifier") private static class ProcessedKeyInfo { final OmKeyInfo keyInfo; final String deleteKey; From b544d5f79051427b01ad9b6b769cc6cedbad317f Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 6 Oct 2025 20:57:32 +0800 Subject: [PATCH 03/40] refactor: revert SuppressWarnings --- .../request/key/OMDirectoriesPurgeRequestWithFSO.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java index 259576774492..ec7f6a41789b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java @@ -240,13 +240,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut /** * Helper class to hold processed key information. */ - @SuppressWarnings("visibilitymodifier") private static class ProcessedKeyInfo { - final OmKeyInfo keyInfo; - final String deleteKey; - final String volumeName; - final String bucketName; - final Pair volBucketPair; + private final OmKeyInfo keyInfo; + private final String deleteKey; + private final String volumeName; + private final String bucketName; + private final Pair volBucketPair; ProcessedKeyInfo(OmKeyInfo keyInfo, String deleteKey, String volumeName, String bucketName, Pair volBucketPair) { From 05ffdc854c5457097ca5b0629cf30e3f0b08b0f2 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Tue, 7 Oct 2025 15:55:15 +0800 Subject: [PATCH 04/40] refactor: move creation of volBucketPair inside constructor of ProcessedKeyInfo --- .../om/request/key/OMDirectoriesPurgeRequestWithFSO.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java index ec7f6a41789b..fb0af869884b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java @@ -247,13 +247,12 @@ private static class ProcessedKeyInfo { private final String bucketName; private final Pair volBucketPair; - ProcessedKeyInfo(OmKeyInfo keyInfo, String deleteKey, String volumeName, String bucketName, - Pair volBucketPair) { + ProcessedKeyInfo(OmKeyInfo keyInfo, String deleteKey, String volumeName, String bucketName) { this.keyInfo = keyInfo; this.deleteKey = deleteKey; this.volumeName = volumeName; this.bucketName = bucketName; - this.volBucketPair = volBucketPair; + this.volBucketPair = Pair.of(volumeName, bucketName); } } @@ -273,9 +272,8 @@ private ProcessedKeyInfo processDeleteKey(OzoneManagerProtocolProtos.KeyInfo key String volumeName = keyInfo.getVolumeName(); String bucketName = keyInfo.getBucketName(); - Pair volBucketPair = Pair.of(volumeName, bucketName); - return new ProcessedKeyInfo(keyInfo, deleteKey, volumeName, bucketName, volBucketPair); + return new ProcessedKeyInfo(keyInfo, deleteKey, volumeName, bucketName); } private List getBucketLockKeySet(PurgeDirectoriesRequest purgeDirsRequest) { From 20d4baebdebbf32bbf00fc07a9fdf3d5089e2e58 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 9 Oct 2025 07:42:27 +0800 Subject: [PATCH 05/40] refactor: no need to throw BadFormatException since we fall back to xml if accept header has invalid value --- .../apache/hadoop/hdds/conf/HddsConfServlet.java | 14 +++++++++----- .../hadoop/hdds/conf/TestHddsConfServlet.java | 11 ++--------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index cc5513e5e900..9d7f524f73e6 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -94,13 +94,19 @@ private void processCommand(String cmd, String format, } else { processConfigTagRequest(request, cmd, out); } - } catch (BadFormatException bfe) { - response.sendError(HttpServletResponse.SC_BAD_REQUEST, bfe.getMessage()); } catch (IllegalArgumentException iae) { response.sendError(HttpServletResponse.SC_NOT_FOUND, iae.getMessage()); } } + /** + * Parse the Accept header to determine response format. + * + * @param request the HTTP servlet request + * @return {@link #FORMAT_JSON} if Accept header contains "application/json", + * otherwise {@link #FORMAT_XML} (default for backwards compatibility) + * @see HttpHeaders#ACCEPT + */ @VisibleForTesting static String parseAcceptHeader(HttpServletRequest request) { String format = request.getHeader(HttpHeaders.ACCEPT); @@ -113,13 +119,11 @@ static String parseAcceptHeader(HttpServletRequest request) { */ static void writeResponse(OzoneConfiguration conf, Writer out, String format, String propertyName) - throws IOException, IllegalArgumentException, BadFormatException { + throws IOException, IllegalArgumentException { 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); } } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index 3c262fdc9243..9750280e0fee 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -17,9 +17,9 @@ package org.apache.hadoop.hdds.conf; +import static org.apache.hadoop.hdds.conf.HddsConfServlet.FORMAT_JSON; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.mock; @@ -166,14 +166,6 @@ public void testWriteXml() throws Exception { assertTrue(foundSetting); } - @Test - public void testBadFormat() throws Exception { - StringWriter sw = new StringWriter(); - assertThrows(HddsConfServlet.BadFormatException.class, - () -> HddsConfServlet.writeResponse(getTestConf(), sw, "not a format", null)); - assertEquals("", sw.toString()); - } - private String getResultWithCmd(OzoneConfiguration conf, String cmd) throws Exception { StringWriter sw = null; @@ -199,6 +191,7 @@ private String getResultWithCmd(OzoneConfiguration conf, String cmd) // response request service.doGet(request, response); if (cmd.equals("illegal")) { + // FIXME: if we are sending error response respect to xml / json, we dont need this verify verify(response).sendError( eq(HttpServletResponse.SC_NOT_FOUND), eq("illegal is not a valid command.")); From 79ea893dee1faad11d3e3baac7658bf1b6ec8b3a Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 9 Oct 2025 10:34:49 +0800 Subject: [PATCH 06/40] refactor: add writeErrorResponse to write error response according to specified format --- .../hadoop/hdds/conf/HddsConfServlet.java | 71 ++++++++++++++++--- .../hadoop/hdds/conf/TestHddsConfServlet.java | 24 +++---- 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 9d7f524f73e6..11ff55fd39ab 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -28,10 +28,22 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.ws.rs.core.HttpHeaders; +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.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.util.XMLUtils; +import org.w3c.dom.Document; +import org.w3c.dom.Element; /** * A servlet to print out the running configuration data. @@ -55,7 +67,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; } @@ -80,13 +92,14 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) Writer out = response.getWriter(); String cmd = request.getParameter(COMMAND); + // FIXME: should close writer if any exception is thrown. processCommand(cmd, format, request, response, out, name); out.close(); } private void processCommand(String cmd, String format, - HttpServletRequest request, HttpServletResponse response, Writer out, - String name) + HttpServletRequest request, HttpServletResponse response, Writer out, + String name) throws IOException { try { if (cmd == null) { @@ -95,7 +108,8 @@ private void processCommand(String cmd, String format, processConfigTagRequest(request, cmd, out); } } catch (IllegalArgumentException iae) { - response.sendError(HttpServletResponse.SC_NOT_FOUND, iae.getMessage()); + response.setStatus(HttpServletResponse.SC_NOT_FOUND); + writeErrorResponse(iae.getMessage(), format, out); } } @@ -104,7 +118,7 @@ private void processCommand(String cmd, String format, * * @param request the HTTP servlet request * @return {@link #FORMAT_JSON} if Accept header contains "application/json", - * otherwise {@link #FORMAT_XML} (default for backwards compatibility) + * otherwise {@link #FORMAT_XML} (default for backwards compatibility) * @see HttpHeaders#ACCEPT */ @VisibleForTesting @@ -118,7 +132,7 @@ static String parseAcceptHeader(HttpServletRequest request) { * Guts of the servlet - extracted for easy testing. */ static void writeResponse(OzoneConfiguration conf, - Writer out, String format, String propertyName) + Writer out, String format, String propertyName) throws IOException, IllegalArgumentException { if (FORMAT_JSON.equals(format)) { OzoneConfiguration.dumpConfiguration(conf, propertyName, out); @@ -127,6 +141,47 @@ static void writeResponse(OzoneConfiguration conf, } } + /** + * Write error response respect to format + * + * @param errorMessage the error message + * @param format the response format (json or xml) + * @param out the writer + */ + static void writeErrorResponse(String errorMessage, String format, Writer out) + throws IOException { + if (FORMAT_JSON.equals(format)) { + Map errorMap = new HashMap<>(); + errorMap.put("error", errorMessage); + out.write(JsonUtils.toJsonString(errorMap)); + } else if (FORMAT_XML.equals(format)) { + writeXmlError(errorMessage, out); + } + } + + private static void writeXmlError(String errorMessage, Writer out) throws IOException { + try { + DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory(); + DocumentBuilder builder = factory.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); + } + } + /** * Exception for signal bad content type. */ @@ -140,7 +195,7 @@ public BadFormatException(String msg) { } private void processConfigTagRequest(HttpServletRequest request, String cmd, - Writer out) throws IOException { + Writer out) throws IOException { OzoneConfiguration config = getOzoneConfig(); switch (cmd) { @@ -151,7 +206,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 propMap = new HashMap<>(); diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index 9750280e0fee..d62fd814c481 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -114,8 +114,11 @@ public void testGetPropertyWithCmd() throws Exception { // cmd is getPropertyByTag result = getResultWithCmd(conf, "getPropertyByTag"); assertThat(result).contains("ozone.test.test.key"); - // cmd is illegal - getResultWithCmd(conf, "illegal"); + // cmd is illegal - verify XML error response + result = getResultWithCmd(conf, "illegal"); + String expectedXmlResult = "" + + "illegal is not a valid command."; + assertEquals(expectedXmlResult, result); } @Test @@ -190,14 +193,7 @@ private String getResultWithCmd(OzoneConfiguration conf, String cmd) when(response.getWriter()).thenReturn(pw); // response request service.doGet(request, response); - if (cmd.equals("illegal")) { - // FIXME: if we are sending error response respect to xml / json, we dont need this verify - verify(response).sendError( - eq(HttpServletResponse.SC_NOT_FOUND), - eq("illegal is not a valid command.")); - } - String result = sw.toString().trim(); - return result; + return sw.toString().trim(); } finally { if (sw != null) { sw.close(); @@ -256,11 +252,9 @@ private void verifyGetProperty(OzoneConfiguration conf, String format, } } else { // if property name is not empty, and it's not in configuration - // expect proper error code and error message is set to the response - verify(response) - .sendError( - eq(HttpServletResponse.SC_NOT_FOUND), - eq("Property " + propertyName + " not found")); + // expect proper error code and error message in response + verify(response).setStatus(eq(HttpServletResponse.SC_NOT_FOUND)); + assertThat(result).contains("Property " + propertyName + " not found"); } } } finally { From 5bf2702bb546aa69bfb5aff0664fb3a18533039c Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 9 Oct 2025 10:52:09 +0800 Subject: [PATCH 07/40] refactor: extract enum: ResponseFormat --- .../hadoop/hdds/conf/HddsConfServlet.java | 60 ++++++++++++------- .../hadoop/hdds/conf/TestHddsConfServlet.java | 25 ++++---- 2 files changed, 52 insertions(+), 33 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 11ff55fd39ab..6acab4ee547a 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -54,8 +54,6 @@ public class HddsConfServlet extends HttpServlet { 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(); @@ -81,10 +79,11 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) return; } - String format = parseAcceptHeader(request); - if (FORMAT_XML.equals(format)) { + ResponseFormat format = parseAcceptHeader(request); + switch (format) { + case JSON: response.setContentType("text/xml; charset=utf-8"); - } else if (FORMAT_JSON.equals(format)) { + case XML: response.setContentType("application/json; charset=utf-8"); } @@ -97,7 +96,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) out.close(); } - private void processCommand(String cmd, String format, + private void processCommand(String cmd, ResponseFormat format, HttpServletRequest request, HttpServletResponse response, Writer out, String name) throws IOException { @@ -117,44 +116,46 @@ private void processCommand(String cmd, String format, * Parse the Accept header to determine response format. * * @param request the HTTP servlet request - * @return {@link #FORMAT_JSON} if Accept header contains "application/json", - * otherwise {@link #FORMAT_XML} (default for backwards compatibility) + * @return {@link ResponseFormat#JSON} if Accept header contains "application/json", + * otherwise {@link ResponseFormat#XML} (default for backwards compatibility) * @see HttpHeaders#ACCEPT */ @VisibleForTesting - static String parseAcceptHeader(HttpServletRequest request) { + static ResponseFormat parseAcceptHeader(HttpServletRequest request) { String format = request.getHeader(HttpHeaders.ACCEPT); - return format != null && format.contains(FORMAT_JSON) ? - FORMAT_JSON : FORMAT_XML; + return format != null && format.contains(ResponseFormat.JSON.getValue()) ? + ResponseFormat.JSON : ResponseFormat.XML; } /** * Guts of the servlet - extracted for easy testing. */ static void writeResponse(OzoneConfiguration conf, - Writer out, String format, String propertyName) + Writer out, ResponseFormat format, String propertyName) throws IOException, IllegalArgumentException { - if (FORMAT_JSON.equals(format)) { + switch (format) { + case JSON: OzoneConfiguration.dumpConfiguration(conf, propertyName, out); - } else if (FORMAT_XML.equals(format)) { + case XML: conf.writeXml(propertyName, out); } } /** - * Write error response respect to format + * Write error response according to the specified format. * * @param errorMessage the error message - * @param format the response format (json or xml) - * @param out the writer + * @param format the response format + * @param out the writer */ - static void writeErrorResponse(String errorMessage, String format, Writer out) + static void writeErrorResponse(String errorMessage, ResponseFormat format, Writer out) throws IOException { - if (FORMAT_JSON.equals(format)) { + switch (format) { + case JSON: Map errorMap = new HashMap<>(); errorMap.put("error", errorMessage); out.write(JsonUtils.toJsonString(errorMap)); - } else if (FORMAT_XML.equals(format)) { + case XML: writeXmlError(errorMessage, out); } } @@ -182,6 +183,25 @@ private static void writeXmlError(String errorMessage, Writer out) throws IOExce } } + enum ResponseFormat { + JSON("json"), + XML("xml"); + private final String value; + + ResponseFormat(String value) { + this.value = value; + } + + public String getValue() { + return value; + } + + @Override + public String toString() { + return value; + } + } + /** * Exception for signal bad content type. */ diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index d62fd814c481..e9ad5528cf80 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -17,7 +17,6 @@ package org.apache.hadoop.hdds.conf; -import static org.apache.hadoop.hdds.conf.HddsConfServlet.FORMAT_JSON; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -64,22 +63,22 @@ public static void setup() { TEST_PROPERTIES.put("test.key1", "value1"); TEST_PROPERTIES.put("test.key2", "value2"); TEST_PROPERTIES.put("test.key3", "value3"); - TEST_FORMATS.put(HddsConfServlet.FORMAT_XML, "application/xml"); - TEST_FORMATS.put(HddsConfServlet.FORMAT_JSON, "application/json"); + TEST_FORMATS.put(HddsConfServlet.ResponseFormat.XML.toString(), "application/xml"); + TEST_FORMATS.put(HddsConfServlet.ResponseFormat.JSON.toString(), "application/json"); } @Test public void testParseHeaders() throws Exception { - HashMap verifyMap = new HashMap(); - verifyMap.put("text/plain", HddsConfServlet.FORMAT_XML); - verifyMap.put(null, HddsConfServlet.FORMAT_XML); - verifyMap.put("text/xml", HddsConfServlet.FORMAT_XML); - verifyMap.put("application/xml", HddsConfServlet.FORMAT_XML); - verifyMap.put("application/json", HddsConfServlet.FORMAT_JSON); + HashMap verifyMap = new HashMap<>(); + verifyMap.put("text/plain", HddsConfServlet.ResponseFormat.XML); + verifyMap.put(null, HddsConfServlet.ResponseFormat.XML); + verifyMap.put("text/xml", HddsConfServlet.ResponseFormat.XML); + verifyMap.put("application/xml", HddsConfServlet.ResponseFormat.XML); + verifyMap.put("application/json", HddsConfServlet.ResponseFormat.JSON); HttpServletRequest request = mock(HttpServletRequest.class); - for (Map.Entry entry : verifyMap.entrySet()) { - String contenTypeActual = entry.getValue(); + for (Map.Entry entry : verifyMap.entrySet()) { + HddsConfServlet.ResponseFormat contenTypeActual = entry.getValue(); when(request.getHeader(HttpHeaders.ACCEPT)) .thenReturn(entry.getKey()); assertEquals(contenTypeActual, @@ -125,7 +124,7 @@ public void testGetPropertyWithCmd() throws Exception { @SuppressWarnings("unchecked") public void testWriteJson() throws Exception { StringWriter sw = new StringWriter(); - HddsConfServlet.writeResponse(getTestConf(), sw, "json", null); + HddsConfServlet.writeResponse(getTestConf(), sw, HddsConfServlet.ResponseFormat.JSON, null); String json = sw.toString(); boolean foundSetting = false; Object parsed = JSON.parse(json); @@ -146,7 +145,7 @@ public void testWriteJson() throws Exception { @Test public void testWriteXml() throws Exception { StringWriter sw = new StringWriter(); - HddsConfServlet.writeResponse(getTestConf(), sw, "xml", null); + HddsConfServlet.writeResponse(getTestConf(), sw, HddsConfServlet.ResponseFormat.XML, null); String xml = sw.toString(); DocumentBuilderFactory docBuilderFactory = From 0d5f9cfbdbfa07d5fc046a9118b656afdf738c5d Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 9 Oct 2025 10:54:47 +0800 Subject: [PATCH 08/40] refactor: use try with resource to close writer --- .../java/org/apache/hadoop/hdds/conf/HddsConfServlet.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 6acab4ee547a..7b7cab6e0dc3 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -88,12 +88,11 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) } String name = request.getParameter("name"); - Writer out = response.getWriter(); String cmd = request.getParameter(COMMAND); - // FIXME: should close writer if any exception is thrown. - processCommand(cmd, format, request, response, out, name); - out.close(); + try (Writer out = response.getWriter()) { + processCommand(cmd, format, request, response, out, name); + } } private void processCommand(String cmd, ResponseFormat format, From 84e7baa10525751fb86da99baf335f32987650b4 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 9 Oct 2025 11:17:06 +0800 Subject: [PATCH 09/40] fix: fix wrong switch case usage --- .../apache/hadoop/hdds/conf/HddsConfServlet.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 7b7cab6e0dc3..4e37548eb785 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -82,9 +82,12 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) ResponseFormat format = parseAcceptHeader(request); switch (format) { case JSON: - response.setContentType("text/xml; charset=utf-8"); - case XML: response.setContentType("application/json; charset=utf-8"); + break; + case XML: + default: + response.setContentType("text/xml; charset=utf-8"); + break; } String name = request.getParameter("name"); @@ -135,8 +138,11 @@ static void writeResponse(OzoneConfiguration conf, switch (format) { case JSON: OzoneConfiguration.dumpConfiguration(conf, propertyName, out); + break; case XML: + default: conf.writeXml(propertyName, out); + break; } } @@ -154,8 +160,11 @@ static void writeErrorResponse(String errorMessage, ResponseFormat format, Write Map errorMap = new HashMap<>(); errorMap.put("error", errorMessage); out.write(JsonUtils.toJsonString(errorMap)); + break; case XML: + default: writeXmlError(errorMessage, out); + break; } } From 24bc3e8f46eff4902823e84d1a005fd21717cbfd Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 9 Oct 2025 12:12:37 +0800 Subject: [PATCH 10/40] refactor: no need to close writer, because servlet container handled this --- .../java/org/apache/hadoop/hdds/conf/HddsConfServlet.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 4e37548eb785..75fdb0d4757f 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -91,11 +91,10 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) } String name = request.getParameter("name"); + Writer out = response.getWriter(); String cmd = request.getParameter(COMMAND); - try (Writer out = response.getWriter()) { - processCommand(cmd, format, request, response, out, name); - } + processCommand(cmd, format, request, response, out, name); } private void processCommand(String cmd, ResponseFormat format, From a8b885ec37b03d3caca5bce22abe1c77c6cbf58b Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 07:51:53 +0800 Subject: [PATCH 11/40] refactor: introduce HttpServletUtils --- .../hadoop/hdds/conf/HddsConfServlet.java | 125 +----------------- .../hadoop/hdds/utils/HttpServletUtils.java | 125 ++++++++++++++++++ .../hadoop/hdds/conf/TestHddsConfServlet.java | 28 +--- .../hdds/utils/TestHttpServletUtils.java | 33 +++++ 4 files changed, 170 insertions(+), 141 deletions(-) create mode 100644 hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java create mode 100644 hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 75fdb0d4757f..2f47ef1fa837 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -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; @@ -27,23 +26,11 @@ import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import javax.ws.rs.core.HttpHeaders; -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.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.util.XMLUtils; -import org.w3c.dom.Document; -import org.w3c.dom.Element; +import org.apache.hadoop.hdds.utils.HttpServletUtils; /** * A servlet to print out the running configuration data. @@ -79,7 +66,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) return; } - ResponseFormat format = parseAcceptHeader(request); + HttpServletUtils.ResponseFormat format = HttpServletUtils.parseAcceptHeader(request); switch (format) { case JSON: response.setContentType("application/json; charset=utf-8"); @@ -97,115 +84,18 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) processCommand(cmd, format, request, response, out, name); } - private void processCommand(String cmd, ResponseFormat format, - HttpServletRequest request, HttpServletResponse response, Writer out, - String name) + private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, HttpServletRequest request, + HttpServletResponse response, Writer out, String name) throws IOException { try { if (cmd == null) { - writeResponse(getConfFromContext(), out, format, name); + HttpServletUtils.writeResponse(getConfFromContext(), out, format, name); } else { processConfigTagRequest(request, cmd, out); } } catch (IllegalArgumentException iae) { response.setStatus(HttpServletResponse.SC_NOT_FOUND); - writeErrorResponse(iae.getMessage(), format, out); - } - } - - /** - * Parse the Accept header to determine response format. - * - * @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 - */ - @VisibleForTesting - static ResponseFormat parseAcceptHeader(HttpServletRequest request) { - String format = request.getHeader(HttpHeaders.ACCEPT); - return format != null && format.contains(ResponseFormat.JSON.getValue()) ? - ResponseFormat.JSON : ResponseFormat.XML; - } - - /** - * Guts of the servlet - extracted for easy testing. - */ - static void writeResponse(OzoneConfiguration conf, - Writer out, ResponseFormat format, String propertyName) - throws IOException, IllegalArgumentException { - switch (format) { - case JSON: - OzoneConfiguration.dumpConfiguration(conf, propertyName, out); - break; - case XML: - default: - conf.writeXml(propertyName, out); - break; - } - } - - /** - * Write error response according to the specified format. - * - * @param errorMessage the error message - * @param format the response format - * @param out the writer - */ - static void writeErrorResponse(String errorMessage, ResponseFormat format, Writer out) - throws IOException { - switch (format) { - case JSON: - Map errorMap = new HashMap<>(); - errorMap.put("error", errorMessage); - out.write(JsonUtils.toJsonString(errorMap)); - break; - case XML: - default: - writeXmlError(errorMessage, out); - break; - } - } - - private static void writeXmlError(String errorMessage, Writer out) throws IOException { - try { - DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory(); - DocumentBuilder builder = factory.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); - } - } - - enum ResponseFormat { - JSON("json"), - XML("xml"); - private final String value; - - ResponseFormat(String value) { - this.value = value; - } - - public String getValue() { - return value; - } - - @Override - public String toString() { - return value; + HttpServletUtils.writeErrorResponse(iae.getMessage(), format, out); } } @@ -221,8 +111,7 @@ public BadFormatException(String msg) { } } - private void processConfigTagRequest(HttpServletRequest request, String cmd, - Writer out) throws IOException { + private void processConfigTagRequest(HttpServletRequest request, String cmd, Writer out) throws IOException { OzoneConfiguration config = getOzoneConfig(); switch (cmd) { diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java new file mode 100644 index 000000000000..38b6ecf0ab4b --- /dev/null +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -0,0 +1,125 @@ +package org.apache.hadoop.hdds.utils; + +import com.google.common.annotations.VisibleForTesting; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.server.JsonUtils; +import org.apache.hadoop.util.XMLUtils; +import org.w3c.dom.Document; +import org.w3c.dom.Element; + +import javax.servlet.http.HttpServletRequest; +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 java.io.IOException; +import java.io.Serializable; +import java.io.Writer; +import java.util.HashMap; +import java.util.Map; + +public class HttpServletUtils implements Serializable { + /** + * Parse the Accept header to determine response format. + * + * @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 + */ + @VisibleForTesting + public static ResponseFormat parseAcceptHeader(HttpServletRequest request) { + String format = request.getHeader(HttpHeaders.ACCEPT); + MediaType mediaType = MediaType.valueOf(format); + return format != null && format.contains(ResponseFormat.JSON.getValue()) ? + ResponseFormat.JSON : ResponseFormat.XML; + } + + /** + * Guts of the servlet - extracted for easy testing. + */ + public static void writeResponse(OzoneConfiguration conf, + Writer out, ResponseFormat format, String propertyName) + throws IOException, IllegalArgumentException { + switch (format) { + case JSON: + OzoneConfiguration.dumpConfiguration(conf, propertyName, out); + break; + case XML: + default: + conf.writeXml(propertyName, out); + break; + } + } + + /** + * Write error response according to the specified format. + * + * @param errorMessage the error message + * @param format the response format + * @param out the writer + */ + public static void writeErrorResponse(String errorMessage, ResponseFormat format, Writer out) + throws IOException { + switch (format) { + case JSON: + Map errorMap = new HashMap(); + errorMap.put("error", errorMessage); + out.write(JsonUtils.toJsonString(errorMap)); + break; + case XML: + default: + writeXmlError(errorMessage, out); + break; + } + } + + public static void writeXmlError(String errorMessage, Writer out) throws IOException { + try { + DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory(); + DocumentBuilder builder = factory.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); + } + } + + public enum ResponseFormat { + JSON("json"), + XML("xml"); + private final String value; + + ResponseFormat(String value) { + this.value = value; + } + + public String getValue() { + return value; + } + + @Override + public String toString() { + return value; + } + } +} diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index e9ad5528cf80..6f2bce81b8d2 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -40,6 +40,7 @@ import javax.xml.parsers.DocumentBuilderFactory; import org.apache.hadoop.hdds.JsonTestUtils; import org.apache.hadoop.hdds.server.http.HttpServer2; +import org.apache.hadoop.hdds.utils.HttpServletUtils; import org.apache.hadoop.util.XMLUtils; import org.eclipse.jetty.util.ajax.JSON; import org.junit.jupiter.api.BeforeAll; @@ -63,27 +64,8 @@ public static void setup() { TEST_PROPERTIES.put("test.key1", "value1"); TEST_PROPERTIES.put("test.key2", "value2"); TEST_PROPERTIES.put("test.key3", "value3"); - TEST_FORMATS.put(HddsConfServlet.ResponseFormat.XML.toString(), "application/xml"); - TEST_FORMATS.put(HddsConfServlet.ResponseFormat.JSON.toString(), "application/json"); - } - - @Test - public void testParseHeaders() throws Exception { - HashMap verifyMap = new HashMap<>(); - verifyMap.put("text/plain", HddsConfServlet.ResponseFormat.XML); - verifyMap.put(null, HddsConfServlet.ResponseFormat.XML); - verifyMap.put("text/xml", HddsConfServlet.ResponseFormat.XML); - verifyMap.put("application/xml", HddsConfServlet.ResponseFormat.XML); - verifyMap.put("application/json", HddsConfServlet.ResponseFormat.JSON); - - HttpServletRequest request = mock(HttpServletRequest.class); - for (Map.Entry entry : verifyMap.entrySet()) { - HddsConfServlet.ResponseFormat contenTypeActual = entry.getValue(); - when(request.getHeader(HttpHeaders.ACCEPT)) - .thenReturn(entry.getKey()); - assertEquals(contenTypeActual, - HddsConfServlet.parseAcceptHeader(request)); - } + TEST_FORMATS.put(HttpServletUtils.ResponseFormat.XML.toString(), "application/xml"); + TEST_FORMATS.put(HttpServletUtils.ResponseFormat.JSON.toString(), "application/json"); } @Test @@ -124,7 +106,7 @@ public void testGetPropertyWithCmd() throws Exception { @SuppressWarnings("unchecked") public void testWriteJson() throws Exception { StringWriter sw = new StringWriter(); - HddsConfServlet.writeResponse(getTestConf(), sw, HddsConfServlet.ResponseFormat.JSON, null); + HttpServletUtils.writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.JSON, null); String json = sw.toString(); boolean foundSetting = false; Object parsed = JSON.parse(json); @@ -145,7 +127,7 @@ public void testWriteJson() throws Exception { @Test public void testWriteXml() throws Exception { StringWriter sw = new StringWriter(); - HddsConfServlet.writeResponse(getTestConf(), sw, HddsConfServlet.ResponseFormat.XML, null); + HttpServletUtils.writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.XML, null); String xml = sw.toString(); DocumentBuilderFactory docBuilderFactory = diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java new file mode 100644 index 000000000000..1aa03eb53432 --- /dev/null +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -0,0 +1,33 @@ +package org.apache.hadoop.hdds.utils; + +import org.junit.jupiter.api.Test; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.HttpHeaders; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class TestHttpServletUtils { + + @Test + public void testParseHeaders() throws Exception { + HashMap verifyMap = new HashMap<>(); + verifyMap.put("text/plain", HttpServletUtils.ResponseFormat.XML); + verifyMap.put(null, HttpServletUtils.ResponseFormat.XML); + verifyMap.put("text/xml", HttpServletUtils.ResponseFormat.XML); + verifyMap.put("application/xml", HttpServletUtils.ResponseFormat.XML); + verifyMap.put("application/json", HttpServletUtils.ResponseFormat.JSON); + + HttpServletRequest request = mock(HttpServletRequest.class); + for (Map.Entry entry : verifyMap.entrySet()) { + HttpServletUtils.ResponseFormat contenTypeActual = entry.getValue(); + when(request.getHeader(HttpHeaders.ACCEPT)) + .thenReturn(entry.getKey()); + assertEquals(contenTypeActual, + HttpServletUtils.parseAcceptHeader(request)); + } + } +} From ea772b25564dfad2b055462e5677427e307f8b60 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 16:06:12 +0800 Subject: [PATCH 12/40] fix: fix testGetPropertyWithCmd --- .../org/apache/hadoop/hdds/conf/HddsConfServlet.java | 8 +++++++- .../org/apache/hadoop/hdds/utils/HttpServletUtils.java | 10 +++++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 2f47ef1fa837..a5178bebe67c 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -66,7 +66,13 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) return; } - HttpServletUtils.ResponseFormat format = HttpServletUtils.parseAcceptHeader(request); + HttpServletUtils.ResponseFormat format; + try { + format = HttpServletUtils.parseAcceptHeader(request); + } catch (IllegalArgumentException iae) { + format = HttpServletUtils.ResponseFormat.XML; + } + // TODO: set content type should be in HttpServletUtils switch (format) { case JSON: response.setContentType("application/json; charset=utf-8"); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 38b6ecf0ab4b..4c6e68dd0f1c 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -35,10 +35,14 @@ public class HttpServletUtils implements Serializable { * @see HttpHeaders#ACCEPT */ @VisibleForTesting - public static ResponseFormat parseAcceptHeader(HttpServletRequest request) { + public static ResponseFormat parseAcceptHeader(HttpServletRequest request) throws IllegalArgumentException { + // FIXME: parseAcceptHeader not related to ResponseFormat (name not match) + // this contains specic logic of HddsConfServlet: use xml as default String format = request.getHeader(HttpHeaders.ACCEPT); - MediaType mediaType = MediaType.valueOf(format); - return format != null && format.contains(ResponseFormat.JSON.getValue()) ? + if (format == null) { + throw new IllegalArgumentException("invalid accept-header format"); + } + return format.contains(ResponseFormat.JSON.getValue()) ? ResponseFormat.JSON : ResponseFormat.XML; } From 8c3e71c1b2b7fcd342b88774479ca6f33cd070e4 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 17:59:52 +0800 Subject: [PATCH 13/40] refactor(HttpServletUtils): pass response as params to writeErrorResponse --- .../hadoop/hdds/conf/HddsConfServlet.java | 2 +- .../hadoop/hdds/utils/HttpServletUtils.java | 11 ++++++----- .../hdds/utils/TestHttpServletUtils.java | 18 ++++++++++++++++++ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index a5178bebe67c..98bf3df78544 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -101,7 +101,7 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, } } catch (IllegalArgumentException iae) { response.setStatus(HttpServletResponse.SC_NOT_FOUND); - HttpServletUtils.writeErrorResponse(iae.getMessage(), format, out); + HttpServletUtils.writeErrorResponse(iae.getMessage(), format, response); } } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 4c6e68dd0f1c..1f5d0960ae18 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -68,19 +68,20 @@ public static void writeResponse(OzoneConfiguration conf, * * @param errorMessage the error message * @param format the response format - * @param out the writer + * @param response the response */ - public static void writeErrorResponse(String errorMessage, ResponseFormat format, Writer out) + public static void writeErrorResponse(String errorMessage, ResponseFormat format, HttpServletResponse response) throws IOException { - switch (format) { + PrintWriter writer = response.getWriter(); + switch (format) { case JSON: Map errorMap = new HashMap(); errorMap.put("error", errorMessage); - out.write(JsonUtils.toJsonString(errorMap)); + writer.write(JsonUtils.toJsonString(errorMap)); break; case XML: default: - writeXmlError(errorMessage, out); + writeXmlError(errorMessage, writer); break; } } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index 1aa03eb53432..39cf727ab767 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -30,4 +30,22 @@ public void testParseHeaders() throws Exception { HttpServletUtils.parseAcceptHeader(request)); } } + + @Test + public void testWriteErrorResponse_JSON() throws Exception { + StringWriter sw = new StringWriter(); + HttpServletResponse response = mock(HttpServletResponse.class); + when(response.getWriter()).thenReturn(new PrintWriter(sw)); + HttpServletUtils.writeErrorResponse("example error", HttpServletUtils.ResponseFormat.JSON, response); + assertEquals("{\"error\":\"example error\"}", sw.toString()); + } + + @Test + public void testWriteErrorResponse_XML() throws Exception { + StringWriter sw = new StringWriter(); + HttpServletResponse response = mock(HttpServletResponse.class); + when(response.getWriter()).thenReturn(new PrintWriter(sw)); + HttpServletUtils.writeErrorResponse("example error", HttpServletUtils.ResponseFormat.XML, response); + assertEquals("example error", sw.toString()); + } } From 91c115aafd0d3926a10028819182cb7f07347322 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 18:43:08 +0800 Subject: [PATCH 14/40] refactor: HddsConfServlet: remove unused error class --- .../org/apache/hadoop/hdds/conf/HddsConfServlet.java | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 98bf3df78544..5d3d9bbce914 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -105,18 +105,6 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat 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); - } - } - private void processConfigTagRequest(HttpServletRequest request, String cmd, Writer out) throws IOException { OzoneConfiguration config = getOzoneConfig(); From e0ddbb8d8f2597c4b0af498ddd432d18467a30f6 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 19:05:43 +0800 Subject: [PATCH 15/40] refactor(TestHttpServletUtils): introduce parameterized test to testParseHeaders --- .../hdds/utils/TestHttpServletUtils.java | 53 ++++++++++++------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index 39cf727ab767..c86c32077003 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -1,33 +1,46 @@ package org.apache.hadoop.hdds.utils; -import org.junit.jupiter.api.Test; -import javax.servlet.http.HttpServletRequest; -import javax.ws.rs.core.HttpHeaders; -import java.util.HashMap; -import java.util.Map; - -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import jakarta.annotation.Nullable; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.stream.Stream; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.HttpHeaders; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + class TestHttpServletUtils { - @Test - public void testParseHeaders() throws Exception { - HashMap verifyMap = new HashMap<>(); - verifyMap.put("text/plain", HttpServletUtils.ResponseFormat.XML); - verifyMap.put(null, HttpServletUtils.ResponseFormat.XML); - verifyMap.put("text/xml", HttpServletUtils.ResponseFormat.XML); - verifyMap.put("application/xml", HttpServletUtils.ResponseFormat.XML); - verifyMap.put("application/json", HttpServletUtils.ResponseFormat.JSON); + public static Stream provideParseHeadersTestCases() { + return Stream.of( + Arguments.of("text/plain", HttpServletUtils.ResponseFormat.XML, null), + // if content type is null, an IllegalArgumentException is expected to be thrown. + Arguments.of(null, null, IllegalArgumentException.class), + Arguments.of("text/xml", HttpServletUtils.ResponseFormat.XML, null), + Arguments.of("application/xml", HttpServletUtils.ResponseFormat.XML,null), + Arguments.of("application/json", HttpServletUtils.ResponseFormat.JSON, null) + ); + } + @ParameterizedTest + @MethodSource("provideParseHeadersTestCases") + public void testParseHeaders(@Nullable String contentType, HttpServletUtils.ResponseFormat expectResponseFormat, Class expectedException) { HttpServletRequest request = mock(HttpServletRequest.class); - for (Map.Entry entry : verifyMap.entrySet()) { - HttpServletUtils.ResponseFormat contenTypeActual = entry.getValue(); - when(request.getHeader(HttpHeaders.ACCEPT)) - .thenReturn(entry.getKey()); - assertEquals(contenTypeActual, + when(request.getHeader(HttpHeaders.ACCEPT)) + .thenReturn(contentType); + if (expectedException == null) { + assertEquals(expectResponseFormat, HttpServletUtils.parseAcceptHeader(request)); + } else { + assertThrows(expectedException, () -> HttpServletUtils.parseAcceptHeader(request)); } } From 49e706b26f58e7f18558df8eb8e47569fe348fbb Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 19:06:43 +0800 Subject: [PATCH 16/40] refactor(HttpServletUtils): add status code to writeErrorResponse --- .../apache/hadoop/hdds/utils/TestHttpServletUtils.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index c86c32077003..19947d488593 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -49,7 +49,8 @@ public void testWriteErrorResponse_JSON() throws Exception { StringWriter sw = new StringWriter(); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getWriter()).thenReturn(new PrintWriter(sw)); - HttpServletUtils.writeErrorResponse("example error", HttpServletUtils.ResponseFormat.JSON, response); + HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "example error", + HttpServletUtils.ResponseFormat.JSON, response); assertEquals("{\"error\":\"example error\"}", sw.toString()); } @@ -58,7 +59,9 @@ public void testWriteErrorResponse_XML() throws Exception { StringWriter sw = new StringWriter(); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getWriter()).thenReturn(new PrintWriter(sw)); - HttpServletUtils.writeErrorResponse("example error", HttpServletUtils.ResponseFormat.XML, response); - assertEquals("example error", sw.toString()); + HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "example error", + HttpServletUtils.ResponseFormat.XML, response); + assertEquals("example error", + sw.toString()); } } From 16ad0210dd1182b14090619bcfe898c9b8bb35ec Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 19:27:49 +0800 Subject: [PATCH 17/40] refactor(HttpServletUtils): refactor and rename to getResponseFormat --- .../hadoop/hdds/conf/HddsConfServlet.java | 8 +- .../hadoop/hdds/utils/HttpServletUtils.java | 148 +++++++++--------- .../hdds/utils/TestHttpServletUtils.java | 26 ++- 3 files changed, 89 insertions(+), 93 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 5d3d9bbce914..462155d7f10c 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -66,18 +66,14 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) return; } - HttpServletUtils.ResponseFormat format; - try { - format = HttpServletUtils.parseAcceptHeader(request); - } catch (IllegalArgumentException iae) { - format = HttpServletUtils.ResponseFormat.XML; - } + HttpServletUtils.ResponseFormat format = HttpServletUtils.getResponseFormat(request); // TODO: set content type should be in HttpServletUtils switch (format) { case JSON: response.setContentType("application/json; charset=utf-8"); break; case XML: + case UNSPECIFIED: default: response.setContentType("text/xml; charset=utf-8"); break; diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 1f5d0960ae18..f085cbce086d 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -8,8 +8,8 @@ import org.w3c.dom.Element; 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; @@ -20,31 +20,32 @@ import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; import java.io.IOException; +import java.io.PrintWriter; import java.io.Serializable; import java.io.Writer; import java.util.HashMap; import java.util.Map; public class HttpServletUtils implements Serializable { - /** - * Parse the Accept header to determine response format. - * - * @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 - */ - @VisibleForTesting - public static ResponseFormat parseAcceptHeader(HttpServletRequest request) throws IllegalArgumentException { - // FIXME: parseAcceptHeader not related to ResponseFormat (name not match) - // this contains specic logic of HddsConfServlet: use xml as default - String format = request.getHeader(HttpHeaders.ACCEPT); - if (format == null) { - throw new IllegalArgumentException("invalid accept-header format"); - } - return format.contains(ResponseFormat.JSON.getValue()) ? - ResponseFormat.JSON : ResponseFormat.XML; + /** + * 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 + */ + @VisibleForTesting + public static ResponseFormat getResponseFormat(HttpServletRequest request) throws IllegalArgumentException { + // FIXME: parseAcceptHeader not related to ResponseFormat (name not match) + // this contains specic logic of HddsConfServlet: use xml as default + String format = request.getHeader(HttpHeaders.ACCEPT); + if (format == null) { + return ResponseFormat.UNSPECIFIED; } + return format.contains(ResponseFormat.JSON.getValue()) ? + ResponseFormat.JSON : ResponseFormat.XML; + } /** * Guts of the servlet - extracted for easy testing. @@ -63,68 +64,71 @@ public static void writeResponse(OzoneConfiguration conf, } } - /** - * 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(String errorMessage, ResponseFormat format, HttpServletResponse response) - throws IOException { - PrintWriter writer = response.getWriter(); - switch (format) { - case JSON: - Map errorMap = new HashMap(); - errorMap.put("error", errorMessage); - writer.write(JsonUtils.toJsonString(errorMap)); - break; - case XML: - default: - writeXmlError(errorMessage, writer); - break; - } + /** + * 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 errorMap = new HashMap(); + errorMap.put("error", errorMessage); + writer.write(JsonUtils.toJsonString(errorMap)); + break; + case XML: + default: + writeXmlError(errorMessage, writer); + break; } + } - public static void writeXmlError(String errorMessage, Writer out) throws IOException { - try { - DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory(); - DocumentBuilder builder = factory.newDocumentBuilder(); - Document doc = builder.newDocument(); + private static void writeXmlError(String errorMessage, Writer out) throws IOException { + try { + DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory(); + DocumentBuilder builder = factory.newDocumentBuilder(); + Document doc = builder.newDocument(); - Element root = doc.createElement("error"); - root.setTextContent(errorMessage); - doc.appendChild(root); + 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"); + 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); - } + 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); } + } - public enum ResponseFormat { - JSON("json"), - XML("xml"); - private final String value; + public enum ResponseFormat { + UNSPECIFIED("unspecified"), + JSON("json"), + XML("xml"); + private final String value; - ResponseFormat(String value) { - this.value = value; - } + ResponseFormat(String value) { + this.value = value; + } - public String getValue() { - return value; - } + public String getValue() { + return value; + } - @Override - public String toString() { - return value; - } + @Override + public String toString() { + return value; } + } } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index 19947d488593..d3a63bf41455 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -19,29 +19,25 @@ class TestHttpServletUtils { - public static Stream provideParseHeadersTestCases() { + public static Stream provideGetResponseFormatTestCases() { return Stream.of( - Arguments.of("text/plain", HttpServletUtils.ResponseFormat.XML, null), - // if content type is null, an IllegalArgumentException is expected to be thrown. - Arguments.of(null, null, IllegalArgumentException.class), - Arguments.of("text/xml", HttpServletUtils.ResponseFormat.XML, null), - Arguments.of("application/xml", HttpServletUtils.ResponseFormat.XML,null), - Arguments.of("application/json", HttpServletUtils.ResponseFormat.JSON, null) + Arguments.of("text/plain", HttpServletUtils.ResponseFormat.XML), + Arguments.of(null, HttpServletUtils.ResponseFormat.UNSPECIFIED), + Arguments.of("text/xml", HttpServletUtils.ResponseFormat.XML), + Arguments.of("application/xml", HttpServletUtils.ResponseFormat.XML), + Arguments.of("application/json", HttpServletUtils.ResponseFormat.JSON) ); } @ParameterizedTest - @MethodSource("provideParseHeadersTestCases") - public void testParseHeaders(@Nullable String contentType, HttpServletUtils.ResponseFormat expectResponseFormat, Class expectedException) { + @MethodSource("provideGetResponseFormatTestCases") + public void testGetResponseFormat(@Nullable String contentType, + HttpServletUtils.ResponseFormat expectResponseFormat) { HttpServletRequest request = mock(HttpServletRequest.class); when(request.getHeader(HttpHeaders.ACCEPT)) .thenReturn(contentType); - if (expectedException == null) { - assertEquals(expectResponseFormat, - HttpServletUtils.parseAcceptHeader(request)); - } else { - assertThrows(expectedException, () -> HttpServletUtils.parseAcceptHeader(request)); - } + assertEquals(expectResponseFormat, + HttpServletUtils.getResponseFormat(request)); } @Test From 93d5a34546ed0902239295424edb96d81ab07631 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 19:28:12 +0800 Subject: [PATCH 18/40] fix writeErrorResponse --- .../main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 462155d7f10c..a17547a8ad05 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -96,8 +96,7 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, processConfigTagRequest(request, cmd, out); } } catch (IllegalArgumentException iae) { - response.setStatus(HttpServletResponse.SC_NOT_FOUND); - HttpServletUtils.writeErrorResponse(iae.getMessage(), format, response); + HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_NOT_FOUND, iae.getMessage(), format, response); } } From c3992da72999ca2f87fe7dcbce0a449ac8714475 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Wed, 15 Oct 2025 19:28:32 +0800 Subject: [PATCH 19/40] fix format --- .../hadoop/hdds/utils/HttpServletUtils.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index f085cbce086d..727ec870a6f1 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -47,22 +47,22 @@ public static ResponseFormat getResponseFormat(HttpServletRequest request) throw ResponseFormat.JSON : ResponseFormat.XML; } - /** - * Guts of the servlet - extracted for easy testing. - */ - public static void writeResponse(OzoneConfiguration conf, - Writer out, ResponseFormat format, String propertyName) - throws IOException, IllegalArgumentException { - switch (format) { - case JSON: - OzoneConfiguration.dumpConfiguration(conf, propertyName, out); - break; - case XML: - default: - conf.writeXml(propertyName, out); - break; - } + /** + * Guts of the servlet - extracted for easy testing. + */ + public static void writeResponse(OzoneConfiguration conf, + Writer out, ResponseFormat format, String propertyName) + throws IOException, IllegalArgumentException { + switch (format) { + case JSON: + OzoneConfiguration.dumpConfiguration(conf, propertyName, out); + break; + case XML: + default: + conf.writeXml(propertyName, out); + break; } + } /** * Write error response according to the specified format. From 8f57f9f2329cf7dfdc10f936ff84c7ee3b8fb9aa Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 07:42:09 +0800 Subject: [PATCH 20/40] refactor(HttpServletUtils): revert writeResponse method --- .../hadoop/hdds/conf/HddsConfServlet.java | 15 ++++++++++++++- .../hadoop/hdds/utils/HttpServletUtils.java | 17 ----------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index a17547a8ad05..cbece2775717 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -91,7 +91,7 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, throws IOException { try { if (cmd == null) { - HttpServletUtils.writeResponse(getConfFromContext(), out, format, name); + dumpConfiguration(format, out, name); } else { processConfigTagRequest(request, cmd, out); } @@ -100,6 +100,19 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, } } + private void dumpConfiguration(HttpServletUtils.ResponseFormat format, Writer out, String name) throws IOException { + OzoneConfiguration conf = getConfFromContext(); + switch (format) { + case JSON: + OzoneConfiguration.dumpConfiguration(conf, name, out); + break; + case XML: + default: + conf.writeXml(name, out); + break; + } + } + private void processConfigTagRequest(HttpServletRequest request, String cmd, Writer out) throws IOException { OzoneConfiguration config = getOzoneConfig(); diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 727ec870a6f1..1609bd15bfc5 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -47,23 +47,6 @@ public static ResponseFormat getResponseFormat(HttpServletRequest request) throw ResponseFormat.JSON : ResponseFormat.XML; } - /** - * Guts of the servlet - extracted for easy testing. - */ - public static void writeResponse(OzoneConfiguration conf, - Writer out, ResponseFormat format, String propertyName) - throws IOException, IllegalArgumentException { - switch (format) { - case JSON: - OzoneConfiguration.dumpConfiguration(conf, propertyName, out); - break; - case XML: - default: - conf.writeXml(propertyName, out); - break; - } - } - /** * Write error response according to the specified format. * From 60e1cc5a97ce50ff58c1f4298f564abe99aefe46 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 11:33:26 +0800 Subject: [PATCH 21/40] revert writeResponse --- .../hadoop/hdds/conf/HddsConfServlet.java | 20 ++++++++++++------- .../hadoop/hdds/conf/TestHddsConfServlet.java | 6 ++++-- .../hdds/utils/TestHttpServletUtils.java | 1 - 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index cbece2775717..76eb7494f66b 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -67,13 +67,15 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) } HttpServletUtils.ResponseFormat format = HttpServletUtils.getResponseFormat(request); - // TODO: set content type should be in HttpServletUtils + if (format == HttpServletUtils.ResponseFormat.UNSPECIFIED) { + format = HttpServletUtils.ResponseFormat.XML; + } + switch (format) { case JSON: response.setContentType("application/json; charset=utf-8"); break; case XML: - case UNSPECIFIED: default: response.setContentType("text/xml; charset=utf-8"); break; @@ -91,7 +93,7 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, throws IOException { try { if (cmd == null) { - dumpConfiguration(format, out, name); + writeResponse(getConfFromContext(), out, format, name); } else { processConfigTagRequest(request, cmd, out); } @@ -100,15 +102,19 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, } } - private void dumpConfiguration(HttpServletUtils.ResponseFormat format, Writer out, String name) throws IOException { - OzoneConfiguration conf = getConfFromContext(); + /** + * Guts of the servlet - extracted for easy testing. + */ + static void writeResponse(OzoneConfiguration conf, Writer out, HttpServletUtils.ResponseFormat format, + String propertyName) + throws IOException, IllegalArgumentException { switch (format) { case JSON: - OzoneConfiguration.dumpConfiguration(conf, name, out); + OzoneConfiguration.dumpConfiguration(conf, propertyName, out); break; case XML: default: - conf.writeXml(name, out); + conf.writeXml(propertyName, out); break; } } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index 6f2bce81b8d2..c43d64f1ef2c 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -17,6 +17,8 @@ package org.apache.hadoop.hdds.conf; +import static org.apache.hadoop.conf.Configuration.dumpConfiguration; +import static org.apache.hadoop.hdds.conf.HddsConfServlet.writeResponse; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -106,7 +108,7 @@ public void testGetPropertyWithCmd() throws Exception { @SuppressWarnings("unchecked") public void testWriteJson() throws Exception { StringWriter sw = new StringWriter(); - HttpServletUtils.writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.JSON, null); + writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.JSON, null); String json = sw.toString(); boolean foundSetting = false; Object parsed = JSON.parse(json); @@ -127,7 +129,7 @@ public void testWriteJson() throws Exception { @Test public void testWriteXml() throws Exception { StringWriter sw = new StringWriter(); - HttpServletUtils.writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.XML, null); + writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.XML, null); String xml = sw.toString(); DocumentBuilderFactory docBuilderFactory = diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index d3a63bf41455..c00da0f524f2 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -1,7 +1,6 @@ package org.apache.hadoop.hdds.utils; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; From 6734b23c5581c72d9266142e44d868bf73fa936d Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 11:34:39 +0800 Subject: [PATCH 22/40] docs: remove unused comment --- .../java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 1609bd15bfc5..3bf9cf02f4ea 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -1,7 +1,6 @@ package org.apache.hadoop.hdds.utils; import com.google.common.annotations.VisibleForTesting; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.server.JsonUtils; import org.apache.hadoop.util.XMLUtils; import org.w3c.dom.Document; @@ -37,8 +36,6 @@ public class HttpServletUtils implements Serializable { */ @VisibleForTesting public static ResponseFormat getResponseFormat(HttpServletRequest request) throws IllegalArgumentException { - // FIXME: parseAcceptHeader not related to ResponseFormat (name not match) - // this contains specic logic of HddsConfServlet: use xml as default String format = request.getHeader(HttpHeaders.ACCEPT); if (format == null) { return ResponseFormat.UNSPECIFIED; From 52c89b46f5f8e7904aec6e7bf43668312a30b35d Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 11:50:01 +0800 Subject: [PATCH 23/40] refactor(HttpServletUtils): introduce ResponseFormat.getContentType() --- .../hadoop/hdds/conf/HddsConfServlet.java | 12 +----- .../hadoop/hdds/utils/HttpServletUtils.java | 39 +++++++++++++------ 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 76eb7494f66b..a779b5dcc4d1 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -68,18 +68,11 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) HttpServletUtils.ResponseFormat format = HttpServletUtils.getResponseFormat(request); if (format == HttpServletUtils.ResponseFormat.UNSPECIFIED) { + // use XML as default response format format = HttpServletUtils.ResponseFormat.XML; } - 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()); String name = request.getParameter("name"); Writer out = response.getWriter(); @@ -145,7 +138,6 @@ private void processConfigTagRequest(HttpServletRequest request, String cmd, Wri default: throw new IllegalArgumentException(cmd + " is not a valid command."); } - } private static OzoneConfiguration getOzoneConfig() { diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 3bf9cf02f4ea..c94259100959 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -1,14 +1,16 @@ package org.apache.hadoop.hdds.utils; import com.google.common.annotations.VisibleForTesting; -import org.apache.hadoop.hdds.server.JsonUtils; -import org.apache.hadoop.util.XMLUtils; -import org.w3c.dom.Document; -import org.w3c.dom.Element; - +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; @@ -18,12 +20,10 @@ import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; -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 org.apache.hadoop.hdds.server.JsonUtils; +import org.apache.hadoop.util.XMLUtils; +import org.w3c.dom.Document; +import org.w3c.dom.Element; public class HttpServletUtils implements Serializable { /** @@ -110,5 +110,22 @@ public String getValue() { 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; + } + } } } From ff49170554fe3b7d80a86136cb57d382e81fcc70 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 11:50:11 +0800 Subject: [PATCH 24/40] minor tweaks --- .../java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index c00da0f524f2..1e71acad0ffa 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -17,7 +17,6 @@ import org.junit.jupiter.params.provider.MethodSource; class TestHttpServletUtils { - public static Stream provideGetResponseFormatTestCases() { return Stream.of( Arguments.of("text/plain", HttpServletUtils.ResponseFormat.XML), From 1006c1cc4013846653fb9c1a92c5edfff874fbde Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 11:57:39 +0800 Subject: [PATCH 25/40] minor: add license --- .../hadoop/hdds/utils/HttpServletUtils.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index c94259100959..ebbcfca2d535 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -1,3 +1,20 @@ +/* + * 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 com.google.common.annotations.VisibleForTesting; From e72b084c8515520958a5f7cc2478bf179e9dfce0 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 12:03:10 +0800 Subject: [PATCH 26/40] minor: add license --- .../hadoop/hdds/utils/TestHttpServletUtils.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index 1e71acad0ffa..185d23f28027 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -1,3 +1,20 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; From 6e0b35a64d9dbd55d78055656fdce2d49a68f6ab Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Thu, 16 Oct 2025 12:09:13 +0800 Subject: [PATCH 27/40] minor: fix checkstyle --- .../hadoop/hdds/utils/HttpServletUtils.java | 20 ++++++++++++++++++- .../hadoop/hdds/conf/TestHddsConfServlet.java | 1 - .../hdds/utils/TestHttpServletUtils.java | 4 ++-- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index ebbcfca2d535..ba1c8170b573 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -42,7 +42,16 @@ import org.w3c.dom.Document; import org.w3c.dom.Element; -public class HttpServletUtils implements Serializable { +/** + * Utility class for HTTP servlet operations. + * Provides methods for parsing request headers and writing responses. + */ +public final class HttpServletUtils implements Serializable { + + private HttpServletUtils() { + // Utility class, prevent instantiation + } + /** * Get the response format from request header. * @@ -109,6 +118,10 @@ private static void writeXmlError(String errorMessage, Writer out) throws IOExce } } + /** + * Response format enumeration for HTTP responses. + * Supports JSON, XML, and UNSPECIFIED formats. + */ public enum ResponseFormat { UNSPECIFIED("unspecified"), JSON("json"), @@ -119,6 +132,11 @@ public enum ResponseFormat { 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; } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index c43d64f1ef2c..5631cdc7af6f 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -17,7 +17,6 @@ package org.apache.hadoop.hdds.conf; -import static org.apache.hadoop.conf.Configuration.dumpConfiguration; import static org.apache.hadoop.hdds.conf.HddsConfServlet.writeResponse; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index 185d23f28027..a1c731726ad3 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -56,7 +56,7 @@ public void testGetResponseFormat(@Nullable String contentType, } @Test - public void testWriteErrorResponse_JSON() throws Exception { + public void testWriteErrorResponseJson() throws Exception { StringWriter sw = new StringWriter(); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getWriter()).thenReturn(new PrintWriter(sw)); @@ -66,7 +66,7 @@ public void testWriteErrorResponse_JSON() throws Exception { } @Test - public void testWriteErrorResponse_XML() throws Exception { + public void testWriteErrorResponseXml() throws Exception { StringWriter sw = new StringWriter(); HttpServletResponse response = mock(HttpServletResponse.class); when(response.getWriter()).thenReturn(new PrintWriter(sw)); From 8f357fd6872159090db48a89f126f8678f5d9bc9 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 20 Oct 2025 20:39:16 +0800 Subject: [PATCH 28/40] refactor(HttpServletUtils): extract DOCUMENT_BUILDER_FACTORY --- .../org/apache/hadoop/hdds/utils/HttpServletUtils.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index ba1c8170b573..d5a9c0d40292 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -39,6 +39,8 @@ 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; @@ -48,6 +50,9 @@ */ public final class HttpServletUtils implements Serializable { + private static final CheckedSupplier DOCUMENT_BUILDER_FACTORY = + MemoizedCheckedSupplier.valueOf(XMLUtils::newSecureDocumentBuilderFactory); + private HttpServletUtils() { // Utility class, prevent instantiation } @@ -97,8 +102,7 @@ public static void writeErrorResponse(int status, String errorMessage, ResponseF private static void writeXmlError(String errorMessage, Writer out) throws IOException { try { - DocumentBuilderFactory factory = XMLUtils.newSecureDocumentBuilderFactory(); - DocumentBuilder builder = factory.newDocumentBuilder(); + DocumentBuilder builder = DOCUMENT_BUILDER_FACTORY.get().newDocumentBuilder(); Document doc = builder.newDocument(); Element root = doc.createElement("error"); From dd6efd092dcdc7610747a07a8859c2e06e7b5178 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Tue, 21 Oct 2025 07:31:05 +0800 Subject: [PATCH 29/40] Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com> --- .../main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index a779b5dcc4d1..005882a9f403 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -82,7 +82,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) } private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, HttpServletRequest request, - HttpServletResponse response, Writer out, String name) + HttpServletResponse response, Writer out, String name) throws IOException { try { if (cmd == null) { From 092e5fac0bd6ca56a610bb5d413356f79f153dbe Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Tue, 21 Oct 2025 07:31:24 +0800 Subject: [PATCH 30/40] Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com> --- .../main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 005882a9f403..507bd5589431 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -99,7 +99,7 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, * Guts of the servlet - extracted for easy testing. */ static void writeResponse(OzoneConfiguration conf, Writer out, HttpServletUtils.ResponseFormat format, - String propertyName) + String propertyName) throws IOException, IllegalArgumentException { switch (format) { case JSON: From 5c6aa18f79d025786d92290116b28420550ca5d4 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Tue, 21 Oct 2025 07:31:37 +0800 Subject: [PATCH 31/40] Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com> --- .../java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index d5a9c0d40292..d4f36c5b171d 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -83,7 +83,7 @@ public static ResponseFormat getResponseFormat(HttpServletRequest request) throw * @param response the response */ public static void writeErrorResponse(int status, String errorMessage, ResponseFormat format, - HttpServletResponse response) + HttpServletResponse response) throws IOException { response.setStatus(status); PrintWriter writer = response.getWriter(); From d65d5fc5dcb8651b850e1291069039b66c647a2b Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Tue, 21 Oct 2025 07:31:47 +0800 Subject: [PATCH 32/40] Update hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java fix indent Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com> --- .../java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java index a1c731726ad3..46de47742f44 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestHttpServletUtils.java @@ -47,7 +47,7 @@ public static Stream provideGetResponseFormatTestCases() { @ParameterizedTest @MethodSource("provideGetResponseFormatTestCases") public void testGetResponseFormat(@Nullable String contentType, - HttpServletUtils.ResponseFormat expectResponseFormat) { + HttpServletUtils.ResponseFormat expectResponseFormat) { HttpServletRequest request = mock(HttpServletRequest.class); when(request.getHeader(HttpHeaders.ACCEPT)) .thenReturn(contentType); From ae742f12840ab26f4d8a62f13001e98e4fb51318 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Sat, 25 Oct 2025 21:01:27 +0800 Subject: [PATCH 33/40] refactor(HddsConfServlet): explicitly throw BadFormatException --- .../java/org/apache/hadoop/hdds/conf/HddsConfServlet.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 507bd5589431..dc8c7addbb73 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -26,6 +26,7 @@ import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +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; @@ -92,6 +93,8 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, } } catch (IllegalArgumentException iae) { HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_NOT_FOUND, iae.getMessage(), format, response); + } catch (BadFormatException bfe){ + HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_BAD_REQUEST, bfe.getMessage(), format, response); } } @@ -100,15 +103,16 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, */ static void writeResponse(OzoneConfiguration conf, Writer out, HttpServletUtils.ResponseFormat format, String propertyName) - throws IOException, IllegalArgumentException { + throws IOException, IllegalArgumentException, BadFormatException { switch (format) { case JSON: OzoneConfiguration.dumpConfiguration(conf, propertyName, out); break; case XML: - default: conf.writeXml(propertyName, out); break; + default: + throw new BadFormatException("Bad format: " + format); } } From 656fb87530b8cd630ff44e87352c862a5e281d2c Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Sat, 25 Oct 2025 22:21:50 +0800 Subject: [PATCH 34/40] refactor(HddsConfServlet): throw internal server error for BadFormatException --- .../apache/hadoop/hdds/conf/HddsConfServlet.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index dc8c7addbb73..5e2879e165b5 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -32,6 +32,8 @@ 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. @@ -39,6 +41,8 @@ @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; @@ -83,7 +87,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) } private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, HttpServletRequest request, - HttpServletResponse response, Writer out, String name) + HttpServletResponse response, Writer out, String name) throws IOException { try { if (cmd == null) { @@ -93,8 +97,10 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, } } catch (IllegalArgumentException iae) { HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_NOT_FOUND, iae.getMessage(), format, response); - } catch (BadFormatException bfe){ - HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_BAD_REQUEST, bfe.getMessage(), format, response); + } catch (BadFormatException bfe) { + LOG.error("format should be either JSON or XML, got {}", format, bfe); + HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + "Caught an exception while processing HddsConfServlet request", format, response); } } @@ -102,7 +108,7 @@ private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, * Guts of the servlet - extracted for easy testing. */ static void writeResponse(OzoneConfiguration conf, Writer out, HttpServletUtils.ResponseFormat format, - String propertyName) + String propertyName) throws IOException, IllegalArgumentException, BadFormatException { switch (format) { case JSON: From e928eee766dfc592c3252b15efe933b3ebbe96e5 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 27 Oct 2025 08:36:14 +0800 Subject: [PATCH 35/40] refactor(HttpServletUtils): add writeResponse method --- .../hadoop/hdds/conf/HddsConfServlet.java | 48 +++++++------------ .../hadoop/hdds/utils/HttpServletUtils.java | 43 +++++++++++++++++ .../hadoop/hdds/conf/TestHddsConfServlet.java | 21 ++++++-- 3 files changed, 79 insertions(+), 33 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java index 5e2879e165b5..066735a02582 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/HddsConfServlet.java @@ -77,53 +77,41 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) format = HttpServletUtils.ResponseFormat.XML; } - response.setContentType(format.getContentType()); - String name = request.getParameter("name"); - Writer out = response.getWriter(); String cmd = request.getParameter(COMMAND); - processCommand(cmd, format, request, response, out, name); + processCommand(cmd, format, request, response, name); } private void processCommand(String cmd, HttpServletUtils.ResponseFormat format, HttpServletRequest request, - HttpServletResponse response, Writer out, String name) + 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 (IllegalArgumentException iae) { HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_NOT_FOUND, iae.getMessage(), format, response); - } catch (BadFormatException bfe) { - LOG.error("format should be either JSON or XML, got {}", format, bfe); - HttpServletUtils.writeErrorResponse(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, - "Caught an exception while processing HddsConfServlet request", format, response); - } - } - - /** - * Guts of the servlet - extracted for easy testing. - */ - static void writeResponse(OzoneConfiguration conf, Writer out, HttpServletUtils.ResponseFormat format, - String propertyName) - throws IOException, IllegalArgumentException, BadFormatException { - switch (format) { - case JSON: - OzoneConfiguration.dumpConfiguration(conf, propertyName, out); - break; - case XML: - conf.writeXml(propertyName, out); - break; - default: - throw new BadFormatException("Bad format: " + format); } } - 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": diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index d4f36c5b171d..7af8e24fa869 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -122,6 +122,49 @@ private static void writeXmlError(String errorMessage, Writer out) throws IOExce } } + /** + * 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 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 void writeResponse(HttpServletResponse response, ResponseFormat format, + CheckedConsumer contentWriter, + Class 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 the type of the input to the operation + */ + @FunctionalInterface + public interface CheckedConsumer { + void accept(T t) throws Exception; + } + /** * Response format enumeration for HTTP responses. * Supports JSON, XML, and UNSPECIFIED formats. diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java index 5631cdc7af6f..671b3707a332 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestHddsConfServlet.java @@ -17,7 +17,6 @@ package org.apache.hadoop.hdds.conf; -import static org.apache.hadoop.hdds.conf.HddsConfServlet.writeResponse; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -107,7 +106,15 @@ public void testGetPropertyWithCmd() throws Exception { @SuppressWarnings("unchecked") public void testWriteJson() throws Exception { StringWriter sw = new StringWriter(); - writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.JSON, null); + PrintWriter pw = new PrintWriter(sw); + HttpServletResponse response = mock(HttpServletResponse.class); + when(response.getWriter()).thenReturn(pw); + + OzoneConfiguration conf = getTestConf(); + HttpServletUtils.writeResponse(response, HttpServletUtils.ResponseFormat.JSON, (out) -> { + OzoneConfiguration.dumpConfiguration(conf, null, out); + }, IllegalArgumentException.class); + String json = sw.toString(); boolean foundSetting = false; Object parsed = JSON.parse(json); @@ -128,7 +135,15 @@ public void testWriteJson() throws Exception { @Test public void testWriteXml() throws Exception { StringWriter sw = new StringWriter(); - writeResponse(getTestConf(), sw, HttpServletUtils.ResponseFormat.XML, null); + PrintWriter pw = new PrintWriter(sw); + HttpServletResponse response = mock(HttpServletResponse.class); + when(response.getWriter()).thenReturn(pw); + + OzoneConfiguration conf = getTestConf(); + HttpServletUtils.writeResponse(response, HttpServletUtils.ResponseFormat.XML, (out) -> { + conf.writeXml(null, out); + }, IllegalArgumentException.class); + String xml = sw.toString(); DocumentBuilderFactory docBuilderFactory = From c7c8567ad2f037b53e9eb0524e6fea559f294ebb Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 27 Oct 2025 08:45:52 +0800 Subject: [PATCH 36/40] remove VisibleForTesting --- .../main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 7af8e24fa869..3db29ab710d6 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -65,7 +65,6 @@ private HttpServletUtils() { * otherwise {@link ResponseFormat#XML} (default for backwards compatibility) * @see HttpHeaders#ACCEPT */ - @VisibleForTesting public static ResponseFormat getResponseFormat(HttpServletRequest request) throws IllegalArgumentException { String format = request.getHeader(HttpHeaders.ACCEPT); if (format == null) { From d42479cbc8cfb0706d4c63d735e0d971e1697be6 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 27 Oct 2025 08:46:35 +0800 Subject: [PATCH 37/40] minor: manually tweak indentation of `writeResponse` --- .../java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 3db29ab710d6..40591fde0aa8 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -134,9 +134,7 @@ private static void writeXmlError(String errorMessage, Writer out) throws IOExce * @throws E if contentWriter throws an exception of type E */ public static void writeResponse(HttpServletResponse response, ResponseFormat format, - CheckedConsumer contentWriter, - Class exceptionClass) - throws IOException, E { + CheckedConsumer contentWriter, Class exceptionClass) throws IOException, E { response.setContentType(format.getContentType()); Writer out = response.getWriter(); try { From d7e17f5e290e7bf9f72a357e848b80f34fc0f7a3 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 27 Oct 2025 08:51:34 +0800 Subject: [PATCH 38/40] remove unused import --- .../main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 40591fde0aa8..6c2a4b6b25b9 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -17,7 +17,6 @@ package org.apache.hadoop.hdds.utils; -import com.google.common.annotations.VisibleForTesting; import java.io.IOException; import java.io.PrintWriter; import java.io.Serializable; From 20a7bd3b8b7418fc77bdb0e6e5158924970d4103 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 27 Oct 2025 08:52:44 +0800 Subject: [PATCH 39/40] refactor(HttpServletUtils.writeErrorResponse): explicitly throw exception for unspecified format --- .../java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index 6c2a4b6b25b9..b322bbf96c43 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -87,14 +87,15 @@ public static void writeErrorResponse(int status, String errorMessage, ResponseF PrintWriter writer = response.getWriter(); switch (format) { case JSON: - Map errorMap = new HashMap(); + Map errorMap = new HashMap<>(); errorMap.put("error", errorMessage); writer.write(JsonUtils.toJsonString(errorMap)); break; case XML: - default: writeXmlError(errorMessage, writer); break; + default: + throw new IOException("Unsupported response format for error response: " + format); } } From 1443f96f582791f31eb8a449d53af1f37a7539c6 Mon Sep 17 00:00:00 2001 From: Eric Chang Date: Mon, 27 Oct 2025 09:04:26 +0800 Subject: [PATCH 40/40] refactor(HttpServletUtils.writeErrorResponse): throw IOException, which cause is IllegalArgumentException --- .../java/org/apache/hadoop/hdds/utils/HttpServletUtils.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java index b322bbf96c43..682777e1f20e 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HttpServletUtils.java @@ -95,7 +95,8 @@ public static void writeErrorResponse(int status, String errorMessage, ResponseF writeXmlError(errorMessage, writer); break; default: - throw new IOException("Unsupported response format for error response: " + format); + throw new IOException("Unsupported response format for error response: " + format, + new IllegalArgumentException("Bad format: " + format)); } }