It looks good to me. Thanks, Regards JB
On Tue, Jan 16, 2024 at 3:38 PM Christopher Shannon <christopher.l.shan...@gmail.com> wrote: > > As background, last week it was discovered (thanks Justin) that there was > an oversight with the Exception handling over OpenWire with the upgrade to > Jakarta apis in the 6.0.x broker. > https://issues.apache.org/jira/browse/AMQ-9418 > > The problem is that OpenWire has a response type to return an exception to > the client and this response type includes the actual throwable type > written as a string with the fully qualified package. This means that if an > older 5.x client that is using the javax JMS apis receives back something > like InvalidClientIDException from a 6.x broker the exception type received > is actually going to be "jakarta.jms.InvalidIDException" and then the > client can't unmarshall that since it's likely not on the classpath. The > result is that if it converts it will get a ClassNotFoundException and just > convert to a generic exception type so this could be a breaking change for > clients that are trying to handle specific exceptions. Note that the > reverse is actually also true, if a new 6.x client connected to a 5.x > broker the same problem happens. > > I've been looking at ways to fix this broker side and it's not great. We > would need to detect if the client wants jakarta or javax types (we can > kind of do this for newer clients because > https://issues.apache.org/jira/browse/AMQ-6379 has new clients set their > actual client version from the manifest file in the jar) and then we have > to pass that to the marshallers and signal in some way to convert. The > main problem is having to pass it to the marshaller means probably having > to update the Openwire format to include the version discovered and having > complicated detection logic depending on if it is set or not as not > everything sets it. If we didn't want to do it at the marshaller level, > we'd have to include or repackage both versions of the Exceptions which is > also not ideal as having to include both javax and jakarta exception types > could cause mistakes elsewhere and maintaining isn't fun. > > I think a much simpler approach is to just let the receiver (usually > client) handle it. I think instead of trying to have the broker detect what > to send, it should be handled on the unmarshal side of things by the > receiver of the error command. The receiver (client or broker) knows if it > will need to convert so it makes things much simpler. Obviously this would > only work if we update the marshaller in the client code jar so my proposal > would be to just release a small update in the next version of 5.18.x > client and also 6.0.x clients to handle this. We can just document saying > that if you need to have specific exception type translation and are using > a broker and client that are not both javax or jakarta then you need to > upgrade to a new client version for the translation. Otherwise they can > just get the generic Throwable back and will work. The main use case I > think is if clients are stuck on older versions of Java and can't upgrade > yet when talking to a new broker or an older broker can't be upgraded yet > but the clients can. > > Here is an example of fixing the 5.18.x client: > https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4 > It's pretty simple, if we get back jakarta then convert to javax and done. > We would do the reverse in 6.x branch. > > Going forward as a permanent fix, we should just fix this entirely in > OpenWire v13. Including the Java exception types already caused a major CVE > issue and now it's causing issues here. I think for the next OpenWire > version we should create a new Exception type that uses an Error code just > like Artemis and other projects do and then the client can just take the > code and convert it to whatever exception it wants. We could either include > the error handling change by itself in OpenWire v13 so we could even > upgrade older 5.x brokers with it or just include the new openwire > exception handling only for 6.x along with shared sub OpenWire changes. > > Thoughts?
