[BugFix] Modifying JsonContainerConfig to add SERVER_HOST and SERVER_PORT env. to all containers

Currently JsonContainerConfig adds SERVER_HOST and SERVER_PORT environment variables only to default container. However, this addition is done after merging the containers with default container.

This change moves the SERVER_HOST / SERVER_PORT environment variables addition code to before container merging so that all all containers configs get these environment variables.

[anupama.dutta, 2010-08-21 11:07:04]

I didn't fully understand the effect of this change. Could you elaborate on why
this change is useful to have - for e.g. are there URLs which used to not work
before which will now work because of this change?

http://codereview.appspot.com/2000043/diff/8001/9002
File
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
(right):

http://codereview.appspot.com/2000043/diff/8001/9002#newcode90
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:90:
addCommonEnvironmentToAllContainers(configJson, host, port);
Maybe addCommonEnvironmentVarsToAllContainers or
addCommonEnvtVarsToAllContainers would be better?

[gagan.goku, 2010-08-21 11:12:32]

On 2010/08/21 11:07:04, anupama.dutta wrote:
> I didn't fully understand the effect of this change. Could you elaborate on
why
> this change is useful to have - for e.g. are there URLs which used to not work
> before which will now work because of this change?
Currently everything works fine. But this change is needed when you have more
than 1 containers defined in container.js
For example, if you want to add container X (like i added container "accel" in
http://codereview.appspot.com/1949048/diff/7001/2007), you need to bind
SERVER_HOST and SERVER_PORT for these containers as well. Otherwise those
containers dont get their values.
I am myself unclear on how shindig handles containers and how come no1 else has
run into this? I thought easiest thing to do was to make a small patch and test
and ask :)

> 
> http://codereview.appspot.com/2000043/diff/8001/9002
> File
> java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
> (right):
> 
> http://codereview.appspot.com/2000043/diff/8001/9002#newcode90
>
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:90:
> addCommonEnvironmentToAllContainers(configJson, host, port);
> Maybe addCommonEnvironmentVarsToAllContainers or
> addCommonEnvtVarsToAllContainers would be better?

[gagan.goku, 2010-08-21 11:19:47]

http://codereview.appspot.com/2000043/diff/8001/9002
File
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
(right):

http://codereview.appspot.com/2000043/diff/8001/9002#newcode90
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:90:
addCommonEnvironmentToAllContainers(configJson, host, port);
On 2010/08/21 11:07:04, anupama.dutta wrote:
> Maybe addCommonEnvironmentVarsToAllContainers or
> addCommonEnvtVarsToAllContainers would be better?

Done.

[gagan.goku, 2010-08-24 13:02:28]

Hi Satya

Please take a look.

Thanks
Gagan

[satya3656, 2010-08-24 14:38:03]

LGTM

[gagan.goku, 2010-08-24 14:38:36]

Thanks Satya

On Tue, Aug 24, 2010 at 8:08 PM, <satya3656@gmail.com> wrote:

> LGTM
>
>
>
> http://codereview.appspot.com/2000043/
>

[zhoresh, 2010-08-24 16:50:26]

http://codereview.appspot.com/2000043/diff/13001/5003
File
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
(right):

http://codereview.appspot.com/2000043/diff/13001/5003#newcode90
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:90:
addCommonEnvironmentVarsToAllContainers(configJson, host, port);
I thought that all containers configs are derived from the default container
config, so won't they just inherit the host and port?
Can you explain the case you notice the problem.

[gagan.goku, 2010-08-24 16:55:08]

I believe Jesse raised this problem sometime ago:
http://markmail.org/message/27jfrktjrmhw7nm5

http://codereview.appspot.com/2000043/diff/13001/5003
File
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
(right):

http://codereview.appspot.com/2000043/diff/13001/5003#newcode90
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:90:
addCommonEnvironmentVarsToAllContainers(configJson, host, port);
On 2010/08/24 16:50:26, zhoresh wrote:
> I thought that all containers configs are derived from the default container
> config, so won't they just inherit the host and port?
> Can you explain the case you notice the problem.

I noticed the problem when i tried to add accel container config by modifying
container.js like this:
{"gadgets.container" : ["default", "accel"],

In case of accel servlet, the following variable:
"defaultShindigProxyConcatAuthority": "${SERVER_HOST}:${SERVER_PORT}"

has value ":"
and hence proxied urls look like this:

//:/gadgets/...

Look at the test case to see whats going wrong.

[Paul Lindner, 2010-08-25 06:46:26]

patch is okay, but we already have code to propagate the default container
values into all other containers.  It's in loadContainers()  See comments for an
alternate approach to deal with this issue.

http://codereview.appspot.com/2000043/diff/13001/5003
File
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
(left):

http://codereview.appspot.com/2000043/diff/13001/5003#oldcode378
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:378:
// Now that all containers are loaded, we go back through them and merge
This is the code that merges the default params, we should probably split this
into it's own routine (mergeContainers?) and call it after we modify the
container with values from the environment..

[gagan.goku, 2010-08-25 08:04:27]

Thanks for taking a look Paul. Comment inline:

http://codereview.appspot.com/2000043/diff/13001/5003
File
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java
(left):

http://codereview.appspot.com/2000043/diff/13001/5003#oldcode378
java/common/src/main/java/org/apache/shindig/config/JsonContainerConfig.java:378:
// Now that all containers are loaded, we go back through them and merge
On 2010/08/25 06:46:26, Paul Lindner wrote:
> This is the code that merges the default params, we should probably split this
> into it's own routine (mergeContainers?) and call it after we modify the
> container with values from the environment..
> 

I don't have enough idea / confidence about refactoring this class yet. I took a
brief look at the mergeParents and mergeObjects method and it seems we resolve
all the way to the default container, and somehow merge fields into the current
container (i guess every1 knew that duhh)
Then i tried moving out the SERVER_HOST and SERVER_PORT code before this merge
business and my test seems to still pass. I guess whats happening is that
SERVER_HOST and SERVER_PORT fields of default container are getting merged into
all the containers.

Are you okay with this for now ? Please let me know if i'm missing out something
obvious.

[Paul Lindner, 2010-08-27 13:06:42]

yes, you got it.  adding the host/port to the default container before the merge
gets you the desired behavior.

[gagan.goku, 2010-08-27 14:10:14]

Thanks for submitting this change as well Paul.

Gagan

On Fri, Aug 27, 2010 at 6:36 PM, <lindner@inuus.com> wrote:

> yes, you got it.  adding the host/port to the default container before
> the merge gets you the desired behavior.
>
>
>
> http://codereview.appspot.com/2000043/
>