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

Adapting ratio endpoint processing #27

Merged
merged 6 commits into from
Sep 2, 2020
Merged

Conversation

FabiKo117
Copy link
Contributor

@FabiKo117 FabiKo117 commented Aug 18, 2020

@bonaparten bonaparten force-pushed the adapting-ratio-endpoint branch from de4d1e4 to 09b20b1 Compare August 21, 2020 12:50
@bonaparten bonaparten closed this Aug 21, 2020
@bonaparten bonaparten force-pushed the adapting-ratio-endpoint branch from 09b20b1 to ae33581 Compare August 21, 2020 13:10
@FabiKo117 FabiKo117 reopened this Aug 21, 2020
@bonaparten bonaparten linked an issue Aug 21, 2020 that may be closed by this pull request
@FabiKo117 FabiKo117 changed the title WIP: Adapting ratio endpoint Adapting ratio endpoint processing Aug 27, 2020
tyrasd
tyrasd previously requested changes Aug 27, 2020
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.

this needs some unit tests to check if the behaviour for empty filters is correct. While testing I discovered that an empty filter2 with a share ratio request does not work because the API internally converts such a query to something like (<filter1>) or () (see screenshot below), which is not a valid filter syntax.

image

@FabiKo117
Copy link
Contributor Author

FabiKo117 commented Aug 27, 2020

You found this behavior with a /share request? The resource "/share" is not supported anymore since the release 1.0 from what I know.

edit: Ok I think you've used /ratio :D
Can you link the exact URL that you've used for this test?

@FabiKo117
Copy link
Contributor Author

Ok I could reproduce it. Working on it now.

@tyrasd
Copy link
Member

tyrasd commented Aug 28, 2020

sorry, I meant it happens in a ratio request (as you can see in the screenshot) 😅 . here is an example request and output:

$ curl "localhost:8082/elements/count/ratio?bboxes=8.6906,49.3994,8.6962,49.4032&filter=building=*&filter2="
{
  "timestamp" : "2020-08-27T16:38:17.077418",
  "status" : 400,
  "message" : "Invalid filter syntax. Please look at the additional info and examples about the filter parameter at https://docs.ohsome.org/ohsome-api. Detailed error message: line 1, column 18: whitespaces, not, (, KEY_STRING (a-z, 0-9, : or -), \", id, type or geometry expected, ) encountered.",
  "requestUrl" : "http://localhost:8082/elements/count/ratio?bboxes=8.6906,49.3994,8.6962,49.4032&filter=building=*&filter2="
}

@tyrasd tyrasd self-requested a review August 28, 2020 07:47
@tyrasd tyrasd dismissed their stale review August 31, 2020 13:23

identified issues have been resolved, will review again

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.

I would have preferred if the filter2 parameter was not optional for ratio requests, so that users don't by mistake send complex, long-running queries if they just alter an existing /count/ query to a /count/share/ one without thinking to add the filter2 parameter (I know I would probably often make this mistake). It would still be possible to leave the filter2 parameter empty to do the "compare against all OSM data" use case. But if you prefer it this way @FabiKo117 I am willing to accept it as is.

@FabiKo117
Copy link
Contributor Author

Ahh yeah I get that point. If someone is just playing around with the endpoints using the same set of parameters, defining filter2 as being non-optional for /ratio would avoid that issue with requesting too much data unintentionally. We can switch that restriction yes.

FabiKo117 and others added 6 commits September 2, 2020 10:46
in all /ratio processings
if given filter param is null or empty - throws 400 BadRequestException
only used in /ratio requests
that still used types-keys-values parameters in ElementsRequestExecutor
making combined filter empty if filter2 is empty
adding a test
@tyrasd tyrasd force-pushed the adapting-ratio-endpoint branch from 6da01e0 to 7e349c5 Compare September 2, 2020 08:46
@FabiKo117 FabiKo117 merged commit 2dda1f8 into master Sep 2, 2020
@FabiKo117 FabiKo117 deleted the adapting-ratio-endpoint branch September 2, 2020 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unfitting response for missing filter2 parameter
3 participants