Concat & Proxied Url should not contain complete url as gadget param

Currently, Concat & Proxied Url contains complete url of original page as gadet param. But this causes cacheability issue (cross page same resource usage).  Browser uses this Get url as a key to check whether a resource is present in cache or not. And for two different pages, these cant be same.

[satya3656, 2011-01-21 07:02:30]

LGTM

[nikhilmadan23, 2011-01-21 07:12:01]

lgtm

http://codereview.appspot.com/4050043/diff/3001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java
(right):

http://codereview.appspot.com/4050043/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java:45:
import org.apache.shindig.gadgets.uri.ProxyUriBase;
Order alphabetically.

[anupama.dutta, 2011-01-21 10:26:08]

Can we add a test to ensure that this is working as expected when the flag is
true?

http://codereview.appspot.com/4050043/diff/3001/java/common/conf/shindig.prop...
File java/common/conf/shindig.properties (right):

http://codereview.appspot.com/4050043/diff/3001/java/common/conf/shindig.prop...
java/common/conf/shindig.properties:162: #Set gadget param in proxied uri as
authority if this is true
Space before Set.

[pulkitgoyal2000, 2011-02-21 10:29:09]

Test added.

http://codereview.appspot.com/4050043/diff/3001/java/common/conf/shindig.prop...
File java/common/conf/shindig.properties (right):

http://codereview.appspot.com/4050043/diff/3001/java/common/conf/shindig.prop...
java/common/conf/shindig.properties:162: #Set gadget param in proxied uri as
authority if this is true
On 2011/01/21 10:26:08, anupama.dutta wrote:
> Space before Set.

Done.

http://codereview.appspot.com/4050043/diff/3001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java
(right):

http://codereview.appspot.com/4050043/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java:45:
import org.apache.shindig.gadgets.uri.ProxyUriBase;
On 2011/01/21 07:12:01, nikhilmadan23 wrote:
> Order alphabetically.

Done.

[anupama.dutta, 2011-03-01 12:41:21]

Thanks for adding tests. Some minor comments below.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:1:
package org.apache.shindig.gadgets.uri;
Could you add the usual copyright header to this file?

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:17:
Test for ProxyUriBase
Can remove this block of comments.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:22:
private Gadget getGadget(final String container) throws SpecParserException {
Can we call this createGadget? It is easy to confuse this with the .getGadget()
method that is being tested here.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:42:
@Override
Remove @Override (I think this won't compile in java 1.5)

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:43:
protected void configure() {
Indentation should be 2 spaces from line 41's indentation.

[pulkitgoyal2000, 2011-03-02 06:45:41]

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:1:
package org.apache.shindig.gadgets.uri;
On 2011/03/01 12:41:21, anupama.dutta wrote:
> Could you add the usual copyright header to this file?

Done.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:17:
Test for ProxyUriBase
On 2011/03/01 12:41:21, anupama.dutta wrote:
> Can remove this block of comments.

Done.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:22:
private Gadget getGadget(final String container) throws SpecParserException {
On 2011/03/01 12:41:21, anupama.dutta wrote:
> Can we call this createGadget? It is easy to confuse this with the
.getGadget()
> method that is being tested here.

Done.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:42:
@Override
On 2011/03/01 12:41:21, anupama.dutta wrote:
> Remove @Override (I think this won't compile in java 1.5)
I checked that it compiled in java 1.5. Also, @override is used at many other
places too.

http://codereview.appspot.com/4050043/diff/19001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:43:
protected void configure() {
On 2011/03/01 12:41:21, anupama.dutta wrote:
> Indentation should be 2 spaces from line 41's indentation.

Done.

[anupama.dutta, 2011-03-02 07:13:10]

LGTM. Please send to dev with Gagan, Fargo and Ziv as reviewers.

[gagan.goku, 2011-03-02 15:29:01]

LGTM.
Just make sure that the PUriBase(Gadget gadget) is the only constructor that you
need to override and this cl is good.

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:60:
bind(Boolean.TYPE).annotatedWith(
shouldnt this be Boolean.class

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:72:
Injector injector = Guice.createInjector(new AbstractModule() {
you can make a function out of this, say:

injectAuthorityAsGadgetParam(Boolean val) {
  return Guice.createInjector(new AbstractModule() {
    @Override
    protected void configure() {
      bind(Boolean.class).annotatedWith(
         
Names.named("org.apache.shindig.gadgets.uri.setAuthorityAsGadgetParam"))
          .toInstance(val);
          requestStaticInjection(ProxyUriBase.class);
  };
}

[zhoresh, 2011-03-02 18:17:42]

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
(right):

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:45:
@Inject(optional=false)
Maybe set optional=true so you don't force other shindig users to add injection?

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:47:
static private boolean setAuthorityAsGadgetParam = false;
Maybe set it as a Provider<Boolean>, so you can link it to a dflag later.

[satya3656, 2011-03-02 19:04:27]

Small nits

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:67:
assertEquals(proxyUriBase.getGadget(), URI.getAuthority());
It should be assertEquals(expected, actual)

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:82:
assertEquals(proxyUriBase.getGadget(), URI.toString());
It should be assertEquals(expected, actual)

[pulkitgoyal2000, 2011-03-03 15:04:55]

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
(right):

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:45:
@Inject(optional=false)
On 2011/03/02 18:17:42, zhoresh wrote:
> Maybe set optional=true so you don't force other shindig users to add
injection?

Done.

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:47:
static private boolean setAuthorityAsGadgetParam = false;
On 2011/03/02 18:17:42, zhoresh wrote:
> Maybe set it as a Provider<Boolean>, so you can link it to a dflag later.

Done.

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:60:
bind(Boolean.TYPE).annotatedWith(
On 2011/03/02 15:29:02, gagan.goku wrote:
> shouldnt this be Boolean.class

Done.

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:67:
assertEquals(proxyUriBase.getGadget(), URI.getAuthority());
On 2011/03/02 19:04:27, satya3656 wrote:
> It should be assertEquals(expected, actual)

Done.

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:72:
Injector injector = Guice.createInjector(new AbstractModule() {
On 2011/03/02 15:29:02, gagan.goku wrote:
> you can make a function out of this, say:
> 
> injectAuthorityAsGadgetParam(Boolean val) {
>   return Guice.createInjector(new AbstractModule() {
>     @Override
>     protected void configure() {
>       bind(Boolean.class).annotatedWith(
>          
> Names.named("org.apache.shindig.gadgets.uri.setAuthorityAsGadgetParam"))
>           .toInstance(val);
>           requestStaticInjection(ProxyUriBase.class);
>   };
> }

Done.

http://codereview.appspot.com/4050043/diff/25002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:82:
assertEquals(proxyUriBase.getGadget(), URI.toString());
On 2011/03/02 19:04:27, satya3656 wrote:
> It should be assertEquals(expected, actual) 

Done.

[gagan.goku, 2011-03-03 15:13:15]

lgtm

@Ziv: This addresses your comment on being able to make a dflag out of it.

[pulkitgoyal2000, 2011-03-04 05:40:25]

Hi Ziv,
I reverted back Provider Change. Actually we dont want to make
setAuthorityAsGadgetParam as dflag. We have moved to separate binary so we wont
change the value for gadgets.
Sorry for the inconvenience.

[zhoresh, 2011-03-04 17:19:45]

On 2011/03/04 05:40:25, pulkitgoyal2000 wrote:
> Hi Ziv,
> I reverted back Provider Change. Actually we dont want to make
> setAuthorityAsGadgetParam as dflag. We have moved to separate binary so we
wont
> change the value for gadgets.
> Sorry for the inconvenience.

Even though your current plan is a separate server it might change, so provide
runtime config capabilities might not be a bad idea.
But either way is fine.
I have concern about static injection here - it doesn't play well with unit
tests. Tests run in parallel threads so changing value in one thread can effect
the other - as Jacobo just find out and had to fix similar case.

[satya3656, 2011-03-08 10:22:49]

small nit

LGTM

http://codereview.appspot.com/4050043/diff/37002/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/37002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:30:
import static org.junit.Assert.assertEquals;
Move static import before normal imports

[pulkitgoyal2000, 2011-03-08 13:40:18]

http://codereview.appspot.com/4050043/diff/37002/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java
(right):

http://codereview.appspot.com/4050043/diff/37002/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/ProxyUriBaseTest.java:30:
import static org.junit.Assert.assertEquals;
On 2011/03/08 10:22:49, satya3656 wrote:
> Move static import before normal imports

Done.

[zhoresh, 2011-03-08 16:43:23]

LGTM
Thanks.

[gagan.goku, 2011-03-08 18:59:10]

Build looks good.
Committed as r1079486.