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

Structure admin area endpoints #61

Open
olafveerman opened this issue Apr 29, 2015 · 24 comments
Open

Structure admin area endpoints #61

olafveerman opened this issue Apr 29, 2015 · 24 comments

Comments

@olafveerman
Copy link
Collaborator

Triggered by Derek's question in orma/openroads-analytics#52, I think there is some room for improvement on the endpoints that deal with administrative areas, especially on how we structure the routes.

Here's a proposal that aims for two things:

  • makes it easy to get data from either a particular admin area or its descendants
  • provides high degree of control over what data is returned

Single administrative areas

Get information from a single administrative area. Does not include a reference to its descendants.

endpoint returns
/admin/:id Basic meta-data (name, id, type)
/admin/:id/boundary Basic meta-data + Boundary
/admin/:id/road-network Basic meta-data + Road network

Subregions

Get data from the subregions of a given area.

endpoint returns
/admin/:id/subregions Basic meta-data of all sub-regions (name, id, type)
/admin/:id/subregions/boundary Basic metadata + boundary of subregions

@dereklieu @kamicut @anandthakker @danielfdsilva
Would be great to hear your thoughts. Whether you agree that there is a need to begin with, the proposed structure, etc. This could be tackled first thing in phase 2.
Let me know if you want me to illustrate it with examples from the response.

@olafveerman
Copy link
Collaborator Author

BTW I can even imagine we don't need /admin/:id/road-network at all and use what's now the /map endpoint.

@danielfdsilva
Copy link
Contributor

It helps to understand how the api is structured and what's the relation between the different elements.
If the changes are easy to perform I say go for it.

@anandthakker
Copy link
Contributor

A big 👍 from me!

@olafveerman Not sure about /map replacing /admin/:id/road-network, since the latter is meant to clip the roads to the boundary of the region, whereas the former returns the entirety of any way that intersects the requested bbox. (Unless something's changed about the map endpoint.)

@olafveerman
Copy link
Collaborator Author

Ah, good point. Will also include that in the documentation, thanks @anandthakker

@dereklieu
Copy link
Contributor

Yeah, I also think this is a good list to start with on Phase 2. It'd be nice to have the flexibility as we flesh out these analytics pages more.

@danielfdsilva
Copy link
Contributor

Had a look at the current endpoints and how the data is structured. Here's a proposal for what the data could look like for the new endpoints:

/admin/:id (Basic meta-data (name, id, type))

{
    "id": ​2110147000,
    "adminType": ​3,
    "name": "Basco",
    "properties":  {
        "ID_0": ​177,
        "ISO": "PHL",
        "NAME_0": "Philippines",
        "ID_2_OR": ​2110000000,
        "NAME_2": "Batanes",
        "ID_3_OR": ​2110147000,
        "NAME_3": "Basco",
        "NL_NAME_3": null,
        "VARNAME_3": null,
        "TYPE_3": "Bayan|Munisipyo",
        "ENGTYPE_2": "Municipality",
        "X_COORD": ​121.99262517700001,
        "Y_COORD": ​20.458421853399997
    }
}

/admin/:id/boundary (Basic meta-data + Boundary)

{
  "type": "Feature",
  "properties": {
    "ID_0": 177,
    "ISO": "PHL",
    "NAME_0": "Philippines",
    "ID_2_OR": 2110000000,
    "NAME_2": "Batanes",
    "ID_3_OR": 2110147000,
    "NAME_3": "Basco",
    "NL_NAME_3": null,
    "VARNAME_3": null,
    "TYPE_3": "Bayan|Munisipyo",
    "ENGTYPE_2": "Municipality",
    "X_COORD": 121.99262517700001,
    "Y_COORD": 20.458421853399997
  },
  "geometry": {
    "type": "Polygon",
    "coordinates": [
      [
        [
          122.00363922119175,
          20.489030838012688
        ]
        // More Coords
      ]
    ]
  },
  "id": 2110147000,
  "adminType": 3,
  "name": "Basco"
}

/admin/:id/road-network (Basic meta-data + Road network)

{
"id": ​2110147000,
"adminType": ​3,
"name": "Basco",
"roads": {
    "type": "FeatureCollection",
    "features": [ /* no features for this region */ ]
  }
}

Thoughts? Is this what you had in mind?
Something that is kind of bothering me is that the responses vary between json (1st and 3rd) and geojson (2nd). Do you see this as a consistency problem or is it my ocd kicking in? :P

@dereklieu You thinks it's enough to omit the properties we don't want from the response or should we change the db queries and do not return them in the first place? What kind of implications does this have?

@dereklieu
Copy link
Contributor

@danielfdsilva do you mean all the nulls for NL_NAME_3 and so forth? I don't see any harm in keeping them there, and it keeps the response structure consistent for every admin level, which would be helpful if you're new to using this part of the API.

As for the json/geojson schism, I hadn't thought of it but it's a little strange now that you bring it up. I guess I wouldn't mind turning the road-network into geojson format, but since the base admin endpoint is about the metadata, I'm fine keeping that as json.

@dereklieu
Copy link
Contributor

@danielfdsilva and I'd have to check but I'm pretty sure making any changes here will break the dashboards, to some degree. So we should look into that and do the appropriate changes before we push these live.

@danielfdsilva
Copy link
Contributor

@dereklieu Actually I meant something different.
For the first response (/admin/:id) we're not returning the geometry, but the function getAdminBoundary(id) returns it. My question was do we create another function that returns just what we're looking for, or is it enough to omit what we don't want (like geometry) from the result before sending it to the client.

The roads object in the road-network is a geojson, but is not at root level...

These changes are going to break the dashboards. Before pushing this we need to make changes to the dashboards as well.

@dereklieu
Copy link
Contributor

@danielfdsilva sorry, not sure I understand the question here. What are we looking for here? Is it that we're doing an unnecessary calculation of geometry without using it?

And yes, the roads is not currently geojson at root level. If we wanted to standardize on geojson we could just attach id, adminType, and name as properties.

Yes, these changes will break the dashboards. The easiest way would be, once you're at a stage where you feel good about the refactor, push these changes up to a branch so we can figure out what's different, then modify the dashboards accordingly.

@danielfdsilva
Copy link
Contributor

Document with the current status / some points to consider:

https://gist.github.com/danielfdsilva/29156361658a921eabb4

@olafveerman
Copy link
Collaborator Author

  • What is NL_NAME_3 and VARNAME_3?

I think we can remove them, but I'll double check that tomorrow morning

  • What are ENGTYPE_2 and TYPE_3 used for? And Why one is _2 and the other _3 even though it looks like the refer to the same?

The third administrative level is usually a Municipality, but can also be a City. My guess is that TYPE_3 is the Philippine term and ENGTYPE_3 is the English translation. Again, will check tomorrow.

  • Since this endoint is specific for boundaries and we're not returning them, should we return an error instead?

Yes, but in principle we should always return a boundary on the boundary endpoints.

Could we simplify the /admin/:id/subregions endpoint to something like:

{
  "id": 13000000000,
  "name": "Region IV-B (Mimaropa)",
  "properties": {
    "NAME_0": "Philippines"
  },
  "adminAreas": [
    {
      "id": "13020000000",
      "name": "Agusan Del Norte",
      "X_COORD": 118.46191032700001,
      "Y_COORD": 9.50722963322
    },
   {}
  ]
}

Each adminArea doesn't need a sub-object with properties, since they will always be the same as their parent.

@danielfdsilva
Copy link
Contributor

The adminAreas don't need a sub-object with properties, since they will always be the same as their parent.

Yes, valid point. We could event remove it for the parent and standardize. We'd only leave it when the response is geojson

@olafveerman
Copy link
Collaborator Author

There is still value in storing data about the parents of the parent. The above example is not really good, since we're dealing with the highest level in the country, but on a municipality level, we'd include the names and id's of the parents.

@danielfdsilva
Copy link
Contributor

Gist updated with current version of the API.
https://gist.github.com/danielfdsilva/29156361658a921eabb4

@olafveerman @dereklieu please check if the proposed structure works

@olafveerman
Copy link
Collaborator Author

Looking good.

  1. on /road-network and /boundaries, should we be returning the names and id's of the parents?
  2. Do we still need /admin to return a list with regions? Shouldn't this be returned on /admin/:id/subregions? (where :id is the id of the whole country)

@dereklieu
Copy link
Contributor

Ah, it probably would be useful to return the names/ids of parents even when the road network or boundary is too large to display. I'm onboard with the second point too, it's small but would make calling the api more consistent throughout the dashboards.

@danielfdsilva
Copy link
Contributor

@olafveerman
1 - Didn't understand this. The parent's ids and names are inside the properties object, like in every other geojson response.
2 - Seems logical. What's the id of the whole country?

@dereklieu
For the subregion boundary that would be possible, but for the road network it is actually triggering an error. (https://github.com/opengovt/openroads-api/blob/develop/services/bounding-box.js#L35) Not sure what the implications of removing/ changing this would be.
But anyway, why would this be useful? Removing the coordinates from the data returned by admin/:id/subregion/boundary is basically the data returned by admin/:id/subregion

@anandthakker
Copy link
Contributor

Sorry I'm coming in so late here -- couple comments:

Looks like /admin/:id/road-network and /admin/:id/subregions/boundary are returning a FeatureCollection with properties on it. Note that this is technically not something described in the GeoJSON spec. It's also not prohibited by the spec, but just wanted to raise it so we're aware.

I wonder if, semantically, it makes more sense to accept a query parameter like ?boundaryFeatures=true or ?format=GeoJSON on the /admin/:id and /admin/:id/subregions endpoints, instead of having the .../boundary endpoints be separate.

@dereklieu
Copy link
Contributor

@danielfdsilva I think Olaf's first point is also what I raised, which is that we should have parent data even when the boundary is too large. However, you responded to this nicely which is that we already have an endpoint that does this.

And then Anand came along with the better suggestion that yes, this does seem to be the case where a ? param seems to make sense, such as boundary=true and roadnetworks=true.

Is this feasible @danielfdsilva?

@olafveerman
Copy link
Collaborator Author

👍 to what was said above

@olafveerman
Copy link
Collaborator Author

Should both parameters be mutually exclusive?

@danielfdsilva
Copy link
Contributor

I'd say so, because since the response is a geoJSON it can only be about one thing (right?), and also because it probably grows too large.

So, summarizing the new endpoints:

  • /admin/:id -> stays as it is. Meta about an admin region. (JSON)
  • /admin/:id/boundary -> changes to /admin/:id?boundary=true. Region meta and boundary (GeoJSON)
  • /admin/:id/road-network -> changes to /admin/:id?roadNetwork=true. Region meta and road network. (GeoJSON)
  • /admin -> Dropped. Use /admin/:id/subregions with country id.
  • /admin/:id/subregions -> stays as it is. List of subregions. (JSON)
  • /admin/:id/subregions/boundary -> changes to /admin/:id/subregions?boundary=true. List of subregions with boundaries. (GeoJSON)

Yay?

@dereklieu
Copy link
Contributor

👍

danielfdsilva added a commit that referenced this issue Nov 19, 2015
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

No branches or pull requests

4 participants