From 8d65bf75f3fc3ce3ce32317205c8d3f7356280fe Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 1 Sep 2020 07:53:21 +0200 Subject: [PATCH] refactor I18nServlet to avoid stacktrace logging on unknown language --- .../java/sonia/scm/web/i18n/I18nServlet.java | 108 +++++++++--------- .../sonia/scm/web/i18n/I18nServletTest.java | 26 ++--- 2 files changed, 65 insertions(+), 69 deletions(-) diff --git a/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java b/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java index 903250c68b..4671915a9d 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java +++ b/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java @@ -33,7 +33,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.inject.Singleton; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import sonia.scm.NotFoundException; import sonia.scm.SCMContext; import sonia.scm.Stage; import sonia.scm.cache.Cache; @@ -52,11 +51,6 @@ import java.net.URL; import java.util.Enumeration; import java.util.Iterator; import java.util.Optional; -import java.util.function.BiConsumer; -import java.util.function.Function; - -import static sonia.scm.ContextEntry.ContextBuilder.entity; -import static sonia.scm.NotFoundException.notFound; /** @@ -89,45 +83,54 @@ public class I18nServlet extends HttpServlet { cache.clear(); } - private JsonNode getCollectedJson(String path, - Function> jsonFileProvider, - BiConsumer createdJsonFileConsumer) { - return Optional.ofNullable(jsonFileProvider.apply(path) - .orElseGet(() -> { - Optional createdFile = collectJsonFile(path); - createdFile.ifPresent(map -> createdJsonFileConsumer.accept(path, map)); - return createdFile.orElse(null); - } - )).orElseThrow(() -> notFound(entity("jsonprovider", path))); - } - @VisibleForTesting @Override - protected void doGet(HttpServletRequest req, HttpServletResponse response) { + protected void doGet(HttpServletRequest request, HttpServletResponse response) { + String path = request.getServletPath(); + try { + Optional json = findJson(path); + if (json.isPresent()) { + write(response, json.get()); + } else { + LOG.debug("could not find translation at {}", path); + response.setStatus(HttpServletResponse.SC_NOT_FOUND); + } + } catch (IOException ex) { + LOG.error("Error on getting the translation of the plugins", ex); + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } + } + + private void write(HttpServletResponse response, JsonNode jsonNode) throws IOException { response.setCharacterEncoding("UTF-8"); response.setContentType("application/json"); response.setHeader("Cache-Control", "no-cache"); - try (PrintWriter out = response.getWriter()) { - String path = req.getServletPath(); - Function> jsonFileProvider = usedPath -> Optional.empty(); - BiConsumer createdJsonFileConsumer = (usedPath, jsonNode) -> LOG.debug("A json File is created from the path {}", usedPath); - if (isProductionStage()) { - LOG.debug("In Production Stage get the plugin translations from the cache"); - jsonFileProvider = usedPath -> Optional.ofNullable(cache.get(usedPath)); - createdJsonFileConsumer = createdJsonFileConsumer - .andThen((usedPath, jsonNode) -> LOG.debug("Put the created json File in the cache with the key {}", usedPath)) - .andThen(cache::put); - } - objectMapper.writeValue(out, getCollectedJson(path, jsonFileProvider, createdJsonFileConsumer)); - } catch (IOException e) { - LOG.error("Error on getting the translation of the plugins", e); - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); - } catch (NotFoundException e) { - LOG.error("Plugin translations are not found", e); - response.setStatus(HttpServletResponse.SC_NOT_FOUND); + + try (PrintWriter writer = response.getWriter()) { + objectMapper.writeValue(writer, jsonNode); } } + public Optional findJson(String path) throws IOException { + if (isProductionStage()) { + return findJsonCached(path); + } + return collectJsonFile(path); + } + + private Optional findJsonCached(String path) throws IOException { + JsonNode jsonNode = cache.get(path); + if (jsonNode != null) { + LOG.debug("return json node from cache for path {}", path); + return Optional.of(jsonNode); + } + + LOG.debug("collect json for path {}", path); + Optional collected = collectJsonFile(path); + collected.ifPresent(node -> cache.put(path, node)); + return collected; + } + @VisibleForTesting protected boolean isProductionStage() { return SCMContext.getContext().getStage() == Stage.PRODUCTION; @@ -140,46 +143,39 @@ public class I18nServlet extends HttpServlet { * @return a collected Json File as JsonNode from the given path from all plugins in the class path */ @VisibleForTesting - protected Optional collectJsonFile(String path) { + protected Optional collectJsonFile(String path) throws IOException { LOG.debug("Collect plugin translations from path {} for every plugin", path); JsonNode mergedJsonNode = null; - try { - Enumeration resources = classLoader.getResources(path.replaceFirst("/", "")); - while (resources.hasMoreElements()) { - URL url = resources.nextElement(); - JsonNode jsonNode = objectMapper.readTree(url); - if (mergedJsonNode != null) { - merge(mergedJsonNode, jsonNode); - } else { - mergedJsonNode = jsonNode; - } + Enumeration resources = classLoader.getResources(path.replaceFirst("/", "")); + while (resources.hasMoreElements()) { + URL url = resources.nextElement(); + JsonNode jsonNode = objectMapper.readTree(url); + if (mergedJsonNode != null) { + merge(mergedJsonNode, jsonNode); + } else { + mergedJsonNode = jsonNode; } - } catch (IOException e) { - LOG.error("Error on loading sources from {}", path, e); - return Optional.empty(); } + return Optional.ofNullable(mergedJsonNode); } /** * Merge the updateNode into the mainNode and return it. - * + *

* This is not a deep merge. * - * @param mainNode the main node + * @param mainNode the main node * @param updateNode the update node * @return the merged mainNode */ @VisibleForTesting protected JsonNode merge(JsonNode mainNode, JsonNode updateNode) { Iterator fieldNames = updateNode.fieldNames(); - while (fieldNames.hasNext()) { - String fieldName = fieldNames.next(); JsonNode jsonNode = mainNode.get(fieldName); - if (jsonNode != null) { mergeNode(updateNode, fieldName, jsonNode); } else { diff --git a/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java b/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java index 4770a54d91..32c3fcc772 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.web.i18n; import com.fasterxml.jackson.databind.JsonNode; @@ -40,7 +40,6 @@ import org.mockito.MockSettings; import org.mockito.internal.creation.MockSettingsImpl; import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.MockitoJUnitRunner; -import sonia.scm.lifecycle.RestartEvent; import sonia.scm.cache.Cache; import sonia.scm.cache.CacheManager; import sonia.scm.event.ScmEventBus; @@ -87,7 +86,7 @@ public class I18nServletTest { "}", "}" ); - private static String SVN_PLUGIN_JSON = json( + private static final String SVN_PLUGIN_JSON = json( "{", "'scm-svn-plugin': {", "'information': {", @@ -116,11 +115,11 @@ public class I18nServletTest { private I18nServlet servlet; @Mock - private Cache cache; + private Cache cache; + private Enumeration resources; @Before - @SuppressWarnings("unchecked") public void init() throws IOException { resources = Collections.enumeration(Lists.newArrayList( createFileFromString(SVN_PLUGIN_JSON).toURI().toURL(), @@ -128,7 +127,7 @@ public class I18nServletTest { createFileFromString(HG_PLUGIN_JSON).toURI().toURL() )); when(pluginLoader.getUberClassLoader()).thenReturn(classLoader); - when(cacheManager.getCache(I18nServlet.CACHE_NAME)).thenReturn(cache); + when(cacheManager.getCache(I18nServlet.CACHE_NAME)).thenReturn(cache); MockSettings settings = new MockSettingsImpl<>(); settings.useConstructor(pluginLoader, cacheManager); settings.defaultAnswer(InvocationOnMock::callRealMethod); @@ -145,7 +144,6 @@ public class I18nServletTest { } @Test - @SuppressWarnings("unchecked") public void shouldFailWith404OnMissingResources() throws IOException { String path = "/locales/de/plugins.json"; HttpServletRequest request = mock(HttpServletRequest.class); @@ -153,7 +151,9 @@ public class I18nServletTest { PrintWriter writer = mock(PrintWriter.class); when(response.getWriter()).thenReturn(writer); when(request.getServletPath()).thenReturn(path); - when(classLoader.getResources("locales/de/plugins.json")).thenThrow(IOException.class); + when(classLoader.getResources("locales/de/plugins.json")).thenReturn( + I18nServlet.class.getClassLoader().getResources("something/not/available") + ); servlet.doGet(request, response); @@ -161,9 +161,10 @@ public class I18nServletTest { } @Test - @SuppressWarnings("unchecked") public void shouldFailWith500OnIOException() throws IOException { + when(servlet.isProductionStage()).thenReturn(false); HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getServletPath()).thenReturn("/locales/de/plugins.json"); HttpServletResponse response = mock(HttpServletResponse.class); doThrow(IOException.class).when(response).getWriter(); @@ -173,7 +174,7 @@ public class I18nServletTest { } @Test - @SuppressWarnings("unchecked") + @SuppressWarnings("UnstableApiUsage") public void inDevelopmentStageShouldNotUseCache() throws IOException { String path = "/locales/de/plugins.json"; when(servlet.isProductionStage()).thenReturn(false); @@ -193,7 +194,7 @@ public class I18nServletTest { } @Test - @SuppressWarnings("unchecked") + @SuppressWarnings("UnstableApiUsage") public void inProductionStageShouldUseCache() throws IOException { String path = "/locales/de/plugins.json"; when(servlet.isProductionStage()).thenReturn(true); @@ -216,7 +217,7 @@ public class I18nServletTest { } @Test - @SuppressWarnings("unchecked") + @SuppressWarnings("UnstableApiUsage") public void inProductionStageShouldGetFromCache() throws IOException { String path = "/locales/de/plugins.json"; when(servlet.isProductionStage()).thenReturn(true); @@ -245,7 +246,6 @@ public class I18nServletTest { } @Test - @SuppressWarnings("unchecked") public void shouldCollectJsonFile() throws IOException { String path = "locales/de/plugins.json"; when(classLoader.getResources(path)).thenReturn(resources);