Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#12231 closed enhancement (fixed)

Uniform access to HTTP resources

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 15.12
Component: Core Version:
Keywords: http compression urlconnection Cc:

Description

(This is meant as a broader follow-up of #12220.)

Currently, we have various ways to access HTTP resources. Especially, currently we have 3 implementations to follow HTTP redirects, which do not cover all possible HTTP calls.

src/org/openstreetmap/josm/tools/Utils.java
943:                    connection = openHttpConnection(new URL(connection.getHeaderField("Location")), keepAlive);

src/org/openstreetmap/josm/tools/HttpClient.java
85:                final String redirectLocation = connection.getHeaderField("Location");

src/org/openstreetmap/josm/data/cache/JCSCachedTileLoaderJob.java
327:                    urlConn = getURLConnection(new URL(urlConn.getHeaderField("Location")));
486:                urlConn = getURLConnection(new URL(urlConn.getHeaderField("Location")));

Despite having some utility functions, many classes perform operations directly on HttpURLConnections.

Inspired by https://github.com/google/google-http-java-client and our very handy ImageProvider, I implemented a HttpClient class.

My final goal is to replace all Utils.openURL* and HttpURLConnection usages with this new class.

Attachments (1)

12231-alpha.patch (23.8 KB) - added by simon04 4 years ago.

Download all attachments as: .zip

Change History (29)

Changed 4 years ago by simon04

Attachment: 12231-alpha.patch added

comment:1 Changed 4 years ago by Don-vip

Nice :) I think you can go ahead :)

comment:2 Changed 4 years ago by simon04

In 9168/josm:

see #12231 - Uniform access to HTTP resources

comment:3 Changed 4 years ago by simon04

In 9169/josm:

see #12231 - HttpClient: add support to uncompress streams

comment:4 Changed 4 years ago by simon04

In 9170/josm:

see #12231 - Deprecate Utils.open* functions

comment:5 Changed 4 years ago by simon04

In 9171/josm:

see #12231 - Use HttpClient instead of some Utils.openHttpConnection usages

comment:6 Changed 4 years ago by simon04

In 9172/josm:

see #12231 - Use HttpClient for OSM API calls

This requires adaptors to the OAuth library: SignpostAdapters

comment:7 Changed 4 years ago by simon04

In 9174/josm:

see #12231 - Update unit tests, fix regression

comment:8 Changed 4 years ago by simon04

In 9175/josm:

see #12231 - Use HttpClient instead of remaining Utils.setupURLConnection

comment:9 Changed 4 years ago by simon04

The DirectUpload plugin needs to be adapted in org.openstreetmap.josm.plugins.DirectUpload.UploadOsmConnection#addAuthHack – fixing a hack :( …

comment:10 Changed 4 years ago by Don-vip

In 9177/josm:

see #12231 - javadoc

comment:11 Changed 4 years ago by Don-vip

In 9178/josm:

see #12231 - add support for setFixedLengthStreamingMode (used in DirectUpload plugin)

comment:12 Changed 4 years ago by Don-vip

In 9179/josm:

see #12231 - initialize API to use progress monitor during HTTP connection

comment:13 in reply to:  9 ; Changed 4 years ago by Don-vip

Replying to simon04:

The DirectUpload plugin needs to be adapted in org.openstreetmap.josm.plugins.DirectUpload.UploadOsmConnection#addAuthHack – fixing a hack :( …

I have made some changes in r9178:9179 + [o31871:31872] in order to fix the build but this is not finished yet, the plugin now lacks the ability of displaying upload progress as before. Can you please look into that?

comment:14 Changed 4 years ago by Don-vip

Also, two unit tests are now failing:

MultiFetchServerObjectReaderTest

OsmServerBackreferenceReaderTest

For both of them we have:

WARNING: Already here java.net.SocketTimeoutException: Read timed out
WARNING: Already here java.net.SocketTimeoutException: Read timed out
WARNING: Already here java.net.SocketTimeoutException: Read timed out
WARNING: Already here java.net.SocketTimeoutException: Read timed out
WARNING: Already here java.net.SocketTimeoutException: Read timed out

comment:15 Changed 4 years ago by Don-vip

Cc: Don-vip removed
Summary: [Patch draft] Uniform access to HTTP resourcesUniform access to HTTP resources

comment:16 Changed 4 years ago by simon04

In 9182/josm:

see #12231 - No read timeout for OsmApi connections

Fixes regression from r9172

comment:17 in reply to:  13 Changed 4 years ago by simon04

Replying to Don-vip:

I have made some changes in r9178:9179 + [o31871:31872] in order to fix the build but this is not finished yet, the plugin now lacks the ability of displaying upload progress as before. Can you please look into that?

Wow, thank you for taking care of the plugin! So what we would need is basically the analogue of ProgressInputStream when sending data to connection.getOutputStream?

comment:18 Changed 4 years ago by Don-vip

It sounds nice, yes :)

comment:19 Changed 4 years ago by stoecker

See #12236 for a possible regression.

comment:20 Changed 4 years ago by simon04

In 9184/josm:

see #12231 - fix MultiFetchServerObjectReaderTest#multiGetWithNonExistingNode

comment:21 Changed 4 years ago by simon04

Replying to comment:18

In 9185/josm:

see #12231 - HttpClient now reports status to ProgressMonitor

Last edited 4 years ago by simon04 (previous) (diff)

comment:22 in reply to:  21 Changed 4 years ago by Don-vip

Replying to simon04:

Replying to comment:18

In 9185/josm:

see #12231 - HttpClient now reports status to ProgressMonitor

thanks! :)

comment:23 Changed 4 years ago by simon04

In 9190/josm:

see #12231 - Fix logging of reasonForRequest

comment:24 in reply to:  23 ; Changed 4 years ago by Don-vip

Replying to simon04:

In 9190/josm:

see #12231 - Fix logging of reasonForRequest

is there a reason to not use !reasonForRequest.isEmpty() ?

comment:25 Changed 4 years ago by simon04

In 9200/josm:

see #12231 - Code style (reasonForRequest)

comment:26 in reply to:  24 Changed 4 years ago by simon04

Replying to Don-vip:

is there a reason to not use !reasonForRequest.isEmpty() ?

No, I must have been confused while taking over this feature from https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/io/OsmServerReader.java?rev=9078#L153

comment:27 Changed 4 years ago by Don-vip

Resolution: fixed
Status: newclosed

comment:28 Changed 4 years ago by simon04

In 9314/josm:

fix #7094 see #12231 - HttpClient: set Content-Length for all POST/PUT/DELETE requests

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.