Re: [JEXL] binary compatibility
On 30 November 2011 22:15, henrib wrote: > I've committed the fix on the 2.0 branch - tests are OK - and if 2.1 is ever > released, this will be needed. That's not quite the fix I had in mind, also I'm not sure it addresses all the issues. I'll apply my fix to the 2.0-API branch before too long so you can see how it differs. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
I've committed the fix on the 2.0 branch - tests are OK - and if 2.1 is ever released, this will be needed. -- View this message in context: http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4125259.html Sent from the Commons - Dev mailing list archive at Nabble.com. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 30 November 2011 21:47, henrib wrote: > If we go back to pre JEXL-83 fix (protected non final strict field + setter) > and deprecate those, we can attempt releasing as 2.1 ? I think that would get us almost there. I propose to fix the strict/lenient bug in the 2.0-API-COMPAT branch and do some more tests. I think there are some other minor tweaks that might need to be made (e.g. I think some @since 2.1 markers are missing). Check plugin updates; update release notes etc. We then need to decide whether to release as 2.1-BETA or 2.1 final. It might be best to get everything ready, bar pom update and actual release creation, and then send an e-mail see if we are going to have support for the proposed release. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
If we go back to pre JEXL-83 fix (protected non final strict field + setter) and deprecate those, we can attempt releasing as 2.1 ? -- View this message in context: http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4125129.html Sent from the Commons - Dev mailing list archive at Nabble.com. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 30 November 2011 17:11, henrib wrote:
>
> About Was: Dear #{p} Doe; Now: Dear ${p} Doe;
> As stated, the issue was that preparing a deferred expression must always
> return an immediate (even composite) expression. When preparing "Dear #{p}
> ${name};" , the immediate ${name} will be evaluated - 'name' is the set of
> variables - and the preparation of the inner-deferred #{p} leads to another
> immediate ${p} thus the expression "Dear ${p} Doe; " - where the set of
> variables is 'p'.
> Since asString() must return the stringified form of the expression, it's
> pretty essential that this behavior remains as is which is correct.
Just to confirm: are you are saying that the previous behaviour of
asString was incorrect?
> About JEXL-83 - which is an issue you reported - and its fix which lead to
> the JexlThreadedArithmetic, the behavior is documented in the Javadoc @
> setLenient():
> * This method is not thread safe; it should be called as an
> optional step of the JexlEngine initialization code before expression
> creation & evaluation.
That's fine (but see below).
> * As of 2.1, you need a JexlThreadedArithmetic instance for this
> call to also modify the JexlArithmetic
> * leniency behavior.
That's the part I find problematic.
Yes, the method is not thread-safe, but under some circumstances that
was not an issue.
The method was conditionally thread-safe.
For example, creating an engine which is used in a single thread, or
even an engine which is then passed to one or more newly created
threads (and not subsequently modified). In both cases the lack of
thread-safety is not a problem with the 2.0.1 code.
> The rationale is pretty well documented in the issue itself.
> I'd vote for throwing an UnsupportedOperationException with the same message
> (or log an error if silent?) and document the change explicitly in the
> release notes.
Unfortunately this will break some applications.
The alternative (for a 2.1 release) would be to make the field
volatile. [Not sure why I did not put this in the JIRA originally].
This will be safe for use in multiple threads, with the caveat that
the behaviour is undefined if the setting is changed whilst when the
engine is busy. But that is no worse than before.
This would be sufficient for any existing use-cases, and would fix
some potential edge cases where the setting is not correctly published
to all threads.
If the user really wants to change the settings on a per-thread basis,
they they can use the new Threaded Arithmetic implementation.
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
About Was: Dear #{p} Doe; Now: Dear ${p} Doe;
As stated, the issue was that preparing a deferred expression must always
return an immediate (even composite) expression. When preparing "Dear #{p}
${name};" , the immediate ${name} will be evaluated - 'name' is the set of
variables - and the preparation of the inner-deferred #{p} leads to another
immediate ${p} thus the expression "Dear ${p} Doe; " - where the set of
variables is 'p'.
Since asString() must return the stringified form of the expression, it's
pretty essential that this behavior remains as is which is correct.
About JEXL-83 - which is an issue you reported - and its fix which lead to
the JexlThreadedArithmetic, the behavior is documented in the Javadoc @
setLenient():
* This method is not thread safe; it should be called as an
optional step of the JexlEngine initialization code before expression
creation & evaluation.
* As of 2.1, you need a JexlThreadedArithmetic instance for this
call to also modify the JexlArithmetic
* leniency behavior.
The rationale is pretty well documented in the issue itself.
I'd vote for throwing an UnsupportedOperationException with the same message
(or log an error if silent?) and document the change explicitly in the
release notes.
--
View this message in context:
http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4123859.html
Sent from the Commons - Dev mailing list archive at Nabble.com.
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 30 November 2011 15:34, henrib wrote:
> About Test org.apache.commons.jexl2.UnifiedJEXLTest that failed, the code had
> bugs and was fixed.
> 1187458 Fri Oct 21 18:40:17 CEST 2011 henrib
> Added getVariables method (similar to JexlEngine) to extract all references
> variables from an UJEXL expression;
> Fixed issue where preparing a deferred expression would not always return an
> immediate expression;
> Updated test accordingly
Yes, but I'm not clear why the asString method changes output.
Was: Dear #{p} Doe; Now: Dear ${p} Doe;
Is that essential?
If so, what happens to ${p} input?
> About Test org.apache.commons.jexl2.IssuesTest FAILED - test73, some changes
> were made on error reporting and the 2.0.1 test was weakly checking the
> exception message:
> 1182807 Thu Oct 13 14:38:05 CEST 2011 henrib
> Added JexlException.Property exception to more accurately report errors due
> to unknown or inaccessible object properties
Yes, found that out myself; not a real problem
> About Test org.apache.commons.jexl2.ArithmeticTest FAILED, these fail
> against 2.1 due to JEXL-83 fix which needs a JexlThreadedArithmetic instance
> to change the strict mode through the engine.
However, this is a big behavioural change, which does not seem to be
explicitly mentioned in the release notes.
The call to setLenient() appears to work, but no longer has the same affect.
(Does it have any affect?)
Would it not be possible to keep the old behaviour?
Or if not, perhaps the code should throw UsupportedOperationException
if the user request cannot be granted (e.g. cannot change setting).
It looks like the additional methods in the Script interface are
unlikely to cause binary compatibility problems in normal use - though
of course they will cause source incompatibility if any code tries to
implement the interface, and introspection will reveal differences.
If we can get the other compatibility issues fixed, I think it might
be OK to release 2.1 with changes to the Script interface.
Many users won't notice the difference, and if a problem is found, it
would be useful to know for an eventual 3.0 release.
However, perhaps it should be released as BETA initially.
Let's have that discussion when the other incompatibilities are sorted out.
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
About Test org.apache.commons.jexl2.UnifiedJEXLTest that failed, the code had bugs and was fixed. 1187458 Fri Oct 21 18:40:17 CEST 2011 henrib Added getVariables method (similar to JexlEngine) to extract all references variables from an UJEXL expression; Fixed issue where preparing a deferred expression would not always return an immediate expression; Updated test accordingly About Test org.apache.commons.jexl2.IssuesTest FAILED - test73, some changes were made on error reporting and the 2.0.1 test was weakly checking the exception message: 1182807 Thu Oct 13 14:38:05 CEST 2011 henrib Added JexlException.Property exception to more accurately report errors due to unknown or inaccessible object properties About Test org.apache.commons.jexl2.ArithmeticTest FAILED, these fail against 2.1 due to JEXL-83 fix which needs a JexlThreadedArithmetic instance to change the strict mode through the engine. -- View this message in context: http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4123418.html Sent from the Commons - Dev mailing list archive at Nabble.com. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 29 November 2011 16:30, sebb wrote: > On 28 November 2011 16:26, sebb wrote: >> On 28 November 2011 15:55, henrib wrote: >>> I added @since 2.1, renamed the Uberspect.getConstructor, removed final for >>> silent & strict in Interpreter (although Interpreter instances probably >>> never need to change those at runtime) but there are still 21 clirr errors >>> that I'm afraid many will consider as show-stoppers for release. >> >> Not if they can be shown to be unused by end-users. >> >>> I'm pretty sure that no active JEXL user would really be bothered by the 2.1 >>> API modifications - and even less so by the binary incompatibility - but I >>> don't see how the case can be made... >> >> JMeter uses Jexl2 - don't have time to try it now, but I will try >> tomorrow and see if it is affected by the API breaks. > > I've not found any issues - JMeter compiles and tests OK. > However, JMeter does not use many of the its features. > > A better test would be to see what happens with the JUnit test cases from > 2.0.1. > Some of these are bound to fail, but if most pass, then that may > indicate that the API is not too badly broken to release. I recompiled the 2.0.1 test classes against 2.0.1 source (unfortunately we did not release a test jar). An initial test running the 2.0.1 test classes against 2.1_SNAPSHOT (current 2.0 branch) shows only a few errors: [junit] Test org.apache.commons.jexl2.ArithmeticTest FAILED - test*Null*Operand(s) [junit] Test org.apache.commons.jexl2.IssuesTest FAILED - test73 [junit] Test org.apache.commons.jexl2.UnifiedJEXLTest FAILED - testPrepareEvaluate, testImmediate, testDeferred [junit] Test org.apache.commons.jexl2.scripting.JexlScriptEngineTest FAILED I've not fully investigated all the test failures yet. JexlScriptEngineTest - to be expected, new engine has new features. The other tests appear to show failures due behavioural change. For example: junit.framework.ComparisonFailure: expected: but was: at org.apache.commons.jexl2.UnifiedJEXLTest.testPrepareEvaluate(UnifiedJEXLTest.java:108) It's not clear if this is an intentional change or not - is this documented? It's not yet clear whether the binary API changes have affected the 2.0.1 tests or whether the tests don't rely on those parts of the API. If the latter, then this may be an indication that the API changes are unlikely to affect client code. >>> Any hint/advice/idea ? >> >> There are still a few errors that could be fixed. >> e.g. the visit() changes - would it work to re-introduce dummy >> deprecated classes ASTFloatLiteral and ASTIntegerLiteral? >> Or define them in terms of the new classes? >> >> JexlArithmetic.strict can be made non-final >> Likewise the method changes in UnifiedJEXL$Expression could be >> reverted (and pending changes flagged). >> >> I think it's now quite close, apart from the Script interface, which >> may be allowed. >> >>> I've got a migrated-to jexl3 code base ready just in case jexl2 is a >>> dead-end. > > If it does prove necessary, then we should check that there aren't any > other issues with the API that still need fixing. > > Otherwise the process will repeat ... > >>> Cheers, >>> Henrib >>> >>> >>> >>> >>> -- >>> View this message in context: >>> http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4115683.html >>> Sent from the Commons - Dev mailing list archive at Nabble.com. >>> >>> - >>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >>> For additional commands, e-mail: dev-h...@commons.apache.org >>> >>> - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 28 November 2011 16:26, sebb wrote: > On 28 November 2011 15:55, henrib wrote: >> I added @since 2.1, renamed the Uberspect.getConstructor, removed final for >> silent & strict in Interpreter (although Interpreter instances probably >> never need to change those at runtime) but there are still 21 clirr errors >> that I'm afraid many will consider as show-stoppers for release. > > Not if they can be shown to be unused by end-users. > >> I'm pretty sure that no active JEXL user would really be bothered by the 2.1 >> API modifications - and even less so by the binary incompatibility - but I >> don't see how the case can be made... > > JMeter uses Jexl2 - don't have time to try it now, but I will try > tomorrow and see if it is affected by the API breaks. I've not found any issues - JMeter compiles and tests OK. However, JMeter does not use many of the its features. A better test would be to see what happens with the JUnit test cases from 2.0.1. Some of these are bound to fail, but if most pass, then that may indicate that the API is not too badly broken to release. >> Any hint/advice/idea ? > > There are still a few errors that could be fixed. > e.g. the visit() changes - would it work to re-introduce dummy > deprecated classes ASTFloatLiteral and ASTIntegerLiteral? > Or define them in terms of the new classes? > > JexlArithmetic.strict can be made non-final > Likewise the method changes in UnifiedJEXL$Expression could be > reverted (and pending changes flagged). > > I think it's now quite close, apart from the Script interface, which > may be allowed. > >> I've got a migrated-to jexl3 code base ready just in case jexl2 is a >> dead-end. If it does prove necessary, then we should check that there aren't any other issues with the API that still need fixing. Otherwise the process will repeat ... >> Cheers, >> Henrib >> >> >> >> >> -- >> View this message in context: >> http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4115683.html >> Sent from the Commons - Dev mailing list archive at Nabble.com. >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 28 November 2011 15:55, henrib wrote: > I added @since 2.1, renamed the Uberspect.getConstructor, removed final for > silent & strict in Interpreter (although Interpreter instances probably > never need to change those at runtime) but there are still 21 clirr errors > that I'm afraid many will consider as show-stoppers for release. Not if they can be shown to be unused by end-users. > I'm pretty sure that no active JEXL user would really be bothered by the 2.1 > API modifications - and even less so by the binary incompatibility - but I > don't see how the case can be made... JMeter uses Jexl2 - don't have time to try it now, but I will try tomorrow and see if it is affected by the API breaks. > Any hint/advice/idea ? There are still a few errors that could be fixed. e.g. the visit() changes - would it work to re-introduce dummy deprecated classes ASTFloatLiteral and ASTIntegerLiteral? Or define them in terms of the new classes? JexlArithmetic.strict can be made non-final Likewise the method changes in UnifiedJEXL$Expression could be reverted (and pending changes flagged). I think it's now quite close, apart from the Script interface, which may be allowed. > I've got a migrated-to jexl3 code base ready just in case jexl2 is a > dead-end. > > Cheers, > Henrib > > > > > -- > View this message in context: > http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4115683.html > Sent from the Commons - Dev mailing list archive at Nabble.com. > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
I added @since 2.1, renamed the Uberspect.getConstructor, removed final for silent & strict in Interpreter (although Interpreter instances probably never need to change those at runtime) but there are still 21 clirr errors that I'm afraid many will consider as show-stoppers for release. I'm pretty sure that no active JEXL user would really be bothered by the 2.1 API modifications - and even less so by the binary incompatibility - but I don't see how the case can be made... Any hint/advice/idea ? I've got a migrated-to jexl3 code base ready just in case jexl2 is a dead-end. Cheers, Henrib -- View this message in context: http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4115683.html Sent from the Commons - Dev mailing list archive at Nabble.com. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
On 28 November 2011 14:35, henrib wrote: > @since, will do. > > On the Script interface, I suspect anyone implementing it derives > ExpressionImpl which does carry default implementations... That then might be an acceptable break in compatibility. > On the new Script methods (getVariables etc), I'd like to keep the API > simple and avoid multiplying interfaces (esp for 2/3 methods at a time). > > Thanks for the review The additional final qualifiers could easily be removed and the set methods deprecated. > Henrib > > -- > View this message in context: > http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4115380.html > Sent from the Commons - Dev mailing list archive at Nabble.com. > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [JEXL] binary compatibility
@since, will do. On the Script interface, I suspect anyone implementing it derives ExpressionImpl which does carry default implementations... On the new Script methods (getVariables etc), I'd like to keep the API simple and avoid multiplying interfaces (esp for 2/3 methods at a time). Thanks for the review Henrib -- View this message in context: http://apache-commons.680414.n4.nabble.com/JEXL-binary-compatibility-tp4114818p4115380.html Sent from the Commons - Dev mailing list archive at Nabble.com. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[JEXL] binary compatibility
The interface org.apache.commons.jexl2.Script has been extended with several methods. There's no default abstract implementation so I assume this will cause problems for client code. Would it be make sense to implement the new methods in a sub-interface? Or an independent interface? In particular, the Callable methods seem to be rather different. Even the parameters/variables seem to be different from the original interface. == BTW, just noticed that the new methods etc. don't have @since markers. Should really be added before release. - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
