iandees / josm-notes

A JOSM plugin to show OpenStreetMap Notes.
BSD 3-Clause "New" or "Revised" License
3 stars 2 forks source link

Plugin malforms https-URL #18

Closed malenki closed 10 years ago

malenki commented 10 years ago

I've set JOSM to use as API url this one: https:/api.openstreetmap.org/api/0.6/

Now JOSM does not display OSM notes anymore, instead the log at the CLI reads like this:

GET https:/api.openstreetmap.org/api/0.6/notes?bbox=13.4251141,50.6542056,13.5074831,50.6973713... GET https:/api.openstreetmap.org/api/0.6/notes?bbox=13.4251141,50.6542056,13.5074831,50.6973713... GET https:/api.openstreetmap.org/api/0.6/notes?bbox=13.4251141,50.6542056,13.5074831,50.6973713... GET https:/api.openstreetmap.org/api/0.6/notes?bbox=13.4251141,50.6542056,13.5074831,50.6973713... GET https:/api.openstreetmap.org/api/0.6/notes?bbox=13.4251141,50.6542056,13.5074831,50.6973713... GET https:/api.openstreetmap.org/api/0.6/notes?bbox=13.4251141,50.6542056,13.5074831,50.6973713... org.openstreetmap.josm.io.OsmTransferException: java.net.ConnectException: Connection refused
    at org.openstreetmap.josm.plugins.notes.api.util.NotesApi.sendRequest(NotesApi.java:364)
    at org.openstreetmap.josm.plugins.notes.api.util.NotesApi.getNotesInBoundingBox(NotesApi.java:61)
    at org.openstreetmap.josm.plugins.notes.api.DownloadAction.execute(DownloadAction.java:54)
    at org.openstreetmap.josm.plugins.notes.NotesPlugin.updateData(NotesPlugin.java:174)
    at org.openstreetmap.josm.plugins.notes.NotesPlugin$DownloadNotesTask.run(NotesPlugin.java:308)
    at java.util.TimerThread.mainLoop(Timer.java:555)
    at java.util.TimerThread.run(Timer.java:505)
Caused by: java.net.ConnectException: Connection refused
    at java.net.PlainSocketImpl.socketConnect(Native Method)
    at java.net.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:339)
    at java.net.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:200)
    at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:182)
    at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
    at java.net.Socket.connect(Socket.java:579)
    at sun.security.ssl.SSLSocketImpl.connect(SSLSocketImpl.java:618)
    at sun.net.NetworkClient.doConnect(NetworkClient.java:175)
    at sun.net.www.http.HttpClient.openServer(HttpClient.java:432)
    at sun.net.www.http.HttpClient.openServer(HttpClient.java:527)
    at sun.net.www.protocol.https.HttpsClient.<init>(HttpsClient.java:275)
    at sun.net.www.protocol.https.HttpsClient.New(HttpsClient.java:371)
    at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.getNewHttpClient(AbstractDelegateHttpsURLConnection.java:191)
    at sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:932)
    at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:177)
    at sun.net.www.protocol.https.HttpsURLConnectionImpl.connect(HttpsURLConnectionImpl.java:153)
    at org.openstreetmap.josm.plugins.notes.api.util.NotesApi.sendRequest(NotesApi.java:289)
    ... 6 more
iandees commented 10 years ago

Note that after some conversation on IRC, the URL was set correctly to https://api.openstreetmap.org/api/. The notes plugin is mangling the URL to work around a rails bug that is removing the second slash after https:.

don-vip commented 10 years ago

Can't you use core functions to deal with API requests?

iandees commented 10 years ago

Core functions don't support notes, so we extended OsmApi to support notes. This bit of code that generates the (broken) base URL is also used in the JOSM code, so I'm wondering how JOSM works at all...

don-vip commented 10 years ago

The bug is in NotesApi.getBaseUrl() it has been fixed in core some days ago: https://josm.openstreetmap.de/changeset/6840/josm/

This is a very bad practive to copy/paste core functions in plugins instead of calling them. If you can't, please create a core enhancement ticket and we'll change visibility.

iandees commented 10 years ago

I would prefer that the notes plugin work for as many JOSM users as possible. Forcing everyone to update to some future version of JOSM where you've fixed the visibility of sendRequest() seems silly.

don-vip commented 10 years ago

I don't find this silly. I can show you statistics and you will see the vast majority of users run at least the current tested version. Many people use WebStart, a lot of others are accustomed to manually update their version. Don't forget also the previous versions of each plugin are remembered and delivered to users still running an old version of JOSM, so that's not a problem at all. It's even a good thing as it may encourage those users to upgrade their JOSM if they want to use your very latest features.

I fully understand you need to extend OsmApi for notes-specific functions, but duplicating deep core features such as API request sending mechanism is really not a good idea.

But it's your call. We can help you to achieve a better integration, unless you don't want to.

iandees commented 10 years ago

I think @ToeBee was thinking about submitting a patch to add notes support to OsmApi in core. In the mean time, I'll file a JOSM trac ticket to ask for a sendRequest() to change to protected.

ToeBee commented 10 years ago

Yes, my goal has been to improve the notes API code to the point where it can be included in core. But that hasn't happened yet. I think my last hangup was something to do with the progress monitor stuff...

don-vip commented 10 years ago

sendRequest() is now protected :)

ToeBee commented 10 years ago

I'm guessing we need to make the plugin depend on >= 6891 to fix this?

don-vip commented 10 years ago

6875 is enough: https://josm.openstreetmap.de/changeset/6875/josm/