Skip to content
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

providing the /contributions response as a downloadable file #78

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

FabiKo117
Copy link
Contributor

@FabiKo117 FabiKo117 commented Nov 12, 2020

adds a respective header so that the response is provided as a downloadable file

@FabiKo117 FabiKo117 requested a review from tyrasd November 12, 2020 12:03
@FabiKo117
Copy link
Contributor Author

I'd suggest to also make a 1.2.2 release then after this commit, as the compression of the response is somewhat important to avoid that tools like swagger have to deal with responses that are too big.

pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good, but the pom's version in master must always be a SNAPSHOT.

Btw: why don't we just gzip everything? I mean, for the smaller amounts of data returned by the aggregation endpoints it doesn't do much, but it also doesn't hurt also 🤷

//edit:

I'd suggest to also make a 1.2.2 release then after this commit

👍

pom.xml Outdated Show resolved Hide resolved
@tyrasd
Copy link
Member

tyrasd commented Nov 12, 2020

PS: could it be that you accidentally started this branch off of the 1.2 branch? IMHO it's generally better to fix bugs in master first, and only then backport patches into the version branches.

@FabiKo117
Copy link
Contributor Author

Btw: why don't we just gzip everything? I mean, for the smaller amounts of data returned by the aggregation endpoints it doesn't do much, but it also doesn't hurt also 🤷

Good question.. I think we've discussed that already in the past, right? Because there we've also decided to just encode the responses of data extraction requests and not everyone. I guess the idea was that without gzip, the data aggregation responses are also directly displayed in the browser?

PS: could it be that you accidentally started this branch off of the 1.2 branch? IMHO it's generally better to fix bugs in master first, and only then backport patches into the version branches.

That might be the reason actually.. still the versioning in the pom in the master was not fitting, so it's also good that this got fixed now through that "mistake" :)

@tyrasd
Copy link
Member

tyrasd commented Nov 12, 2020

Btw: why don't we just gzip everything? I mean, for the smaller amounts of data returned by the aggregation endpoints it doesn't do much, but it also doesn't hurt also shrug

Good question.. I think we've discussed that already in the past, right? Because there we've also decided to just encode the responses of data extraction requests and not everyone. I guess the idea was that without gzip, the data aggregation responses are also directly displayed in the browser?

Hmm, as far as I can see, it's already the case that all requests are gzip compressed (see the metadata request example below, but the same is also the case for /elemet/count).

$ curl -v --compressed https://api.ohsome.org/v1/metadata
[…]
> GET /v1/metadata HTTP/1.1
> Host: api.ohsome.org
> User-Agent: curl/7.68.0
> Accept: */*
> Accept-Encoding: deflate, gzip, br
[…]
< HTTP/1.1 200 
< Date: Thu, 12 Nov 2020 14:28:40 GMT
< Server: Apache/2.4.29 (Ubuntu)
[…]
< Content-Type: application/json;charset=UTF-8
< Content-Encoding: gzip
< Vary: Accept-Encoding
< Transfer-Encoding: chunked
[…]

@FabiKo117
Copy link
Contributor Author

Ok.. yes.. I used the wrong description for the change that was actually made. I thought it has something to do with the gzip compression, but in fact it is about setting the response header to automatically provide the response as a downloadable file:
response.setHeader("Content-disposition", "attachment;filename=ohsome.geojson");

So the change proposed in this PR is actually "providing the /contributions response as a downloadable file".

@FabiKo117 FabiKo117 changed the title Gzip contributions response providing the /contributions response as a downloadable file Nov 12, 2020
@tyrasd tyrasd force-pushed the gzip-contributions-response branch from 6134249 to 5d491b9 Compare November 12, 2020 15:35
@tyrasd tyrasd self-requested a review November 12, 2020 15:37
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 (btw: I've added a changelog entry for this one as well.)

@tyrasd tyrasd force-pushed the gzip-contributions-response branch from b064500 to a180a2e Compare November 12, 2020 15:40
@tyrasd tyrasd merged commit a180a2e into master Nov 12, 2020
@tyrasd tyrasd deleted the gzip-contributions-response branch November 12, 2020 15:51
@tyrasd tyrasd added the enhancement New feature or request label Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants