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 Sun, 28 Feb 2016 21:25:06 GMT
On 27 February 2016 at 13:45, Philippe Mouawad
<philippe.mouawad@gmail.com> wrote:
> On Sat, Feb 27, 2016 at 2:42 PM, sebb <sebbaz@gmail.com> wrote:
>
>> 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.
>>
> I agree and this is the case for CookieManager, but at least it is safe.
>
> Let's limit ourselves for now to only CookieManager

Let's not even do that; it's unnecessary.

>
> It will also affect some unit tests, and may make it harder to detect
>> unintended JMX changes.
>>
> Yes
>
>>
>> But yes, we don't need apply defaults for new setProperty methods.
>>
> Yes
>
>
>> However if the field has an obvious default that seems very unlikely
>> to change it may be better to continue to use a default.
>>
> Yes, but the problem is how to know that.

As I just wrote, for some fields it will be obvious.
If it's not, then we don't 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.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message