tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kalle Korhonen <kalle.o.korho...@gmail.com>
Subject Re: svn commit: r1068758 - in /tapestry/tapestry5/trunk/tapestry-core/src: main/java/org/apache/tapestry5/ main/java/org/apache/tapestry5/internal/services/ main/java/org/apache/tapestry5/services/ test/java/org/apache/tapestry5/internal/services/
Date Wed, 09 Feb 2011 19:37:57 GMT
On Wed, Feb 9, 2011 at 12:10 AM, Igor Drobiazko
<igor.drobiazko@gmail.com> wrote:
> Hi Kalle,
> congrats to your first commit. You should import Tapestry code formatting
> into your IDE. Please see here:
> http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/support/?pathrev=1068758

Thanks Igor, meant to ask about that.

> Regarding the change. Wouldn't it be better to set defaults for new symbols
> to hostport = 80 and hostport-secure=443?

That's certainly up for discussion. I didn't want to change the
current behavior without any consensus. It'd almost make sense to use
those as the defaults, but it could cause some unexpected surprises in
development environments (interestingly BaseUrlSourceImpl doesn't
honor tapestry.secure-enabled, another issue there?). Perhaps you
could do something more complex, such as contribute 80 and 443 when
production mode is on, but not at that time when contributing factory
defaults. Ties to the larger discussion about better supporting dev,
test, production profiles, but probably best to leave that for another
issue, don't you think?

One more thing - which value should I have used for @since? 5.3.0? (I
used 5.2.5 for now following Howard's lead on another new symbol, but
didn't check if his was actually merged into 5.2 maintenance branch).

Kalle


> On Wed, Feb 9, 2011 at 7:25 AM, <kaosko@apache.org> wrote:
>
>> Author: kaosko
>> Date: Wed Feb  9 06:25:01 2011
>> New Revision: 1068758
>>
>> URL: http://svn.apache.org/viewvc?rev=1068758&view=rev
>> Log:
>> Complete - issue TAP5-1414: Add HOSTNAME symbol to SymbolConstants, use in
>> BaseUrlSource
>> https://issues.apache.org/jira/browse/TAP5-1414
>> - added HOSTNAME, HOSTPORT and HOSTPORT_SECURE symbols to SymbolConstants
>> - contributed defaults for the new symbols (defaults won't change existing
>> behavior)
>> - added javadoc
>> - modified BaseURLSourceImpl to use the new symbols
>> - added new test class for testing BaseURLSourceImpl
>>
>> Added:
>>
>>  tapestry/tapestry5/trunk/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/BaseURLSourceImplTest.java
>> Modified:
>>
>>  tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java
>>
>>  tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/BaseURLSourceImpl.java
>>
>>  tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/BaseURLSource.java
>>
>>  tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java
>>
>> Modified:
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java
>> URL:
>> http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java?rev=1068758&r1=1068757&r2=1068758&view=diff
>>
>> ==============================================================================
>> ---
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java
>> (original)
>> +++
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java
>> Wed Feb  9 06:25:01 2011
>> @@ -334,4 +334,29 @@ public class SymbolConstants
>>      * @since 5.2.5
>>      */
>>     public static final String COMPONENT_RENDER_TRACING_ENABLED =
>> "tapestry.component-render-tracing-enabled";
>> +
>> +    /**
>> +     * The hostname that application should use when constructing an
>> absolute URL. The default is "", i.e. an empty string,
>> +     * in which case system will use request.getServerName(). Not the same
>> as environment variable HOSTNAME, but you can also
>> +     * contribute "$HOSTNAME" as the value to make it the same as the
>> environment variable HOSTNAME.
>> +     *
>> +     * @since 5.2.5
>> +     */
>> +    public static final String HOSTNAME = "tapestry.hostname";
>> +
>> +    /**
>> +     * The hostport that application should use when constructing an
>> absolute URL. The default is "0", i.e. use the port value from
>> +     * the request.
>> +     *
>> +     * @since 5.2.5
>> +     */
>> +    public static final String HOSTPORT = "tapestry.hostport";
>> +
>> +    /**
>> +     * The secure (https) hostport that application should use when
>> constructing an absolute URL. The default is "0", i.e. use
>> +     * the value from the request.
>> +     *
>> +     * @since 5.2.5
>> +     */
>> +    public static final String HOSTPORT_SECURE =
>> "tapestry.hostport-secure";
>>  }
>>
>> Modified:
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/BaseURLSourceImpl.java
>> URL:
>> http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/BaseURLSourceImpl.java?rev=1068758&r1=1068757&r2=1068758&view=diff
>>
>> ==============================================================================
>> ---
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/BaseURLSourceImpl.java
>> (original)
>> +++
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/BaseURLSourceImpl.java
>> Wed Feb  9 06:25:01 2011
>> @@ -14,26 +14,44 @@
>>
>>  package org.apache.tapestry5.internal.services;
>>
>> +import org.apache.tapestry5.SymbolConstants;
>> +import org.apache.tapestry5.ioc.annotations.Inject;
>> +import org.apache.tapestry5.ioc.annotations.Symbol;
>>  import org.apache.tapestry5.services.BaseURLSource;
>>  import org.apache.tapestry5.services.Request;
>>
>>  public class BaseURLSourceImpl implements BaseURLSource
>>  {
>>     private final Request request;
>> +
>> +    private String hostname;
>> +    private int hostPort;
>> +    private int secureHostPort;
>>
>> -    public BaseURLSourceImpl(Request request)
>> +    public BaseURLSourceImpl(Request request, @Inject
>> @Symbol(SymbolConstants.HOSTNAME) String hostname,
>> +           @Symbol(SymbolConstants.HOSTPORT) int hostPort,
>> @Symbol(SymbolConstants.HOSTPORT_SECURE) int secureHostPort)
>>     {
>>         this.request = request;
>> +        this.hostname = hostname;
>> +        this.hostPort = hostPort;
>> +        this.secureHostPort = secureHostPort;
>>     }
>>
>>     public String getBaseURL(boolean secure)
>>     {
>> -        int port = request.getServerPort();
>> +        int port = secure ? secureHostPort : hostPort;
>> +        String portSuffix = "";
>>
>> -        int schemeDefaultPort = request.isSecure() ? 443 : 80;
>> -
>> -        String portSuffix = port == schemeDefaultPort ? "" : ":" + port;
>> -
>> -        return String.format("%s://%s%s", secure ? "https" : "http",
>> request.getServerName(), portSuffix);
>> +        if (port <= 0) {
>> +            port = request.getServerPort();
>> +            int schemeDefaultPort = request.isSecure() ? 443 : 80;
>> +            portSuffix = port == schemeDefaultPort ? "" : ":" + port;
>> +        }
>> +        else if (secure && port != 443) portSuffix = ":" + port;
>> +        else if (port != 80) portSuffix = ":" + port;
>> +
>> +        String hostname = "".equals(this.hostname) ?
>> request.getServerName() : this.hostname.startsWith("$") ?
>> System.getenv(this.hostname.substring(1)) : this.hostname;
>> +
>> +        return String.format("%s://%s%s", secure ? "https" : "http",
>> hostname, portSuffix);
>>     }
>>  }
>>
>> Modified:
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/BaseURLSource.java
>> URL:
>> http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/BaseURLSource.java?rev=1068758&r1=1068757&r2=1068758&view=diff
>>
>> ==============================================================================
>> ---
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/BaseURLSource.java
>> (original)
>> +++
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/BaseURLSource.java
>> Wed Feb  9 06:25:01 2011
>> @@ -24,7 +24,17 @@ import org.apache.tapestry5.ioc.services
>>  * necessary to do a bit more, since <code>getServerName()</code> will
>> often be the name of the internal server (not
>>  * visible to the client web browser), and a hard-coded name of a server
>> that <em>is</em> visible to the web browser
>>  * is needed. Further, in testing, non-default ports are often used. In
>> those cases, an overriding contribution to the
>> - * {@link ServiceOverride} service will allow a custom implementation to
>> supercede the default version.
>> + * {@link ServiceOverride} service will allow a custom implementation to
>> supercede the default version. You may also
>> + * contribute application specific values for the following {@link
>> org.apache.tapestry5.SymbolConstants}:
>> + * {@link org.apache.tapestry5.SymbolConstants#HOSTNAME}, {@link
>> org.apache.tapestry5.SymbolConstants#HOSTPORT} and
>> + * {@link org.apache.tapestry5.SymbolConstants#HOSTPORT_SECURE} to alter
>> the behavior of the default BaseURLSource
>> + * implementation. The default values for the SymbolConstants require
>> {@link org.apache.tapestry5.services.Request}
>> + * context to be available. If you contribute specific values for the
>> specified SymbolConstants, it's safe to use
>> + * the default implementation of this service outside of request context,
>> for example in a batch job. For
>> + * {@link org.apache.tapestry5.SymbolConstants#HOSTNAME}, a value starting
>> with a dollar sign ($) will be resolved
>> + * using {@link java.langSystem#getEnv()} - contributing "$HOSTNAME" for
>> {@link org.apache.tapestry5.SymbolConstants#HOSTNAME}
>> + * is the most sensible choice for a dynamic value that doesn't use
>> + * {@link org.apache.tapestry5.services.Request#getServerName()}.
>>  */
>>  public interface BaseURLSource
>>  {
>>
>> Modified:
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java
>> URL:
>> http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java?rev=1068758&r1=1068757&r2=1068758&view=diff
>>
>> ==============================================================================
>> ---
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java
>> (original)
>> +++
>> tapestry/tapestry5/trunk/tapestry-core/src/main/java/org/apache/tapestry5/services/TapestryModule.java
>> Wed Feb  9 06:25:01 2011
>> @@ -2450,6 +2450,11 @@ public final class TapestryModule
>>         configuration.add(InternalSymbols.PRE_SELECTED_FORM_NAMES,
>> "reset,submit,select,id,method,action,onsubmit");
>>
>>         configuration.add(SymbolConstants.COMPONENT_RENDER_TRACING_ENABLED,
>> "false");
>> +
>> +        // The default values denote "use values from request"
>> +        configuration.add(SymbolConstants.HOSTNAME, "");
>> +        configuration.add(SymbolConstants.HOSTPORT, "0");
>> +        configuration.add(SymbolConstants.HOSTPORT_SECURE, "0");
>>     }
>>
>>     /**
>>
>> Added:
>> tapestry/tapestry5/trunk/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/BaseURLSourceImplTest.java
>> URL:
>> http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/BaseURLSourceImplTest.java?rev=1068758&view=auto
>>
>> ==============================================================================
>> ---
>> tapestry/tapestry5/trunk/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/BaseURLSourceImplTest.java
>> (added)
>> +++
>> tapestry/tapestry5/trunk/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/BaseURLSourceImplTest.java
>> Wed Feb  9 06:25:01 2011
>> @@ -0,0 +1,74 @@
>> +// Copyright 2008, 2010, 2011 The Apache Software Foundation
>> +//
>> +// Licensed 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.tapestry5.internal.services;
>> +
>> +import org.apache.tapestry5.internal.test.InternalBaseTestCase;
>> +import org.apache.tapestry5.services.BaseURLSource;
>> +import org.apache.tapestry5.services.Request;
>> +import org.testng.annotations.BeforeMethod;
>> +import org.testng.annotations.Test;
>> +
>> +
>> +public class BaseURLSourceImplTest extends InternalBaseTestCase {
>> +    private Request request;
>> +
>> +    @BeforeMethod
>> +    public void setUp() {
>> +       request = mockRequest();
>> +    }
>> +
>> +    @Test
>> +    public void getBaseURLFromRequest() {
>> +        expect(request.getServerName()).andReturn("localhost").once();
>> +        expect(request.getServerPort()).andReturn(80).once();
>> +        expect(request.isSecure()).andReturn(false).once();
>> +        replay();
>> +        BaseURLSource baseURLSource = new BaseURLSourceImpl(request,
>> "localhost", 0, 0);
>> +        assertEquals(baseURLSource.getBaseURL(false), "http://localhost
>> ");
>> +    }
>> +
>> +    @Test
>> +    public void getBaseURLWithContributedHostname() {
>> +        expect(request.getServerPort()).andReturn(80).once();
>> +        expect(request.isSecure()).andReturn(false).once();
>> +        replay();
>> +        BaseURLSource baseURLSource = new BaseURLSourceImpl(request, "
>> my.server.com", 0, 0);
>> +        assertEquals(baseURLSource.getBaseURL(false), "
>> http://my.server.com");
>> +    }
>> +
>> +    @Test
>> +    public void getBaseURLWithEnvHostname() {
>> +        expect(request.getServerPort()).andReturn(80).once();
>> +        expect(request.isSecure()).andReturn(false).once();
>> +        replay();
>> +        BaseURLSource baseURLSource = new BaseURLSourceImpl(request,
>> "$HOSTNAME", 0, 0);
>> +        assertEquals(baseURLSource.getBaseURL(false), "http://" +
>> System.getenv("HOSTNAME"));
>> +    }
>> +
>> +    @Test
>> +    public void getBaseURLWithContributedValuesDontUseRequest() {
>> +        replay();
>> +        BaseURLSource baseURLSource = new BaseURLSourceImpl(request,
>> "localhost", 80, 443);
>> +        assertEquals(baseURLSource.getBaseURL(false), "http://localhost
>> ");
>> +    }
>> +
>> +    @Test
>> +    public void getBaseURLWithContributedNonStandardSecurePort() {
>> +        replay();
>> +        BaseURLSource baseURLSource = new BaseURLSourceImpl(request,
>> "localhost", 80, 8443);
>> +        assertEquals(baseURLSource.getBaseURL(true), "
>> https://localhost:8443");
>> +    }
>> +
>> +}
>>
>>
>>
>
>
> --
> Best regards,
>
> Igor Drobiazko
> http://tapestry5.de
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org


Mime
View raw message