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: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http: control/CookieManager.java gui/CookiePanel.java
Date Mon, 29 Feb 2016 15:25:41 GMT
On 29 February 2016 at 12:27, Philippe Mouawad
<philippe.mouawad@gmail.com> wrote:
> Greetings sebb,
>
> I reviewed the code commited in trunk and sorry, I don't find it clear and
> it does not suit me.

Have you reviewed the patch?

Trunk does not have the descriptive comments yet.

> For me it is an issue that Default policy (
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java#L107)
> and implementation (
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java#L110)
> in CookieManager are not the real defaults and that the GUI holds the real
> defaults (
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java#L90,
> https://github.com/apache/jmeter/blob/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java#L95),
> for me:
>
>    - it makes code "ugly", a field named DEFAULT_POLICY in the model is NOT
>    the default policy ?, same for DEFAULT_IMPLEMENTATION ? really strange

Have you read the patch which clearly documents what the fields mean?

>    - it breaks MVC pattern as it's the GUI which overrides the Model

It *IS* the GUI which applies the defaults.
This is only exposed here because the GUI default has been changed
from the JMX default.

> So -1 for me.

We cannot change the public DEFAULT constant names or values in
CookieManager without breaking compatibility.

Please review the patch documentation and see if that answers your complaints.

> Regards
>
> Philippe
>
> On Mon, Feb 29, 2016 at 12:36 AM, sebb <sebbaz@gmail.com> wrote:
>
>> OK, I have reverted the commits to CookieManager and CookiePanel.
>>
>> I reapplied your commit to CookiePanel.
>> In the process I found another bug in CookiePanel - not all the
>> references to the DEFAULT_POLICY were correctly updated.
>> I also extracted the default implementation as a constant and added
>> Javadoc to both defaults.
>>
>> My suggested patch for CookieManager is as follows.
>>
>> Does that make it clear enough what the fields mean?
>>
>>
>> Index: CookieManager.java
>> ===================================================================
>> --- CookieManager.java    (revision 1732815)
>> +++ CookieManager.java    (working copy)
>> @@ -102,11 +102,28 @@
>>
>>      private transient CollectionProperty initialCookies;
>>
>> -    // MUST NOT BE CHANGED
>> -    @SuppressWarnings("deprecation") // cannot be changed
>> +    /**
>> +     * Defines the policy that is assumed when the JMX file does not
>> contain an entry for it
>> +     * MUST NOT BE CHANGED otherwise JMX files will not be correctly
>> interpreted
>> +     * <p>
>> +     * The default policy for new CookieManager elements is defined by
>> +     * {@link
>> org.apache.jmeter.protocol.http.gui.CookiePanel#DEFAULT_POLICY
>> CookiePanel#DEFAULT_POLICY}
>> +     *
>> +     * @deprecated not intended for use outside this class (should
>> have been private)
>> +     */
>> +    @Deprecated
>>      public static final String DEFAULT_POLICY =
>> CookieSpecs.BROWSER_COMPATIBILITY;
>>
>> -    // MUST NOT BE CHANGED
>> +    /**
>> +     * Defines the implementation that is assumed when the JMX file
>> does not contain an entry for it
>> +     * MUST NOT BE CHANGED otherwise JMX files will not be correctly
>> interpreted
>> +     * <p>
>> +     * The default implementation for new CookieManager elements is
>> defined by
>> +     * {@link
>> org.apache.jmeter.protocol.http.gui.CookiePanel#DEFAULT_IMPLEMENTATION
>> CookiePanel#DEFAULT_IMPLEMENTATION}
>> +     *
>> +     * @deprecated not intended for use outside this class (should
>> have been private)
>> +     */
>> +    @Deprecated
>>      public static final String DEFAULT_IMPLEMENTATION =
>> HC3CookieHandler.class.getName();
>>
>>      public CookieManager() {
>>
>>
>>
>> On 28 February 2016 at 22:43, Philippe Mouawad
>> <philippe.mouawad@gmail.com> wrote:
>> > On Sunday, February 28, 2016, sebb <sebbaz@gmail.com> wrote:
>> >
>> >> On 28 February 2016 at 21:51, Philippe Mouawad
>> >> <philippe.mouawad@gmail.com <javascript:;>> wrote:
>> >> > Hi sebb,
>> >> > Could you make a Patch to see what final code will look like ?
>> >>
>> >> Not until you revert the commit.
>> >
>> >
>> > That's funny.
>> >
>> > do it if you want, I have no time to lose reverting code that works and
>> is
>> > readable.
>> > I don't see under what particular conditions you cannot make a patch and
>> I
>> > would have to revert my work
>> >
>> >
>> >
>> >>
>> >> > Frankly I don't understand why you want me to revert, there is no bug
>> >> and I
>> >> > think code is easier to read and understand (without documentation).
>> >>
>> >> The commit included two separate changes AND it changed constants.
>> >
>> >
>> > No, the aim was to fix and cleanup code which was for me unreadable.
>> >
>> > You can disagree but it does not mean you are right
>> >
>> >
>> >>
>> >> Please now revert the commit r1732634
>> >
>> >
>> >  Feel free to do it.
>> >
>> >
>> >
>> >> > What constants did I rename ? I introduced new ones.
>> >>
>> >> DEFAULT_POLICY => POLICY_FOR_BACKWARD_COMPATIBILITY
>> >
>> >
>> > I then reintroduced Default_policy with the real default policy.
>> >
>> >
>> >> > We can change their values , I cannot imagine Defaults will remain
as
>> is
>> >> > for years.
>> >>
>> >> No we cannot, because that changes the way JMXs are interpreted
>> >
>> >
>> > Not acceptable for me.
>> > I remember we already did it in the past.
>> > One thing I remember are the defaults of transaction controller,
>> processing
>> > time inclusion and generate parent sample.
>> > At least 2 or 3 changes
>> >
>> >
>> >>
>> >> > We can change their values, because we created a 3.0 and can introduce
>> >> > breaking changes, knowing that we document a lot all breaking changes
>> (I
>> >> > don't see many project who care so much about documenting all the
>> >> changes).
>> >> >
>> >> > Regards
>> >> > Philippe
>> >> >
>> >> >
>> >> > On Sun, Feb 28, 2016 at 10:44 PM, sebb <sebbaz@gmail.com
>> <javascript:;>>
>> >> wrote:
>> >> >
>> >> >> My objection is two-fold:
>> >> >>
>> >> >> Public constants must not be renamed, nor (in general) can their
>> >> >> values be changed.
>> >> >>
>> >> >> The fix to CookiePanel is a separate bug: i.e. the implementation
>> must
>> >> >> be set before the policy.
>> >> >> It's basically a continuation of the fix I made in r1732595 (I
forgot
>> >> >> to check for other instances of the wrong ordering)
>> >> >> It does not depend on the change to CookieManager; in fact that
>> change
>> >> >> does not affect it at all because DEFAULT_IMPLEMENTATION is defined
>> in
>> >> >> CookiePanel.
>> >> >>
>> >> >> So please revert the commit.
>> >> >>
>> >> >> You can then reapply the change to CookiePanel.
>> >> >>
>> >> >> I will then update CookieManager to better document the constant.
>> >> >>
>> >> >>
>> >> >> On 27 February 2016 at 13:58, Philippe Mouawad
>> >> >> <philippe.mouawad@gmail.com <javascript:;>> wrote:
>> >> >> > On Sat, Feb 27, 2016 at 2:52 PM, sebb <sebbaz@gmail.com
>> >> <javascript:;>> wrote:
>> >> >> >
>> >> >> >> Also the commit fixed a bug and made some unrelated changes.
>> >> >> >>
>> >> >> >> Don't mix separate fixes.
>> >> >> >>
>> >> >> >> It's much harder to review and to follow the history later.
>> >> >> >>
>> >> >> >> Please revert and re-apply the change to CookiePanel separately.
>> >> >> >>
>> >> >> > It is part of the fix.
>> >> >> >
>> >> >> >>
>> >> >> >> I don't agree with the change to CookieManager; that needs
further
>> >> >> >> discussion.
>> >> >> >>
>> >> >> >
>> >> >> > Based  on my last commit few minutes ago, can you open a new
>> >> discussion
>> >> >> (as
>> >> >> > subject is not clear) to mention what you disagree with ?
>> >> >> > Thanks
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >> On 27 February 2016 at 13:45, sebb <sebbaz@gmail.com
>> <javascript:;>>
>> >> wrote:
>> >> >> >> > -1
>> >> >> >> >
>> >> >> >> > Must not change public constant names.
>> >> >> >> >
>> >> >> >> > If the name is not ideal, fix this by adding Javadoc,
not a
>> rename.
>> >> >> >> >
>> >> >> >> > On 27 February 2016 at 12:53,  <pmouawad@apache.org
>> <javascript:;>>
>> >> wrote:
>> >> >> >> >> Author: pmouawad
>> >> >> >> >> Date: Sat Feb 27 12:53:18 2016
>> >> >> >> >> New Revision: 1732634
>> >> >> >> >>
>> >> >> >> >> URL: http://svn.apache.org/viewvc?rev=1732634&view=rev
>> >> >> >> >> Log:
>> >> >> >> >> Bug 58756 - CookieManager : Cookie Policy select
box content
>> must
>> >> >> >> depend on Cookie implementation
>> >> >> >> >> A) Fix bug introduced by commit r1732632:
>> >> >> >> >> 1/ Save with 2.13 a Plan containing CookieManager
that uses
>> >> defaults
>> >> >> >> >> 2/ Open it with trunk
>> >> >> >> >> 3/ It is ok
>> >> >> >> >> 4/ Close it
>> >> >> >> >> 5/ Add a new CookieManager => KO the policy
is default instead
>> of
>> >> >> >> standard
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> B) Also made constants explicit and removed dependency
on
>> >> deprecated
>> >> >> >> class.
>> >> >> >> >>
>> >> >> >> >> C) Forced the save of policy and implementation
even if there
>> >> values
>> >> >> >> are set at defaults to avoid this headache again
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> Bugzilla Id: 58756
>> >> >> >> >>
>> >> >> >> >> Modified:
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CookieManager.java
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.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=1732634&r1=1732633&r2=1732634&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 12:53:18 2016
>> >> >> >> >> @@ -30,7 +30,6 @@ import java.io.Serializable;
>> >> >> >> >>  import java.net.URL;
>> >> >> >> >>  import java.util.ArrayList;
>> >> >> >> >>
>> >> >> >> >> -import org.apache.http.client.config.CookieSpecs;
>> >> >> >> >>  import org.apache.jmeter.config.ConfigTestElement;
>> >> >> >> >>  import org.apache.jmeter.engine.event.LoopIterationEvent;
>> >> >> >> >>  import org.apache.jmeter.testelement.TestIterationListener;
>> >> >> >> >> @@ -102,12 +101,19 @@ public class CookieManager
extends Confi
>> >> >> >> >>
>> >> >> >> >>      private transient CollectionProperty initialCookies;
>> >> >> >> >>
>> >> >> >> >> -    // MUST NOT BE CHANGED
>> >> >> >> >> -    @SuppressWarnings("deprecation") // cannot
be changed
>> >> >> >> >> -    public static final String DEFAULT_POLICY
=
>> >> >> >> CookieSpecs.BROWSER_COMPATIBILITY;
>> >> >> >> >> +    // MUST NOT BE CHANGED because as defaults
are not saved,
>> >> >> >> >> +    // when a Test Plan was loaded from an N-1
version and if
>> >> >> defaults
>> >> >> >> changed,
>> >> >> >> >> +    // you end up changing the previously set
policy
>> >> >> >> >> +    // see issues with Bug 58756
>> >> >> >> >> +    public static final String
>> POLICY_FOR_BACKWARD_COMPATIBILITY
>> >> =
>> >> >> >> "compatibility";
>> >> >> >> >>
>> >> >> >> >> -    // MUST NOT BE CHANGED
>> >> >> >> >> -    public static final String DEFAULT_IMPLEMENTATION
=
>> >> >> >> HC3CookieHandler.class.getName();
>> >> >> >> >> +    // MUST NOT BE CHANGED because as defaults
are not saved,
>> >> >> >> >> +    // when a Test Plan was loaded from an N-1
version and if
>> >> >> defaults
>> >> >> >> changed,
>> >> >> >> >> +    // you end up changing the previously set
policy
>> >> >> >> >> +    // see issues with Bug 58756
>> >> >> >> >> +    public static final String
>> >> >> >> IMPLEMENTATION_FOR_BACKWARD_COMPATIBILITY =
>> >> >> >> HC3CookieHandler.class.getName();
>> >> >> >> >> +
>> >> >> >> >> +    public static final String DEFAULT_IMPLEMENTATION
=
>> >> >> >> HC4CookieHandler.class.getName();
>> >> >> >> >>
>> >> >> >> >>      public CookieManager() {
>> >> >> >> >>          clearCookies(); // Ensure that there
is always a
>> >> collection
>> >> >> >> available
>> >> >> >> >> @@ -124,11 +130,13 @@ public class CookieManager
extends Confi
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>      public String getPolicy() {
>> >> >> >> >> -        return getPropertyAsString(POLICY, DEFAULT_POLICY);
>> >> >> >> >> +        return getPropertyAsString(POLICY,
>> >> >> >> POLICY_FOR_BACKWARD_COMPATIBILITY);
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>      public void setCookiePolicy(String policy){
>> >> >> >> >> -        setProperty(POLICY, policy, DEFAULT_POLICY);
>> >> >> >> >> +        // we must explicitely save the policy
>> >> >> >> >> +        // not use a default implementation
>> >> >> >> >> +        setProperty(POLICY, policy);
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>      public CollectionProperty getCookies() {
>> >> >> >> >> @@ -148,11 +156,13 @@ public class CookieManager
extends Confi
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>      public String getImplementation() {
>> >> >> >> >> -        return getPropertyAsString(IMPLEMENTATION,
>> >> >> >> DEFAULT_IMPLEMENTATION);
>> >> >> >> >> +        return getPropertyAsString(IMPLEMENTATION,
>> >> >> >> IMPLEMENTATION_FOR_BACKWARD_COMPATIBILITY);
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>      public void setImplementation(String implementation){
>> >> >> >> >> -        setProperty(IMPLEMENTATION, implementation,
>> >> >> >> DEFAULT_IMPLEMENTATION);
>> >> >> >> >> +        // we must explicitely save the policy
>> >> >> >> >> +        // not use a default implementation
>> >> >> >> >> +        setProperty(IMPLEMENTATION, implementation);
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>      /**
>> >> >> >> >>
>> >> >> >> >> Modified:
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
>> >> >> >> >> URL:
>> >> >> >>
>> >> >>
>> >>
>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java?rev=1732634&r1=1732633&r2=1732634&view=diff
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>> ==============================================================================
>> >> >> >> >> ---
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
>> >> >> >> (original)
>> >> >> >> >> +++
>> >> >> >>
>> >> >>
>> >>
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/gui/CookiePanel.java
>> >> >> >> Sat Feb 27 12:53:18 2016
>> >> >> >> >> @@ -280,9 +280,9 @@ public class CookiePanel
extends Abstrac
>> >> >> >> >>
>> >> >> >> >>          tableModel.clearData();
>> >> >> >> >>          clearEachIteration.setSelected(false);
>> >> >> >> >> -        policy.setText(HC4CookieHandler.DEFAULT_POLICY_NAME);
>> >> >> >> >>
>> selectHandlerPanel.setSelectedItem(DEFAULT_IMPLEMENTATION
>> >> >> >> >>
>> >> .substring(DEFAULT_IMPLEMENTATION.lastIndexOf('.') +
>> >> >> >> 1));
>> >> >> >> >> +        policy.setText(HC4CookieHandler.DEFAULT_POLICY_NAME);
>> >> >> >> >>          deleteButton.setEnabled(false);
>> >> >> >> >>          saveButton.setEnabled(false);
>> >> >> >> >>      }
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Cordialement.
>> >> >> > Philippe Mouawad.
>> >> >>
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Cordialement.
>> >> > Philippe Mouawad.
>> >>
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message