Vojtech Szocs has posted comments on this change.
Change subject: core: Locale docs servlet.
......................................................................
Patch Set 14: (35 inline comments)
....................................................
File backend/manager/modules/root/pom.xml
Line 21
Line 22
Line 23
Line 24
Line 25
Why remove provided scope for servlet API?
JBoss has "javax.servlet.api" module that provides the servlet API JAR at
runtime to all WAR deployments.
Line 66: <artifactId>commons-lang</artifactId>
Line 67: </dependency>
Line 68: <dependency>
Line 69: <groupId>org.mockito</groupId>
Line 70: <artifactId>mockito-all</artifactId>
This dependency is already defined in root pom.xml
Line 71: <version>${mockito.version}</version>
Line 72: <scope>test</scope>
Line 73: </dependency>
Line 74: <dependency>
Line 75: <groupId>commons-logging</groupId>
Line 76: <artifactId>commons-logging</artifactId>
Line 77: <version>1.1</version>
Line 78: <exclusions>
Line 79: <exclusion> <!-- declare the exclusion here -->
Did you consider moving this exclusion into root pom.xml which declares
commons-logging dependency?
This would reveal Maven modules which currently depend on servlet API JAR
transitively via commons-logging.
Line 80: <groupId>javax.servlet</groupId>
Line 81: <artifactId>servlet-api</artifactId>
Line 82: </exclusion>
Line 83: </exclusions>
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/DocsServlet.java
Line 16: import org.ovirt.engine.core.utils.ServletUtils;
Line 17:
Line 18: /**
Line 19: * @author awels
Line 20: *
Please add some Javadoc that briefly describes the purpose of this servlet.
Line 21: */
Line 22: public class DocsServlet extends FileServlet {
Line 23: // The log:
Line 24: private static final Logger log =
Logger.getLogger(FileServlet.class);
Line 33: private static final String LANG_JSP = "/WEB-INF/help/no_lang.jsp";
Line 34:
Line 35: @Override
Line 36: /**
Line 37: *
Please avoid empty (auto-generated?) Javadoc
Line 38: */
Line 39: protected void doGet(HttpServletRequest request,
HttpServletResponse response) throws ServletException, IOException {
Line 40: // Locate the requested file:
Line 41: File originalFile =
ServletUtils.makeFileFromSanePath(request.getPathInfo(), base);
Line 41: File originalFile =
ServletUtils.makeFileFromSanePath(request.getPathInfo(), base);
Line 42: Locale locale = getLocaleFromRequest(request);
Line 43: File file = determineActualFile(request, locale);
Line 44: file = checkForIndex(request, response, file,
request.getPathInfo());
Line 45: if (null == file) {
Please use "something <operator> null" convention
Line 46: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 47: } else if (!response.isCommitted()){ //If the response is
committed, we have already redirected.
Line 48: Object languagePageShown =
request.getSession(true).getAttribute(LANG_PAGE_SHOWN);
Line 49: if (!file.equals(originalFile)) {
Line 48: Object languagePageShown =
request.getSession(true).getAttribute(LANG_PAGE_SHOWN);
Line 49: if (!file.equals(originalFile)) {
Line 50: //We determined that we are going to redirect the user
to the English version URI.
Line 51: String redirect = request.getServletPath() +
replaceLocaleWithUSLocale(request.getPathInfo(), locale);
Line 52: if ((null == languagePageShown ||
!Boolean.parseBoolean(languagePageShown.toString()))) {
Please use "something <operator> null" convention
Line 53:
request.getSession(true).setAttribute(LANG_PAGE_SHOWN, true);
Line 54: request.setAttribute(LOCALE, locale);
Line 55: request.setAttribute(ENGLISH_HREF, redirect);
Line 56: RequestDispatcher dispatcher =
request.getRequestDispatcher(LANG_JSP);
Line 72: /**
Line 73: *
Line 74: * @param request
Line 75: * @param locale
Line 76: * @return
Please avoid empty (auto-generated?) Javadoc
Line 77: */
Line 78: protected File determineActualFile(final HttpServletRequest
request, Locale locale) {
Line 79: File file =
ServletUtils.makeFileFromSanePath(request.getPathInfo(), base);
Line 80: // Check if file is found. If not found go ahead and try and
look up the English US locale version.
Line 77: */
Line 78: protected File determineActualFile(final HttpServletRequest
request, Locale locale) {
Line 79: File file =
ServletUtils.makeFileFromSanePath(request.getPathInfo(), base);
Line 80: // Check if file is found. If not found go ahead and try and
look up the English US locale version.
Line 81: if (null != file && !ServletUtils.canReadFile(file)) {
Please use "something <operator> null" convention
Line 82: file =
ServletUtils.makeFileFromSanePath(replaceLocaleWithUSLocale(request.getPathInfo(),
locale), base);
Line 83: }
Line 84: return file;
Line 85: }
Line 104: */
Line 105: protected Locale getLocaleFromRequest(final HttpServletRequest
request) {
Line 106: String localeString = getLocaleStringFromReferer(request);
Line 107: //Unable to determine locale string from referer (preferred
way)
Line 108: if (null == localeString) {
Please use "something <operator> null" convention
Line 109: //Note this fails if the is something like /menu.css
Line 110: localeString =
getLocaleStringFromPath(request.getPathInfo());
Line 111: }
Line 112: //Validate that the locale string is valid.
Line 123: * the found locale is a file and not a directory, return null as
the locale is a filename.
Line 124: */
Line 125: protected String getLocaleStringFromPath(final String path) {
Line 126: String result = null;
Line 127: if (null != path) {
Please use "something <operator> null" convention
Line 128: if (!path.startsWith("/")) {
Line 129: throw new IllegalArgumentException("Path should start
with a '/'");
Line 130: }
Line 131: //Attempt to determine locale from path info.
Line 125: protected String getLocaleStringFromPath(final String path) {
Line 126: String result = null;
Line 127: if (null != path) {
Line 128: if (!path.startsWith("/")) {
Line 129: throw new IllegalArgumentException("Path should start
with a '/'");
Do we really want to throw Exception here?
I think it would be better (and consistent with regard to
getLocaleStringFromReferer) to just return null that would propagate up to
doGet, which would do response.sendError(SC_BAD_REQUEST)
Line 130: }
Line 131: //Attempt to determine locale from path info.
Line 132: String[] pathElements = path.substring(1).split("/");
Line 133: File localeFile = new File(base, pathElements[0]);
Line 154: final URI refererURL;
Line 155: String result = null;
Line 156: try {
Line 157: String referer = request.getHeader(REFERER);
Line 158: if (null != referer) {
Please use "something <operator> null" convention
Line 159: refererURL = new URI(referer);
Line 160: String query = refererURL.getQuery();
Line 161: if (null != query) {
Line 162: String[] parameters = query.split("&");
Line 156: try {
Line 157: String referer = request.getHeader(REFERER);
Line 158: if (null != referer) {
Line 159: refererURL = new URI(referer);
Line 160: String query = refererURL.getQuery();
Why create new URI just to get the query part?
You could just use String.indexOf + String.substring instead.
Note - in case the purpose of this method is also to validate referer URL:
creating URI and catching exception is IMHO bad way to do URL validation.
Line 161: if (null != query) {
Line 162: String[] parameters = query.split("&");
Line 163: for (int i = 0; i < parameters.length; i++) {
Line 164: String[] keyValues = parameters[i].split("=");
Line 157: String referer = request.getHeader(REFERER);
Line 158: if (null != referer) {
Line 159: refererURL = new URI(referer);
Line 160: String query = refererURL.getQuery();
Line 161: if (null != query) {
Please use "something <operator> null" convention
Line 162: String[] parameters = query.split("&");
Line 163: for (int i = 0; i < parameters.length; i++) {
Line 164: String[] keyValues = parameters[i].split("=");
Line 165: if (LOCALE.equalsIgnoreCase(keyValues[0])) {
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/FileServlet.java
Line 91: protected void doGet(HttpServletRequest request,
HttpServletResponse response) throws ServletException, IOException {
Line 92: // Locate the requested file:
Line 93: File file =
ServletUtils.makeFileFromSanePath(request.getPathInfo(), base);
Line 94: file = checkForIndex(request, response, file,
request.getPathInfo());
Line 95: if (null == file) {
Please use "something <operator> null" convention
Line 96: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 97: } else {
Line 98: // Send the content of the file:
Line 99: // type is the default MIME type of the Servlet.
Line 103:
Line 104: protected File checkForIndex(HttpServletRequest request,
HttpServletResponse response, File file, String path) throws IOException {
Line 105: // If the requested file is a directory then try to replace
it with the
Line 106: // corresponding index page (if it exists):
Line 107: if (null != file && file.isDirectory()) {
Please use "something <operator> null" convention
Line 108: File index = new File(file, INDEX);
Line 109: log.info("Index is \"" + index.getAbsolutePath() + "\".");
Line 110: if (index.isFile()) {
Line 111: String redirect = null;
....................................................
File
backend/manager/modules/root/src/test/java/org/ovirt/engine/core/DocsServletTest.java
Line 1: /**
Line 2: *
Please avoid empty (auto-generated?) Javadoc
Line 3: */
Line 4: package org.ovirt.engine.core;
Line 5:
Line 6: import static org.junit.Assert.assertEquals;
Line 37: import org.ovirt.engine.core.utils.ServletUtils;
Line 38:
Line 39: /**
Line 40: * @author awels
Line 41: *
Please avoid empty (auto-generated?) Javadoc
Line 42: */
Line 43: @RunWith(MockitoJUnitRunner.class)
Line 44: public class DocsServletTest {
Line 45: DocsServlet testServlet;
Line 52: @Mock
Line 53: ServletConfig mockConfig;
Line 54:
Line 55: /**
Line 56: * @throws java.lang.Exception
Please avoid empty (auto-generated?) Javadoc
Line 57: */
Line 58: @Before
Line 59: public void setUp() throws Exception {
Line 60: testServlet = new DocsServlet();
Line 70: * @throws IOException
Line 71: * @throws ServletException
Line 72: */
Line 73: @Test
Line 74: public void testDoGetHttpServletRequestHttpServletResponseIndex()
throws ServletException, IOException {
Small comment (up to you to consider): I'd recommend using
methodUnderTest_testCaseDescription_extraDescriptionIfNecessary() convention,
which is commonly accepted standard providing test case documentation directly
via method names.
Line 75: //Because we would have found the index file, we do a redirect
and thus the response is committed.
Line 76: when(mockResponse.isCommitted()).thenReturn(Boolean.TRUE);
Line 77: when(mockRequest.getServletPath()).thenReturn("/docs");
Line 78: testServlet.doGet(mockRequest, mockResponse);
....................................................
File
backend/manager/modules/root/src/test/java/org/ovirt/engine/core/FileServletTest.java
Line 1: /**
Line 2: *
Please avoid empty (auto-generated?) Javadoc
Line 3: */
Line 4: package org.ovirt.engine.core;
Line 5:
Line 6: import static org.junit.Assert.assertEquals;
Line 33: import org.mockito.runners.MockitoJUnitRunner;
Line 34:
Line 35: /**
Line 36: * @author awels
Line 37: *
Please avoid empty (auto-generated?) Javadoc
Line 38: */
Line 39: @RunWith(MockitoJUnitRunner.class)
Line 40: public class FileServletTest {
Line 41: FileServlet testServlet;
Line 47: @Mock
Line 48: HttpServletResponse mockResponse;
Line 49:
Line 50: /**
Line 51: * @throws java.lang.Exception
Please avoid empty (auto-generated?) Javadoc
Line 52: */
Line 53: @Before
Line 54: public void setUp() throws Exception {
Line 55: testServlet = new FileServlet();
Line 61: * Test method for {@link
org.ovirt.engine.core.FileServlet#init(javax.servlet.ServletConfig)}.
Line 62: * @throws ServletException
Line 63: */
Line 64: @Test(expected=ServletException.class)
Line 65: public void testInitServletConfigNoInitParams() throws
ServletException {
Small comment (up to you to consider): I'd recommend using
methodUnderTest_testCaseDescription_extraDescriptionIfNecessary() convention,
which is commonly accepted standard providing test case documentation directly
via method names.
Line 66: testServlet.init(mockConfig);
Line 67: fail("Should not get here");
Line 68: }
Line 69:
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/LocaleUtils.java
Line 15: return getLocaleFromString(localeString, false);
Line 16: }
Line 17:
Line 18: /**
Line 19: *
Please provide proper Javadoc here
Line 20: * @param localeString
Line 21: * @param returnNull
Line 22: * @return
Line 23: */
Line 24: public static Locale getLocaleFromString(String localeString,
boolean returnDefaultLocale) {
Line 25: Locale result = returnDefaultLocale ? null : Locale.US;
Line 26: try {
Line 27: result =
org.apache.commons.lang.LocaleUtils.toLocale(localeString);
Line 28: if(null == result && returnDefaultLocale) {
Please use "something <operator> null" convention
Line 29: result = Locale.US;
Line 30: }
Line 31: }
Line 32: catch(IllegalArgumentException e) {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ServletUtils.java
Line 43: }
Line 44:
Line 45: // Anything longer than this is considered a large file and a
warning
Line 46: // will be generating when serving it:
Line 47: private static final long LARGE = 1048576; // 1 MiB
Why move this field beyond method declarations, e.g. past ServletUtils
constructor?
Line 48:
Line 49: /**
Line 50: * Send a file to the output stream of the response passed into
the method.
Line 51: * @param request The {@code HttpServletRequest} so we can get the
path of the file.
Line 152:
Line 153: // All checks passed, the path is sane:
Line 154: return true;
Line 155: }
Line 156:
Please add proper Javadoc here
Line 157: public static File makeFileFromSanePath(String path, File base) {
Line 158: File file = null;
Line 159:
Line 160: if (path == null) {
Line 153: // All checks passed, the path is sane:
Line 154: return true;
Line 155: }
Line 156:
Line 157: public static File makeFileFromSanePath(String path, File base) {
Small comment (up to you to consider): I think the method name is misleading,
e.g. make file from path that's already sane, I'd recommend renaming it to
something like getFileFromPath, and mention the sanity check in Javadoc.
Line 158: File file = null;
Line 159:
Line 160: if (path == null) {
Line 161: file = base;
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/LocaleUtilsTest.java
Line 9:
Line 10: public class LocaleUtilsTest {
Line 11:
Line 12: @Test
Line 13: public void testGetLocaleFromStringString() {
Small comment (up to you to consider): I'd recommend using
methodUnderTest_testCaseDescription_extraDescriptionIfNecessary() convention,
which is commonly accepted standard providing test case documentation directly
via method names.
Line 14: assertNull("The locale should be null",
LocaleUtils.getLocaleFromString(null));
Line 15: assertNull("The locale should be null",
LocaleUtils.getLocaleFromString("notalocale"));
Line 16: assertNull("The locale should be null",
LocaleUtils.getLocaleFromString("index.html"));
Line 17: assertEquals("The locale should be jp", Locale.JAPANESE,
LocaleUtils.getLocaleFromString("ja"));
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/ServletUtilsTest.java
Line 1: /**
Line 2: *
Please avoid empty (auto-generated?) Javadoc
Line 3: */
Line 4: package org.ovirt.engine.core.utils;
Line 5:
Line 6: import static org.junit.Assert.assertEquals;
Line 27: import org.junit.Test;
Line 28:
Line 29: /**
Line 30: * @author awels
Line 31: *
Please avoid empty (auto-generated?) Javadoc
Line 32: */
Line 33: public class ServletUtilsTest {
Line 34:
Line 35: /**
Line 32: */
Line 33: public class ServletUtilsTest {
Line 34:
Line 35: /**
Line 36: * @throws java.lang.Exception
Please avoid empty (auto-generated?) Javadoc
Line 37: */
Line 38: @Before
Line 39: public void setUp() throws Exception {
Line 40: }
Line 42: /**
Line 43: * Test method for {@link
org.ovirt.engine.core.ServletUtils#canReadFile(java.io.File)}.
Line 44: */
Line 45: @Test
Line 46: public void testCanReadFile() {
Small comment (up to you to consider): I'd recommend using
methodUnderTest_testCaseDescription_extraDescriptionIfNecessary() convention,
which is commonly accepted standard providing test case documentation directly
via method names.
Line 47: //Does not exist.
Line 48: File file = new File("/doesnotexist/iamprettysure");
Line 49: assertFalse("We should not be able to read this file.",
ServletUtils.canReadFile(file));
Line 50: //Exists, but should not be readable.
--
To view, visit http://gerrit.ovirt.org/10065
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37156061cbdd7d2df3290c88dee933c41e0087c5
Gerrit-PatchSet: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
