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

Add detail to query params for providers #64

Conversation

asadowns
Copy link
Contributor

For trips and status_changes break query params
out into their own section.

Add more detail around how to specify time
and location based parameters.

For location this is just a suggestion.

There may be url length concerns when passing in large polygons
in query params.

A better approach might be to pass in service_area id or
use a location filter based on the token and
filter data further after retrieval

Comments welcome.

For trips and status_changes break query params
	out into their own section.

Add more detail around how to specify time
	and location based parameters.

For location this is just a suggestion.

There may be url length concerns when passing in large polygons
	in query params.

A better approach might be to pass in service_area id or
	use a location filter based on the token and
	filter data further after retrieval

Comments welcome.
@asadowns asadowns force-pushed the asadowns/add_query_param_detail branch from 654d34a to 0f28172 Compare September 14, 2018 18:02
@hunterowens
Copy link
Collaborator

@asadowns any thoughts on a POST based implementation? It was raised as an issue in #78

@oderby
Copy link
Contributor

oderby commented Sep 19, 2018

Currently the documentation for the /service_areas endpoint of the Agency API states that it's a GET request with a Body containing provider_id and service_id. Per discussion in #78 - is that something you feel like fixing here @asadowns @thekaveman or would you rather I open a separate PR?

@thekaveman
Copy link
Collaborator

@oderby you are absolutely correct, good catch. I think opening a separate PR for that fix makes sense.

provider and agency are kind of evolving a different pace right now but eventually as more clarity is realized, the two should align pretty well.

oderby pushed a commit to oderby/mobility-data-specification that referenced this pull request Sep 20, 2018
Per discussion in openmobilityfoundation#78, all endpoints for GET requests should only accept
request parameters via the query string in the URL. While openmobilityfoundation#64 clarifies
this for the Provider API, this updates the only GET endpoint in the
Agency API.
Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Most of this looks really good.

For the location querying, I am concerned about URL length restrictions for complex polygons. Here's another idea I had for this:

  • event_location a lat/lon pair
  • radius in meters

Which would return events (trips and status_changes) that correspond to the given radius around the given point.

Not as clean as packaging up an exact polygon or some of the other JWT ideas we've discussed, but it's simple from an API perspective.

hunterowens pushed a commit that referenced this pull request Sep 25, 2018
Per discussion in #78, all endpoints for GET requests should only accept
request parameters via the query string in the URL. While #64 clarifies
this for the Provider API, this updates the only GET endpoint in the
Agency API.
@thekaveman thekaveman added this to the 0.2.0 milestone Sep 26, 2018
This fixes the issue where the URLs would exceed distance by changing it from Polygons to Bounding Boxes.
Copy link
Collaborator

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

I like the bounding box approach, but I have a slight complication to bring up:

None of this addresses AND vs. OR - what if I want to query for all trips that started OR ended in my bounding box. With standard querystring semantics, this is an AND. (Of course I could always issue 2 requests and merge on the backend).

Another way to express this, albeit changing the wording and perhaps original intent of the spec, is to replace start_location and end_location params with the single bbox param; this would imply querying for all trips with any observed point in the bounding box. This removes the start/end type querying and grouping from the API and forces it on to the data layer, but I'd argue it makes sense there as the needs will be different for each use-case.

The bbox param would translate to status change querying and semantics as well (vs. start_location, end_location which don't really make sense for status change, or something else entirely, specific to status change).

provider/README.md Show resolved Hide resolved
@hunterowens hunterowens changed the base branch from master to dev September 27, 2018 04:33
@hunterowens hunterowens merged commit 39a72dc into openmobilityfoundation:dev Sep 27, 2018
hunterowens pushed a commit that referenced this pull request Oct 1, 2018
Per discussion in #78, all endpoints for GET requests should only accept
request parameters via the query string in the URL. While #64 clarifies
this for the Provider API, this updates the only GET endpoint in the
Agency API.
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.

5 participants