-
Notifications
You must be signed in to change notification settings - Fork 1.1k
INT-3383: Update to Smack 4.0.0 #1137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@Flowdalic, before we start review it we should be sure that you signed the CLA. Thank you, anyway! |
|
@artembilan Just siged the CLA so I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement. I have no strong opinion about when Smack is updated in spring-integration. Just wanted to point out that 1. there is a newer version with important security fixes and 2. this version is now also available from maven central. The changes to spring itself are minimal, but users of Smack should note that Smack 4.0 comes with a slightly changed API. The important thing is that spring-integration will switch from the (very) old currently used Smack version to 4.0 at some point in the future. |
|
Thanks for this; we can't release 4.0 GA depending on an RC, but we can certainly test with it (travis is building this PR now as I "speak"). If all is well, we can target the update to 4.0.1, unless smack is released by the end of the week (or Monday at the latest). |
|
Updated to use Smack 4.0.0-rc2. Note that some spring-integration unit tests fail because spring-ws is not yet upgraded to Smack 4.0. See also spring-projects/spring-ws#23 I'm not sure how this kind of dependency updates is usually handled in spring, so please advise. Thank you :) |
The family of Message.(set|get)Properties methods has been factored out of Smack as an extra extension. Use JivePropertiesManager as replacement. It is no longer necessary to supress Smack path warnings. Delete custom smack-config.xml, as all it did was disabling certain (important) SASL mechanisms. Also remove the unit test that verified this. Some unit tests mock ChatManager and Chat for no reason, delete those lines. XMPPConnection.getHost() and .getPort() are only valid after the connection got established, due the fact that those information is often only available after a DNS SRV lookup was performed. Remove the lines in unit tests that verified their value. Remove the truststorePath example in documentation, since Smack 4.0 no longer provides support for providing a custom truststore. Instead ConnectionConfiguration in Smack 4.0 allows to set a custom SSLContext (which then again can provide a TrustManager that uses a custom truststore).
|
@Flowdalic, thank you for updating the PR. Starting to review... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I see there is an issue with new Smack and old Spring WS.
Is Smack 4.0 compatible with a Smack 3.0? I mean can we have in the classpath both versions?
I'm not sure how quickly the Spring WS (2.3.0) with new Smack will be released so we should be carefull with the new Smack for SI and old one for WS, because we can't release our project with a milestone dependency.
Just in case: can we live with both Smacks in the CP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need some kind of isolation to have two different versions of the same library loaded (like OSGi). Not sure if this is true or possible in Spring. But even if you could do it, then there is also the API change from Smack 3 to 4. How should the user/JVM decide which version to load?
That's actually what I tried to explain in my JIRA comment: I don't know how the Spring project handles these situations, as it has many release lines, other stuff and sure some policies. All I can say is that the Smack API in spring-integration and spring-ws needs to be compatible. So it's either Smack 3 for both or Smack 4. How you achieve that is up to you.
BTW: I'm not sure if those inter-dependencies are a good design, as it causes situations like we have now, but hadn't time yet to look closer at it.
|
That's all. Thanks |
|
OK, @Flowdalic, nothing to complain about. Thank you! |
|
Merged via 714db85 Thanks you! |
It is no longer necessary to supress Smack path warnings.
Delete custom smack-config.xml, as all it did was disabling
certain (important) SASL mechanisms. Also remove the unit test that
verified this.
Some unit tests mock ChatManager and Chat for no reason, delete
those lines.
XMPPConnection.getHost() and .getPort() are only valid after the
connection got established, due the fact that those information is
often only available after a DNS SRV lookup was performed. Remove the
lines in unit tests that verified their value.
Remove the truststorePath example in documentation, since Smack 4.0 no
longer provides support for providing a custom truststore. Instead
ConnectionConfiguration in Smack 4.0 allows to set a custom SSLContext
(which then again can provide a TrustManager that uses a custom
truststore).