Bugzilla – Full Text Bug Listing
| Summary: | Trac connector: update dependency to Apache XML-RPC 3.0 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Steffen Pingel <steffen.pingel> | ||||||||
| Component: | Mylyn | Assignee: | Steffen Pingel <steffen.pingel> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P1 | CC: | nhapke, pombredanne, robert.elves | ||||||||
| Version: | unspecified | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Steffen Pingel
I assume that we want to add the xmlrpc-common and xmrpc-client JARs, but do not need xmlrpc-server? Please provide me a link to the wc-common JAR as well and I'll add them all to context.core. The download is here: http://www.apache.org/dist/ws/commons/util/binaries/ws-commons-util-1.0.1-bin.tar.gz Yes, xmlrpc-server-3.0b1.jar is not required. So far I've made little progress getting the new version of XML-RPC to work. It looks like basic authentication does not work correctly when HttpClient is used for the underlying transport mechanism. Yikes. I hope that there aren't any showstoppers. On the bright side this is time well spent since it is good for us to be on the latest version of the APIs. Keep me posted on how it goes... Yes, the API has changed quite a bit. The good news is that the test cases now run cleanly (except for some logging output in case of expected authentication failures which needs to be turned off). The bad news is that I used reflection to access private fields of the transport layer of the XML-RPC library. I need to look into this further. I am either doing something horribly wrong or I need to submit a patch to make these fields accessible. Hopefully that sort of reflective access can be avoided, let me know how it goes. I committed the libraries, so I'll await a patch that removes the dependency on your org.apache.xmlrpc plug-in and then we should be ready for another dev build. Created attachment 46883 [details] Updates Trac connector dependency to Apache XML-RPC 3.0 Apache HttpClient is now used as a transport. Unfortunatelly the implementation is not very clean. I am leaning towards using HttpClient as a default for all http connections. This will improve password authentication (bug 151896) and will generally provide a richer set of features than URLConnection. Am I overlooking any reason why URLConnection should be preferred? Mik, please have a look at MylarTracPlugin.start(). It contains the initialization for HttpClient. With the current implementation of the XML-RPC library I can't register the protocol per request. I am not sure how bad this is and where that kind of initialization should actually be done (Mylar context core?). Looks like htis approach is going to work. Yes, I think you should feel free to use HttpClient for all connection, just stick what you need into UrlConnectionUtil (and rename that class accordingly). Rob: if you have any reasons why URLConnection might be preferred please state them here. Steffen: what I'm concerned about is that you're not doing this per-host, so plug-ins like WTP that connect to other servers could have trouble with this. Could you take the approach of BugzillaAttachmentHandler.uploadAttachment(..) and do it per-server lazily as you need to connect? You should move those getDomain(..) and getSslPort(..) methods into UrlConnectionUtil. Steffen: AllTracTests pass for me with this patch, but I'll hold off on applying it until the above is done. (In reply to comment #7) > Steffen: what I'm concerned about is that you're not doing this per-host, so > plug-ins like WTP that connect to other servers could have trouble with this. Doesn't the Eclipse classloader maintain separate instances of the library for different plug-ins? > Could you take the approach of BugzillaAttachmentHandler.uploadAttachment(..) > and do it per-server lazily as you need to connect? You should move those > getDomain(..) and getSslPort(..) methods into UrlConnectionUtil. Alright, I tried that before but failed to get it to work in the end. I'll try again :). Re: comment#7 My understanding is that HttpClient has a superset of the functionality provided by java.net so may be the best way to go. Created attachment 46898 [details]
Removed global protocol handler
I have done the refactoring as recommended. Please have a look at the changes to BugzillaAttachmentHandler. I am not sure if this is all correct. It would be nice to have test cases for https (would it possible to setup https for mylar.eclipse.org?).
Please let me know if the patch does not apply/compile cleanly. The persistance of change sets does not work correctly for me which makes it hard to maintain multiple pending patches (I usually have to assign files manually to the change sets).
Checking patch now... We have a test: BugzillaRepositoryConnectorTest.testTrustAllSslProtocolSocketFactory Regarding seperate instances of the library, I'm not sure, but I think that this only happens if a plug-in is not declared to be a singleton (and AbstractUIPlugin's are singletons). Plug-ins declaring extension points have to be singletons, so context.core is one. Another reason why it would be better to have an apache dependencies plug-in, which we should get at some point from project Orbit. Patch applied and reviewed. I still plan to do some manual testing of the Bugzilla side of the SSL stuff. Changes:
* for now I moved the System.setProperty("org.apache.commons.logging.Log"..) stuff to a static initializer on UrlConnectionUtil
* Did the following rename in the Trac manifest: ..mylar.trac -> ..mylar.trac.ui, since the core/ui split is impending (we should do it when we move mylar.trac out of the sandbox)
* Renamed MylarTracPlugin to TracUiPlugin to follow the new convention
* Added AllTracTests to AllTests
* Added an @author tag for you to UrlConnectionUtil. Remember to add these when you make significant changes to a class.
(In reply to comment #12) > We have a test: > BugzillaRepositoryConnectorTest.testTrustAllSslProtocolSocketFactory As far as I can tell the test case only opens a socket but does not actually verify the certificate (it couldn't since there is none at mylar.eclipse.org). Since the https part is a bit tricky for Trac a few test cases that exchange data would be nice to have. Created attachment 46922 [details]
add UseTLS option to UrlConnectionUtil
Using TLS for the SSLContext breaks bugzilla attachment retrieval over HTTPS with invalid certificates. I added the parameter to allow usage of "TLS" or "SSL" (as bugzilla needs), though I wasn't able to find usage of the Util class outside of bugzilla. Hopefully this doesn't break any of the trac connector.
Trac now exclusively relies on HttpClient for all http connections so this should not cause any breakage. Nathan's patch applied, since this parameter seems generally useful. I changed the class name to WebClientUtil since it encapsulates both HttpClient's and URLConnection's. |