Vojtech Szocs has posted comments on this change.
Change subject: core: rearrange product uris
......................................................................
Patch Set 13:
(13 comments)
Alex and Alon, great job on this patch!
In addition to my inline comments:
* I really like moving most stuff out of root WAR's web.xml and into specific
WARs like services, root's web.xml now contains bare minimum to support legacy
URLs (we're planning to remove them eventually, right?)
* BrandingServlet + BrandingCascadingResourceServlet definitions and mappings
tend to repeat across different web.xml files, why not use Servlet 3.0
web-fragment.xml in some shared JAR (utils?) to define defaults and let
specific WARs override these within their web.xml?
* getting rid of duplicate webadmin/userportal path element is nice, I just
feel this could be done in a separate patch (anyway, I'm OK with this being
part of this patch)
....................................................
File backend/manager/modules/docs/pom.xml
Line 65:
Line 66: <dependency>
Line 67: <groupId>${engine.groupId}</groupId>
Line 68: <artifactId>branding</artifactId>
Line 69: <version>${engine.version}</version>
Isn't branding JAR supposed to be shared across the Engine EAR via some JBoss
module?
Line 70: </dependency>
Line 71: </dependencies>
Line 72:
Line 73: <build>
....................................................
File backend/manager/modules/services/pom.xml
Line 53: <plugins>
Line 54:
Line 55: <plugin>
Line 56: <groupId>org.apache.maven.plugins</groupId>
Line 57: <artifactId>maven-war-plugin</artifactId>
Not sure if this declaration (and therefore the entire build section) is really
needed here, i.e. maven-war-plugin without any additional config overrides.
By default, maven-war-plugin's "war" goal is invoked during the "package" phase
for projects with packaging == "war".
Line 58: </plugin>
Line 59: </plugins>
Line 60: </build>
Line 61:
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/ForwardServlet.java
Line 32: * The key to use in web.xml to specify the target context.
Line 33: */
Line 34: public static final String CONTEXT_PARAM = "targetContext";
//$NON-NLS-1$
Line 35: /**
Line 36: * The key to use in web.xml to specify the page not found forward
URI.
Maybe "target URI" instead of "page not found forward URI" ?
Line 37: */
Line 38: public static final String URI_PARAM = "uri"; //$NON-NLS-1$
Line 39: /**
Line 40: * The URI parameter that will simply try to forward the request
path to the new context.
Line 36: * The key to use in web.xml to specify the page not found forward
URI.
Line 37: */
Line 38: public static final String URI_PARAM = "uri"; //$NON-NLS-1$
Line 39: /**
Line 40: * The URI parameter that will simply try to forward the request
path to the new context.
Consider adding "... without any additional URI suffix"
Line 41: */
Line 42: public static final String ALL = "*"; //$NON-NLS-1$
Line 43: /**
Line 44: * Init param prefix for adding attributes.
Line 46: public static final String ATTR_PREF = "attr-";
Line 47: /**
Line 48: * Init param prefix for adding request parameters.
Line 49: */
Line 50: public static final String REQUEST_PREF = "req-";
doForward method currently doesn't support adding custom request parameters, is
this constant declaration necessary?
Line 51:
Line 52: /**
Line 53: * The target context to use when forwarding.
Line 54: */
Line 53: * The target context to use when forwarding.
Line 54: */
Line 55: private String targetContext = null;
Line 56: /**
Line 57: * The page not found URI to use when forwarding.
Maybe "target URI" instead of "page not found URI" ?
Line 58: */
Line 59: private String uri = null;
Line 60:
Line 61: @Override
Line 70: }
Line 71: if (targetContext == null) {
Line 72: throw new ServletException("Target context not defined in
web.xml"); //$NON-NLS-1$
Line 73: }
Line 74: if (uri == null) {
Why is URI_PARAM mandatory? Can't it be optional with ALL used as fallback
value?
There wouldn't be any impact on logic inside doForward method which deals with
ALL special value regardless.
Line 75: throw new ServletException("Target uri not defined in
web.xml"); //$NON-NLS-1$
Line 76: }
Line 77: }
Line 78:
Line 120:
Line 121: /**
Line 122: * Forward the request to the appropriate servlet in the context
specified in the init method. This works
Line 123: * for all verbs due to the fact that forward calls service in
the target servlet, which determines which
Line 124: * verb was used based on the request object.
Can you please explain what you mean by "forward calls service in the target
servlet"?
IIUC, this servlet just overrides all available doXYZ methods and calls
doForward method.
Line 125: *
Line 126: * If the initialization of the {@code ForwardServlet} includes
special parameters with a key starts with
Line 127: * 'attr-' those keys and values will be added to the request
attributes for use in the target servlet. The
Line 128: * 'attr-' prefix will be stripped from attribute key.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/RedirectServlet.java
Line 8: import javax.servlet.http.HttpServletResponse;
Line 9:
Line 10: import org.ovirt.engine.core.utils.EngineLocalConfig;
Line 11:
Line 12: public class RedirectServlet extends HttpServlet {
I suggest adding a (very brief) Javadoc here.
Line 13: private static final long serialVersionUID = -1794616863361241804L;
Line 14:
Line 15: private static final String URL = "url";
Line 16:
Line 19: @Override
Line 20: public void init(ServletConfig config) throws ServletException {
Line 21: super.init(config);
Line 22:
Line 23: // we use %{x} convention to avoid conflict with jboss
properties
Very useful comment! Can you also mention this in ForwardServlet code?
Ideally, Javadoc for both servlets would include something like:
uri (ForwardServlet) or url (RedirectServlet) param value can contain property
expressions for expanding Engine property values in form of %{PROP_NAME}
Line 24: url =
EngineLocalConfig.getInstance().expandString(config.getInitParameter(URL).replaceAll("%\\{",
"\\${"));
Line 25: }
Line 26:
Line 27: @Override
....................................................
File backend/manager/modules/welcome/pom.xml
Line 54:
Line 55: <dependency>
Line 56: <groupId>${engine.groupId}</groupId>
Line 57: <artifactId>branding</artifactId>
Line 58: <version>${engine.version}</version>
Isn't branding JAR supposed to be shared across the Engine EAR via some JBoss
module?
Line 59: </dependency>
Line 60:
Line 61: </dependencies>
Line 62:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServlet.java
Line 43: ATTR_SELECTOR_SCRIPT("selectorScript"), //$NON-NLS-1$
Line 44: ATTR_USER_INFO("userInfo"), //$NON-NLS-1$
Line 45: ATTR_STYLES("brandingStyle"), //$NON-NLS-1$
Line 46: ATTR_MESSAGES("messages"), //$NON-NLS-1$
Line 47: ATTR_CONF("conf"), //$NON-NLS-1$
Shouldn't we assign more specific name to this attribute, i.e. baseContextPath?
If we expect more config values to be added later on, I'm in favor of grouping
data based on logical purpose, instead of just one big object placeholder to
contain logically different kinds of data.
Line 48: ATTR_LOCALE(LocaleFilter.LOCALE),
Line 49: ATTR_APPLICATION_TYPE(BrandingFilter.APPLICATION_NAME);
Line 50:
Line 51: private final String attributeKey;
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/utils/HostPageConfiguration.java
Line 1: package org.ovirt.engine.ui.frontend.utils;
Line 2:
Line 3: import com.google.gwt.i18n.client.Dictionary;
Line 4:
Line 5: public class HostPageConfiguration {
I think using GWT Dictionary is just unnecessarily complex and error-prone.
If we want to read native JS object data embedded in GWT host page, just use JS
overlay types + JSNI to retrieve that data. Generally speaking, unless the
embedded JS data's structure is dynamic in nature, JS overlay types are always
the preferred way to retrieve embedded data whose structure is static in nature.
Take a look at org.ovirt.engine.ui.common.auth.AutoLoginData class, it's really
simple and small:
* public static instance method to retrieve the JS overlay instance
* native getters to retrieve members (attributes) of native JS object
* (optional) public methods to construct objects composed of individual data
parts, i.e. getVdcUser method
I suggest to replace HostPageConfiguration with something like
BaseContextPathData (see my comment in GwtDynamicHostPageServlet.java):
/**
* Overlay type for {@code baseContextPath} global JS object.
*/
public final class BaseContextPathData extends JavaScriptObject {
protected BaseContextPathData() {
}
public static native BaseContextPathData instance() /*-{
return $wnd.baseContextPath;
}-*/;
private native String getValue() /*-{
return this.value;
}-*/;
}
Above assumes that following object gets embedded into GWT host page:
var baseContextPath = { "value": "xyz" };
As I wrote in my comment in GwtDynamicHostPageServlet.java, I'm in favor of
grouping data based on logical purpose, instead of just one big object
placeholder to contain logically different kinds of data. In other words, I
prefer "baseContextPath" object with single value over "config" object with
potentially multiple values, i.e. granularity driven by logical purpose of
embedded data.
Line 6: /**
Line 7: * The name under which the dictionary will appear in the host page.
Line 8: */
Line 9: private static final String CONF_DICTIONARY_NAME = "conf";
//$NON-NLS-1$
--
To view, visit http://gerrit.ovirt.org/20473
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cb4822f6bf4d372715e12858635db5ed3edd115
Gerrit-PatchSet: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Yaniv Dary <yd...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
