jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1732590 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
Date Sat, 27 Feb 2016 13:42:25 GMT
On 27 February 2016 at 13:06, Philippe Mouawad
<philippe.mouawad@gmail.com> wrote:
> Hi,
> I changed code a bit in r1732634 because there was a bug anyway.
>
> My answers inline.
>
> Regards
> Philippe
>
> On Sat, Feb 27, 2016 at 1:54 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 27 February 2016 at 12:09, Philippe Mouawad
>> <philippe.mouawad@gmail.com> wrote:
>> > On Sat, Feb 27, 2016 at 2:01 AM, <sebb@apache.org> wrote:
>> >
>> >> Author: sebb
>> >> Date: Sat Feb 27 01:01:07 2016
>> >> New Revision: 1732590
>> >>
>> >> URL: http://svn.apache.org/viewvc?rev=1732590&view=rev
>> >> Log:
>> >> Revert change that broke JMX compatibility
>> >> TODO fix GUI to support change in default
>> >>
>> >> Modified:
>> >>
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
>> >>
>> >> Modified:
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java?rev=1732590&r1=1732589&r2=1732590&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> ---
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
>> >> (original)
>> >> +++
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
>> >> Sat Feb 27 01:01:07 2016
>> >> @@ -101,8 +101,13 @@ public class CookieManager extends Confi
>> >>      private transient CookieHandler cookieHandler;
>> >>
>> >>      private transient CollectionProperty initialCookies;
>> >> +
>> >> +    // MUST NOT BE CHANGED
>> >>
>> > A more explicit note should be here to explain why it must not be
>> changed:
>> > - Reference the bug or the case
>>
>> I can expand the text but the reason has little to do with the bug or
>> the test case
>> The reason it cannot be changed is that it relates to how JMX files
>> are read/written.
>> Upwards compatibility requires immutable default values
>>
>> >> +    @SuppressWarnings("deprecation") // cannot be changed
>> >> +    public static final String DEFAULT_POLICY =
>> >> CookieSpecs.BROWSER_COMPATIBILITY;
>> >>
>> > Thinking more about it, keeping "Wrong Default" while the good one are in
>> > GUI does not seem to me great:
>> > - Code readability, HC4CookieHandler is the real default, developers or
>> > users might think it is still HC3CookieHandler
>>
>> That is not the whole story, see below
>>
>> > - These Constants will disappear in next HttpClient 5 version , they are
>> > already deprecated
>>
>> No, they cannot just disappear.
>>
> I am speaking about the HC constants, they are already gone.
> I understand our constants cannot disappear.
>
>>
>> There are two defaults in operation here.
>>
>> * The defaults used in connection with reading/writing JMX files.
>> These must remain immutable otherwise JMX files are not upwards compatible.
>> For the same reason we cannot change the names of other attributes in JMX
>> files.
>> The actual values of defaults are largely arbitrary, but are chosen to
>> be the commonest values.
>>
> I am aware of that
>
>>
>> * The defaults used when adding a new test element to a plan.
>> These can be changed at will.
>>
>
> They can if we stop adding the 3rd parameter to setProperty

If we do that for existing setProperty methods that will cause
spurious JMX file changes which could confuse users.
It will also affect some unit tests, and may make it harder to detect
unintended JMX changes.

But yes, we don't need apply defaults for new setProperty methods.
However if the field has an obvious default that seems very unlikely
to change it may be better to continue to use a default.

>>
>> Now if the default JMX value ceases to have a meaning, then we have a
>> different problem.
>>
>
> The old defaults for CookieManager have the following status:
> - HC3CookieHandler
> - default for HC4 and HC3 have different behaviour. HC3 default is closer
> to HC4 standard , but HC4 supports up to date RFC, not HC3.
>
>
>
>> In the case of class names, this is generally handled by fixing the
>> JMX file on input.
>> The code could be extended for field values, or the GUI code could
>> convert invalid values.
>>
>> >
>> >>
>> >> -    public static final String DEFAULT_IMPLEMENTATION =
>> >> HC4CookieHandler.class.getName();
>> >> +    // MUST NOT BE CHANGED
>> >> +    public static final String DEFAULT_IMPLEMENTATION =
>> >> HC3CookieHandler.class.getName();
>> >>
>> >>      public CookieManager() {
>> >>          clearCookies(); // Ensure that there is always a collection
>> >> available
>> >> @@ -119,11 +124,11 @@ public class CookieManager extends Confi
>> >>      }
>> >>
>> >>      public String getPolicy() {
>> >> -        return getPropertyAsString(POLICY,
>> >> HC4CookieHandler.DEFAULT_POLICY_NAME);
>> >> +        return getPropertyAsString(POLICY, DEFAULT_POLICY);
>> >>      }
>> >>
>> >>      public void setCookiePolicy(String policy){
>> >> -        setProperty(POLICY, policy,
>> HC4CookieHandler.DEFAULT_POLICY_NAME);
>> >> +        setProperty(POLICY, policy, DEFAULT_POLICY);
>> >>      }
>> >>
>> >>      public CollectionProperty getCookies() {
>> >>
>> >>
>> >>
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message