Hi Li, Comments inline.
On Fri, May 13, 2011 at 3:26 PM, Li Xu <[email protected]> wrote: > hello, > I wish we see your comments before committing. A Provider pattern sounds > nice. I have a couple of questions regarding the patch under 4527056. > Agree about having been able to comment earlier. I suspect dev-remailer@ no longer sends to my account, or perhaps it somehow got spam-filtered. Will check. > > The patch submitted under r1102446 will always take host/port from servlet > request, thus it won't take static value setting up in web.xml. We probably > missed some use case here, is there any reason for not taking host/port from > ServletRequest? I checked the new patch submited under 4527056, it still > requires setting up shindig.host/shindig.port in web.xml or as jvm setting. > it will be great help if you could please explain a little. > The logic of your patch seemed fine to me, particularly for the default use case. My concern was over A) the code's side effects (System.setProperty in particular), and B) the fact that there's no *other* way to set this information. Within our installation, we have a separate means for obtaining this information. It's a similar implementation, but one that picks up significantly more information. It sets up a RequestContext object with various items like request type, query params, path info, some derived contextual data, and so on. It propagates this information along forked threads as well and is used in various ways. The underlying impl is similar - a set of Filters that contribute to a carefully-managed ThreadLocal - but the main point is that we'd prefer to be able to inject Host (et al) using it rather than a parallel method. Following on to my comments from before, IMO the best place for this logic is in ContainerConfig. Putting %host% substitution logic there means that all downstream classes get it without code modification. Admittedly, ContainerConfig is a little awkward to augment today. It's done through subclassing. I'd suggesting introducing an Injected list of ConfigModifiers that can perform these edits. WDYT? --j > > thanks, > li > > > > > [image: Inactive hide details for John Hjelmstad ---05/13/2011 04:15:37 > PM---Hi all, This CL unfortunately suffers from a lot of static]John > Hjelmstad ---05/13/2011 04:15:37 PM---Hi all, This CL unfortunately suffers > from a lot of static cling. There's no way to > > > From: > John Hjelmstad <[email protected]> > To: > [email protected] > Date: > 05/13/2011 04:15 PM > Subject: > Re: svn commit: r1102446 - in /shindig/trunk: config/ > content/samplecontainer/examples/commoncontainer/ > java/common/src/main/java/org/apache/shindig/common/servlet/ > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ > java/server/src/main/webap > ------------------------------ > > > > Hi all, > > This CL unfortunately suffers from a lot of static cling. There's no way to > customize this behavior, and the default behavior ends up having odd side > effects like getting and setting system properties, many of which are > irrelevant, and tightly monitored, in production environments. > > I've temporarily pulled this logic out into a provider pattern: > http://codereview.appspot.com/4527056/ > > ...so that users aren't forced into this behavior. > > This said, I actually feel that the better solution for this is to put > host-substitution logic into ContainerConfig. All these config values come > from there anyway. > > --j > > On Thu, May 12, 2011 at 1:00 PM, <[email protected]> wrote: > > > Author: woodser > > Date: Thu May 12 20:00:18 2011 > > New Revision: 1102446 > > > > URL: http://svn.apache.org/viewvc?rev=1102446&view=rev > > Log: > > Committing Li Xu's patch to detect host & port from HttpServletRequest. > > > > Codereview: http://codereview.appspot.com/4534049/ > > JIRA: https://issues.apache.org/jira/browse/SHINDIG-1534 > > > > Added: > > > > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/HostFilter.java > > > > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java > > Modified: > > shindig/trunk/config/container.js > > > > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json > > > > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java > > > > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > > shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml > > > > Modified: shindig/trunk/config/container.js > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/config/container.js?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- shindig/trunk/config/container.js (original) > > +++ shindig/trunk/config/container.js Thu May 12 20:00:18 2011 > > @@ -103,10 +103,13 @@ > > //"gadgets.securityTokenKeyFile" : "/path/to/key/file.txt", > > > > // URI for the default shindig test instance. > > -"defaultShindigTestHost": "http://${SERVER_HOST}:${SERVER_PORT}";, > > +//"defaultShindigTestHost": "http://${SERVER_HOST}:${SERVER_PORT}";, > > +"defaultShindigTestHost":"http://%host%";, > > + > > > > // Authority (host:port without scheme) for the proxy and concat > servlets. > > -"defaultShindigProxyConcatAuthority": "${SERVER_HOST}:${SERVER_PORT}", > > +//"defaultShindigProxyConcatAuthority": "${SERVER_HOST}:${SERVER_PORT}", > > +"defaultShindigProxyConcatAuthority":"%host%", > > > > // OS 2.0 Gadget DOCTYPE: used in Gadgets with @specificationVersion 2.0 > > or greater and > > // quirksmode on Gadget has not been set. > > > > Modified: > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json > > (original) > > +++ > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/gadgetCollections.json > > Thu May 12 20:00:18 2011 > > @@ -4,22 +4,22 @@ > > "name": "Publish Subscribe Demo", > > "Description": "This is a sample pub/sub demo showcasing the > > pubsub-2 feature", > > "apps" : [ > > - {"name": "publisher", "url": " > > http://localhost:8080/container/sample-pubsub-2-publisher.xml"}, > > - {"name": "subscriber", "url": " > > http://localhost:8080/container/sample-pubsub-2-subscriber.xml"} > > + {"name": "publisher", "url": > > "/container/sample-pubsub-2-publisher.xml"}, > > + {"name": "subscriber", "url": > > "/container/sample-pubsub-2-subscriber.xml"} > > ] > > }, > > { > > "name": "Activity Streams Sample", > > "Description": "Simple gadget to test base ActivityStreams > > implementation in features", > > "apps" : [ > > - {"name": "ActivityStreams Sample", "url": " > > > http://localhost:8080/samplecontainer/examples/ActivityStreams/ActivityStreamGadget.xml > > "} > > + {"name": "ActivityStreams Sample", "url": > > "/samplecontainer/examples/ActivityStreams/ActivityStreamGadget.xml"} > > ] > > }, > > { > > "name": "Sample Media Items Gadget", > > "Description": "This is a sample pub/sub demo showcasing the > > pubsub-2 feature", > > "apps" : [ > > - {"name": "publisher", "url": " > > http://localhost:8080/samplecontainer/examples/media/Media.xml"} > > + {"name": "publisher", "url": > > "/samplecontainer/examples/media/Media.xml"} > > ] > > }, > > { > > > > Modified: > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js > > (original) > > +++ > > > shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js > > Thu May 12 20:00:18 2011 > > @@ -45,7 +45,11 @@ $(function() { > > $.each(data.collections, function(i,data){ > > var optionVal = []; > > $.each(data.apps, function(i,data){ > > - optionVal.push(data.url); > > + if (data.url.indexOf("http") < 0){ > > + > > optionVal.push(location.protocol+"//"+location.host+data.url); > > + }else{ > > + optionVal.push(data.url); > > + } > > }); > > $('#gadgetCollection').append('<option > > value="'+ optionVal.toString() + '">' + data.name +'</option>'); > > }); > > > > Added: > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/HostFilter.java > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/HostFilter.java?rev=1102446&view=auto > > > > > ============================================================================== > > --- > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/HostFilter.java > > (added) > > +++ > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/HostFilter.java > > Thu May 12 20:00:18 2011 > > @@ -0,0 +1,50 @@ > > +/* > > + * 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.shindig.common.servlet; > > + > > +import java.io.IOException; > > + > > +import javax.servlet.Filter; > > +import javax.servlet.FilterConfig; > > +import javax.servlet.FilterChain; > > +import javax.servlet.ServletContext; > > +import javax.servlet.ServletException; > > +import javax.servlet.ServletRequest; > > +import javax.servlet.ServletResponse; > > + > > + > > + > > + > > +/** > > + * A Filter that can cache ServletRequest information in ThreadLocal > > variable > > + */ > > +public class HostFilter implements Filter { > > + > > + public void doFilter(ServletRequest request, ServletResponse response, > > FilterChain chain) throws IOException, ServletException { > > + ServletRequestContext.setRequestInfo(request); > > + chain.doFilter(request, response); > > + } > > + > > + public void destroy() { > > + } > > + > > + public void init(FilterConfig arg0) throws ServletException { > > + } > > +} > > > > Added: > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java?rev=1102446&view=auto > > > > > ============================================================================== > > --- > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java > > (added) > > +++ > > > shindig/trunk/java/common/src/main/java/org/apache/shindig/common/servlet/ServletRequestContext.java > > Thu May 12 20:00:18 2011 > > @@ -0,0 +1,105 @@ > > +/* > > + * 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.shindig.common.servlet; > > + > > +import javax.servlet.ServletRequest; > > + > > +public class ServletRequestContext { > > + > > + public static void setRequestInfo(ServletRequest req) { > > + String auth = req.getServerName() + ":" + req.getServerPort(); > > + String fullAuth = req.getScheme() + "://" + auth; > > + authority.set(auth); > > + fullAuthority.set(fullAuth); > > + > > + System.setProperty("authority",auth); > > + System.setProperty("fullAuthority", fullAuth); > > + } > > + > > + /** > > + * A Thread Local holder for authority -- host + port > > + */ > > + private static ThreadLocal<String> authority = new > > ThreadLocal<String>(); > > + > > + /** > > + * A Thread Local holder for full authority -- scheme + host + port > > + */ > > + private static ThreadLocal<String> fullAuthority = new > > ThreadLocal<String>(); > > + > > + > > + public static String getAuthority() { > > + > > + String retVal = authority.get(); > > + if (retVal == null) { > > + retVal = System.getProperty("authority"); > > + if (retVal == null){ > > + retVal = getDefaultAuthority(); > > + } > > + } > > + return retVal; > > + } > > + > > + private static String getDefaultAuthority() { > > + > > + String retVal = System.getProperty("defaultAuthority"); > > + if (retVal == null){ > > + retVal = getServerHostname()+":"+getServerPort(); > > + System.setProperty("defaultAuthority", retVal); > > + } > > + return retVal; > > + > > + } > > + > > + public static String getFullAuthority() { > > + > > + String retVal = fullAuthority.get(); > > + if (retVal == null) { > > + retVal = System.getProperty("fullAuthority"); > > + if (retVal == null){ > > + retVal = getDefaultFullAuthority(); > > + } > > + } > > + return retVal; > > + > > + } > > + > > + private static String getDefaultFullAuthority() { > > + > > + String retVal = System.getProperty("defaultFullAuthority"); > > + if ( retVal != null ){ > > + retVal = "http://"+getServerHostname()+":"+getServerPort(); > > + System.setProperty("defaultFullAuthority", retVal); > > + } > > + return retVal; > > + > > + } > > + > > + private static String getServerPort() { > > + return System.getProperty("shindig.port") != null ? > > System.getProperty("shindig.port") : > > + System.getProperty("jetty.port") != null ? > > System.getProperty("jetty.port") : > > + "8080"; > > + } > > + > > + private static String getServerHostname() { > > + return System.getProperty("shindig.host") != null ? > > System.getProperty("shindig.host") : > > + System.getProperty("jetty.host") != null ? > > System.getProperty("jetty.host") : > > + "localhost"; > > + } > > + > > +} > > > > Modified: > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java > > (original) > > +++ > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java > > Thu May 12 20:00:18 2011 > > @@ -24,6 +24,7 @@ import com.google.inject.Inject; > > import com.google.inject.name.Named; > > > > import org.apache.commons.lang.StringEscapeUtils; > > +import org.apache.shindig.common.servlet.ServletRequestContext; > > import org.apache.shindig.common.uri.Uri; > > import org.apache.shindig.common.uri.UriBuilder; > > import org.apache.shindig.config.ContainerConfig; > > @@ -222,6 +223,8 @@ public class DefaultConcatUriManager imp > > throw new RuntimeException( > > "Missing required config '" + key + "' for container: " + > > container); > > } > > + val = val.replace("%host%", ServletRequestContext.getAuthority()); > > + > > return val; > > } > > > > > > Modified: > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java > > (original) > > +++ > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java > > Thu May 12 20:00:18 2011 > > @@ -27,6 +27,7 @@ import org.apache.commons.lang.StringUti > > import org.apache.shindig.auth.SecurityToken; > > import org.apache.shindig.auth.SecurityTokenCodec; > > import org.apache.shindig.auth.SecurityTokenException; > > +import org.apache.shindig.common.servlet.ServletRequestContext; > > import org.apache.shindig.common.uri.Uri; > > import org.apache.shindig.common.uri.UriBuilder; > > import org.apache.shindig.config.ContainerConfig; > > @@ -342,10 +343,13 @@ public class DefaultIframeUriManager imp > > > > private String getReqVal(String container, String key) { > > String val = config.getString(container, key); > > + > > if (val == null) { > > throw new RuntimeException("Missing required container config > param, > > key: " > > + key + ", container: " + container); > > } > > + val = val.replace("%host%", ServletRequestContext.getAuthority()); > > + > > return val; > > } > > > > > > Modified: > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java > > (original) > > +++ > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java > > Thu May 12 20:00:18 2011 > > @@ -23,6 +23,7 @@ import com.google.common.base.Splitter; > > import com.google.common.collect.ImmutableList; > > import com.google.inject.Inject; > > > > +import org.apache.shindig.common.servlet.ServletRequestContext; > > import org.apache.shindig.common.uri.Uri; > > import org.apache.shindig.common.uri.UriBuilder; > > import org.apache.shindig.config.ContainerConfig; > > @@ -222,6 +223,7 @@ public class DefaultJsUriManager impleme > > "' missing config for required param: " + key); > > } > > } > > + ret = ret.replace("%host%", ServletRequestContext.getAuthority()); > > return ret; > > } > > > > > > Modified: > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > > (original) > > +++ > > > shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > > Thu May 12 20:00:18 2011 > > @@ -25,6 +25,7 @@ import com.google.inject.Inject; > > import com.google.inject.name.Named; > > > > import org.apache.commons.lang.StringUtils; > > +import org.apache.shindig.common.servlet.ServletRequestContext; > > import org.apache.shindig.common.uri.Uri; > > import org.apache.shindig.common.uri.UriBuilder; > > import org.apache.shindig.common.util.Utf8UrlCoder; > > @@ -276,6 +277,7 @@ public class DefaultProxyUriManager impl > > throw new RuntimeException("Missing required container config key: > " > > + key + " for " + > > "container: " + container); > > } > > + val = val.replace("%host%", ServletRequestContext.getAuthority()); > > return val; > > } > > } > > > > Modified: shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml > > URL: > > > http://svn.apache.org/viewvc/shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml?rev=1102446&r1=1102445&r2=1102446&view=diff > > > > > ============================================================================== > > --- shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml (original) > > +++ shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml Thu May 12 > > 20:00:18 2011 > > @@ -59,14 +59,24 @@ > > <context-param> > > <param-name>system.properties</param-name> > > <param-value> > > - shindig.host=localhost > > - > > - aKey=/shindig/gadgets/proxy?container=default&url= > > - shindig.port= > > - > > + aKey=/shindig/gadgets/proxy?container=default&url= > > </param-value> > > </context-param> > > - > > + > > + <filter> > > + <filter-name>hostFilter</filter-name> > > + > > > <filter-class>org.apache.shindig.common.servlet.HostFilter</filter-class> > > + </filter> > > + <filter-mapping> > > + <filter-name>hostFilter</filter-name> > > + <url-pattern>/gadgets/ifr</url-pattern> > > + <url-pattern>/gadgets/js/*</url-pattern> > > + <url-pattern>/gadgets/proxy/*</url-pattern> > > + <url-pattern>/gadgets/concat</url-pattern> > > + <url-pattern>/rpc/*</url-pattern> > > + <url-pattern>/rest/*</url-pattern> > > + </filter-mapping> > > + > > <filter> > > <filter-name>ShiroFilter</filter-name> > > > > <filter-class>org.apache.shiro.web.servlet.IniShiroFilter</filter-class> > > > > > > > > >
