You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jmeter.apache.org by pm...@apache.org on 2015/07/14 23:31:34 UTC
svn commit: r1691090 -
/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
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
Re: svn commit: r1691090 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by sebb <se...@gmail.com>.
On 18 July 2015 at 12:42, Philippe Mouawad <ph...@gmail.com> wrote:
> Hi sebb,
> My answers below.
> Regards
>
> On Saturday, July 18, 2015, sebb <se...@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.
Re: svn commit: r1691090 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi sebb,
My answers below.
Regards
On Saturday, July 18, 2015, sebb <se...@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.
>
> > 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
> >
> >> 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.
Re: svn commit: r1691090 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by sebb <se...@gmail.com>.
On 16 July 2015 at 23:26, Philippe Mouawad <ph...@gmail.com> wrote:
> On Thursday, July 16, 2015, sebb <se...@gmail.com> 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?
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.
> 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.
>
>> 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:;>> 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.
Re: svn commit: r1691090 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by Philippe Mouawad <ph...@gmail.com>.
On Thursday, July 16, 2015, sebb <se...@gmail.com> wrote:
> -1
>
> This may break existing test plans,
Are you sure, I only updated the part that concerns embedded download.
It's a real issue for some users, and should be fixed.
> and is contrary to the
> component_reference documentation.
This can be updated
>
> On 14 July 2015 at 22:31, <pmouawad@apache.org <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.
Re: svn commit: r1691090 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by sebb <se...@gmail.com>.
-1
This may break existing test plans, and is contrary to the
component_reference documentation.
On 14 July 2015 at 22:31, <pm...@apache.org> 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
>
>
Re: svn commit: r1691090 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by Philippe Mouawad <pm...@apache.org>.
Hello,
This commit will break Tests of HTMLParserTestFile_2.jmx which relies on
file: protocol.
URL file:testfiles/HTMLParserTestFile_2.html will lead to wrong input to
escapeIllegalURLCharacters:
file:HTMLParserTestFile_2_files/style.css
instead of:
file://HTMLParserTestFile_2_files/style.css
But I must say I wonder if this test is OK ?
Thoughts ?
Regards
Philippe