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 2016/02/27 13:53:18 UTC
svn commit: r1732634 - in
/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
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);
}
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by sebb <se...@gmail.com>.
On 29 February 2016 at 12:27, Philippe Mouawad
<ph...@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 <se...@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
>> <ph...@gmail.com> wrote:
>> > On Sunday, February 28, 2016, sebb <se...@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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by Philippe Mouawad <ph...@gmail.com>.
Greetings sebb,
I reviewed the code commited in trunk and sorry, I don't find it clear and
it does not suit me.
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
- it breaks MVC pattern as it's the GUI which overrides the Model
So -1 for me.
Regards
Philippe
On Mon, Feb 29, 2016 at 12:36 AM, sebb <se...@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
> <ph...@gmail.com> wrote:
> > On Sunday, February 28, 2016, sebb <se...@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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by sebb <se...@gmail.com>.
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
<ph...@gmail.com> wrote:
> On Sunday, February 28, 2016, sebb <se...@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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by Philippe Mouawad <ph...@gmail.com>.
On Sunday, February 28, 2016, sebb <se...@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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by sebb <se...@gmail.com>.
On 28 February 2016 at 21:51, Philippe Mouawad
<ph...@gmail.com> wrote:
> Hi sebb,
> Could you make a Patch to see what final code will look like ?
Not until you revert the commit.
> 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.
Please now revert the commit r1732634
> What constants did I rename ? I introduced new ones.
DEFAULT_POLICY => POLICY_FOR_BACKWARD_COMPATIBILITY
> 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
> 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 <se...@gmail.com> 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
>> <ph...@gmail.com> wrote:
>> > On Sat, Feb 27, 2016 at 2:52 PM, sebb <se...@gmail.com> 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 <se...@gmail.com> 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, <pm...@apache.org> 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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi sebb,
Could you make a Patch to see what final code will look like ?
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).
What constants did I rename ? I introduced new ones.
We can change their values , I cannot imagine Defaults will remain as is
for years.
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 <se...@gmail.com> 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
> <ph...@gmail.com> wrote:
> > On Sat, Feb 27, 2016 at 2:52 PM, sebb <se...@gmail.com> 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 <se...@gmail.com> 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, <pm...@apache.org> 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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by sebb <se...@gmail.com>.
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
<ph...@gmail.com> wrote:
> On Sat, Feb 27, 2016 at 2:52 PM, sebb <se...@gmail.com> 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 <se...@gmail.com> 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, <pm...@apache.org> 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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by Philippe Mouawad <ph...@gmail.com>.
On Sat, Feb 27, 2016 at 2:52 PM, sebb <se...@gmail.com> 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 <se...@gmail.com> 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, <pm...@apache.org> 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.
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by sebb <se...@gmail.com>.
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.
I don't agree with the change to CookieManager; that needs further discussion.
On 27 February 2016 at 13:45, sebb <se...@gmail.com> 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, <pm...@apache.org> 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);
>> }
>>
>>
Re: svn commit: r1732634 - in /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http:
control/CookieManager.java gui/CookiePanel.java
Posted by sebb <se...@gmail.com>.
-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, <pm...@apache.org> 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);
> }
>
>