You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by bu...@apache.org on 2006/09/14 16:43:54 UTC
DO NOT REPLY [Bug 40512] New: - Fix incompatible API changes to TransformSpi
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
Summary: Fix incompatible API changes to TransformSpi
Product: Security
Version: unspecified
Platform: All
OS/Version: All
Status: NEW
Severity: regression
Priority: P2
Component: Signature
AssignedTo: security-dev@xml.apache.org
ReportedBy: sean.mullan@sun.com
In 1.4, a new abstract protected
enginePerformTransform(XMLSignatureInput, Transform) method was added to the
TransformSpi class and the existing enginePerformTransform(XMLSignatureInput)
method was
removed. The new method was designed to replace the old method and was
done to improve performance, which is all very good.
Unfortunately, it means that existing TransformSpi implementations when
compiled against 1.4 will not compile until the new method is
implemented. This is not a problem for the built-in implementations with
XMLSec (since they have been adjusted) but it is a problem for any
TransformSpi implementations developed by users out there. In any case,
this particular compilation failure is not such a big deal since you
will want to implement the better method anyway.
The more serious problem (IMO) is that you will get a runtime error
(NoSuchMethodError) if the current TransformSpi implementations are run
with 1.4 without updating and recompiling to support the new method
(because the new methods don't exist). So this means you cannot
seamlessly upgrade existing TransformSpi implementations to 1.4 without
reimplementing the new methods.
Suggested fix (for TransformSpi):
1. Restore old method
2. Change new method to be non-abstract and throw UnsupportedOperationExc
by default
3. Change Transform code to fallback to old method if it gets UnsuppOpExc, ex:
try {
result = transformSpi.enginePerformTransform(input,this);
} catch (UnsupportedOperationException ex) {
result = transformSpi.enginePerformTransform(input);
4. Document in release notes that developers should override new method
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
------- Additional Comments From sean.mullan@sun.com 2006-09-14 17:06 -------
> What do you think?
I think your reasons for the new method are good! But I still think we need to
preserve compatibility (esp. binary).
This change breaks binary compat in a service provider API that has been in
XMLSec since the first release. Users should be able to depend on that
API (especially since it was designed for them to plug in their own Transform
implementations) being stable over the long run. Compatibility is really
important IMHO across all APIs. We have too many users now, they need to
be able to trust us that this software will continue to work and they
don't have to keep modifying their code to keep up with the latest revisions.
And maybe we should start thinking about starting an XMLSec 2 that
cleans up some of these issues and ties better with JSR 105 and streaming
implementations :)
> The only main problem with your solution is that when someone want to create a
> Transform the compiler will not force to implement any method.
True. We could log a warning though if it throws an UnsupportedExc.
> If there is enough push for the 3. I will implement this. But I still think it
> should be better to document the change.
>
> Regards,
> Raul
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
------- Additional Comments From sean.mullan@sun.com 2006-09-25 21:10 -------
(In reply to comment #5)
> I have just made TransformSPI backward compatible. Now it is possible to use old
> implementations made for the >1.3 versions paying the performance hit of the old
> way.
> I have adapt a little the ideas above. Instead of doing the try..catch in the
> transform object I change the new method to have the implementation that call
> the old api method. And have the old method throw the NotImplemented exception.
> I have done a small test case that seems to pass but I have not test with a real
> implementation. So I wait for your feedback before closing the bug.
It looks fine and have tested with an implementation.
> The problem in keyresolver still lings on. It is a little diferent than this one.
> In KeyresolverSPI & other resolvers there were two methods boolean
> engineCanResolveX and engineResolveX. And they were called with a code like that:
> X x=null;
> while(x!=null && hasMoreResolvers) {
> if (engineCanResolveX(...)) {
> x=engineResolveX(...);
> }
> }
>
> The new code is simple and works like this:
> X x=null;
> while(x!=null && hasMoreResolvers) {
> x=engineResolveX(...);
> }
>
> It doesn't call engineCanResolve but it goes straight forward to the
> engineResolveX if it is null it keeps trying with other resolver.
> This change can impact resolvers that expect engineCanResolve be called before
> engineResolve(there is one in the old code) that now will be called always
> without engineCanResolve(indeed the engineCanResolve is no more a method of the
> resolver).
>
> SO it is a change in the contract in the api. Well, we neve document very well
> the API and this was not written but I think it follows the semantic of the
> names of the methods.
>
> We have two options.
> 1. Document the current behavior and wait and see if there are any out of the
> tree resolver that expect this behavior.
> 2. Change the name of the new method engineLookAndResolve() and let this call
> the old methods like we do in the TransformSpi.
I prefer to be on the safe side and preserve compatibility so I vote 2.
However, if the consensus is to break compatibility (1) then all of
the engineResolve methods need to changed to clearly specify that null should
be returned if the key material cannot be resolved. They do not document
that behavior right now.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
------- Additional Comments From blautenb@apache.org 2006-09-17 08:57 -------
My feeling is that if this is going to be left with version 4, then we need to
bump the version number from 1.4 to 2.0 - the general convention is that small
version changes should not break compatibility at an API level.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
------- Additional Comments From raul-info@r-bg.com 2006-09-24 17:46 -------
I have just made TransformSPI backward compatible. Now it is possible to use old
implementations made for the >1.3 versions paying the performance hit of the old
way.
I have adapt a little the ideas above. Instead of doing the try..catch in the
transform object I change the new method to have the implementation that call
the old api method. And have the old method throw the NotImplemented exception.
I have done a small test case that seems to pass but I have not test with a real
implementation. So I wait for your feedback before closing the bug.
The problem in keyresolver still lings on. It is a little diferent than this one.
In KeyresolverSPI & other resolvers there were two methods boolean
engineCanResolveX and engineResolveX. And they were called with a code like that:
X x=null;
while(x!=null && hasMoreResolvers) {
if (engineCanResolveX(...)) {
x=engineResolveX(...);
}
}
The new code is simple and works like this:
X x=null;
while(x!=null && hasMoreResolvers) {
x=engineResolveX(...);
}
It doesn't call engineCanResolve but it goes straight forward to the
engineResolveX if it is null it keeps trying with other resolver.
This change can impact resolvers that expect engineCanResolve be called before
engineResolve(there is one in the old code) that now will be called always
without engineCanResolve(indeed the engineCanResolve is no more a method of the
resolver).
SO it is a change in the contract in the api. Well, we neve document very well
the API and this was not written but I think it follows the semantic of the
names of the methods.
We have two options.
1. Document the current behavior and wait and see if there are any out of the
tree resolver that expect this behavior.
2. Change the name of the new method engineLookAndResolve() and let this call
the old methods like we do in the TransformSpi.
What do you think?
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
------- Additional Comments From raul-info@r-bg.com 2006-09-18 10:29 -------
I will add again the old methods with an empty implementation, and I will make
the new methods throw NotImplementedException and I will check this exception
and revert to the old slow method when doing the transformation.
So we have backward compatibility and the speed of the new methods. As Sean
proposed(Thanks Sean it is a good idea).
I will do the change also in KeyResolver.
The timeframe is this week. Sorry I cannot be more precise.
Thanks again for the discussion. Very enlighten
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
sean.mullan@sun.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|RESOLVED |CLOSED
------- Additional Comments From sean.mullan@sun.com 2007-09-19 12:17 -------
Closing old bugs. Fixed in 1.4.1
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 40512] - Fix incompatible API changes to TransformSpi
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=40512>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=40512
------- Additional Comments From raul-info@r-bg.com 2006-09-14 15:22 -------
Sorry for the delay too much daily work.
For me the better is the 4. Document it and let the people work.
The point 3 can be done but the solution is slightly different and can be slow.
The change comes because of the thread issues and the "slowness" of
newInstance() method. Before the change we create a transform object for every
signature to process. After the change we create only a stateless object and
reuse in all transformations.
Your solution can change slightly with(I have no compiler here so take it with a
grain of salt).
try {
result = transformSpi.enginePerformTransform(input,this);
} catch (UnsupportedOperationException ex) {
TransformSpi newTransformSpi=transformSpi.class.newInstance();
newTransformSpi.setTransfor(this);
result = newTransformSpi.enginePerformTransform(input);
}
What do you think?
The only main problem with your solution is that when someone want to create a
Transform the compiler will not force to implement any method.
If there is enough push for the 3. I will implement this. But I still think it
should be better to document the change.
Regards,
Raul
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.