Mailing-List: contact dev-help@jmeter.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jmeter.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAH9fUpZJnbuY1MycS_S1=Lg3z6zLTiAHTG=nq6Z+VCsD3asasQ@mail.gmail.com>
References: <20150714213134.7F9C7AC0041@hades.apache.org>
	<CAOGo0VaFd74Y=FZBKNuk2KetpYcrdR2st3EdjMraUZSxy0M6AQ@mail.gmail.com>
	<CAH9fUpYv4pR+wDSJmXYXwPe=PCZrV+0fBNBMAL_Jxo5n7a3peg@mail.gmail.com>
	<CAOGo0VbnPYXcS7Dq0QGd3WwDNaAB6+be83gMCCgY3UhgJE9Mzw@mail.gmail.com>
	<CAH9fUpZJnbuY1MycS_S1=Lg3z6zLTiAHTG=nq6Z+VCsD3asasQ@mail.gmail.com>
Date: Sat, 18 Jul 2015 12:53:36 +0100
Message-ID: 
 <CAOGo0VatdtUe2c1mMT-vf0LijXtFJL141sRk9Dq2UoFabn2jDw@mail.gmail.com>
Subject: Re: svn commit: r1691090 -
 /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
From: sebb <sebbaz@gmail.com>
To: dev@jmeter.apache.org
Content-Type: text/plain; charset=UTF-8

On 18 July 2015 at 12:42, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> Hi sebb,
> My answers below.
> Regards
>
> On Saturday, July 18, 2015, sebb <sebbaz@gmail.com> wrote:
>
>> On 16 July 2015 at 23:26, Philippe Mouawad <philippe.mouawad@gmail.com
>> <javascript:;>> wrote:
>> > On Thursday, July 16, 2015, sebb <sebbaz@gmail.com <javascript:;>>
>> wrote:
>> >
>> >> -1
>> >>
>> >> This may break existing test plans,
>> >
>> >
>> > Are you sure, I only updated the part that concerns embedded download.
>>
>> Sorry, I thought this related to fixing URLs provided in the "Path" field.
>>
>> However, I am still wary of changing the processing.
>> If the embedded URL has incorrect syntax, then why should JMeter fix it?
>
> The "broken" url work correctly in browsers, you can test with my test
> content.
> And note currently JMeter "partially fixes" spaces
>
>>
>> At the very least, I think JMeter should warn the user that the URL was
>> invalid.
>> That way there is at least a chance that it can be fixed.

This is still true.
If the URL is silently fixed, how is it ever going to be reported and
fixed at source?

>>
>> > It's a real issue for some users, and should be fixed.
>>
>> Or the app developers should fix the URLs so that they are valid.
>>
>> Not always possible and I am not sure url is illegal

If it's not illegal, why is it being rejected?

>> >
>> >> and is contrary to the
>> >> component_reference documentation.
>> >
>> > This can be updated
>>
>> If the patch relates to embedded downloads then the component ref docs
>> don't need to be changed.
>>
>>
>
>> >>
>> >> On 14 July 2015 at 22:31,  <pmouawad@apache.org <javascript:;>
>> <javascript:;>> wrote:
>> >> > Author: pmouawad
>> >> > Date: Tue Jul 14 21:31:34 2015
>> >> > New Revision: 1691090
>> >> >
>> >> > URL: http://svn.apache.org/r1691090
>> >> > Log:
>> >> > Bug 58137 - JMeter fails to download embedded URLS that contain
>> illegal
>> >> characters in URL (it does not escape them)
>> >> > Bugzilla Id: 58137
>> >> >
>> >> > Modified:
>> >> >
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>> >> >
>> >> > Modified:
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>> >> > URL:
>> >>
>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java?rev=1691090&r1=1691089&r2=1691090&view=diff
>> >> >
>> >>
>> ==============================================================================
>> >> > ---
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>> >> (original)
>> >> > +++
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>> >> Tue Jul 14 21:31:34 2015
>> >> > @@ -1242,7 +1242,7 @@ public abstract class HTTPSamplerBase ex
>> >> >                          log.warn("Null URL detected (should not
>> >> happen)");
>> >> >                      } else {
>> >> >                          String urlstr = url.toString();
>> >> > -                        String urlStrEnc=encodeSpaces(urlstr);
>> >> > +                        String
>> >> urlStrEnc=escapeIllegalURLCharacters(encodeSpaces(urlstr));
>> >> >                          if (!urlstr.equals(urlStrEnc)){// There were
>> >> some spaces in the URL
>> >> >                              try {
>> >> >                                  url = new URL(urlStrEnc);
>> >> > @@ -1352,6 +1352,23 @@ public abstract class HTTPSamplerBase ex
>> >> >      }
>> >> >
>> >> >      /**
>> >> > +     * @param url URL to escape
>> >> > +     * @return escaped url
>> >> > +     */
>> >> > +    private String escapeIllegalURLCharacters(String url) {
>> >> > +        try {
>> >> > +            String escapedUrl =
>> >> ConversionUtils.escapeIllegalURLCharacters(url);
>> >> > +            if(log.isDebugEnabled()) {
>> >> > +                log.debug("Successfully escaped url:'"+url +"'
>> >> to:'"+escapedUrl+"'");
>> >> > +            }
>> >> > +            return escapedUrl;
>> >> > +        } catch (Exception e1) {
>> >> > +            log.error("Error escaping URL:'"+url+"',
>> >> message:"+e1.getMessage());
>> >> > +            return url;
>> >> > +        }
>> >> > +    }
>> >> > +
>> >> > +    /**
>> >> >       * Extract User-Agent header value
>> >> >       * @param sampleResult HTTPSampleResult
>> >> >       * @return User Agent part
>> >> >
>> >> >
>> >>
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.