Structure admin area endpoints

olafveerman commented 9 years ago

Triggered by Derek's question in developmentseed/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 commented 9 years ago

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

danielfdsilva commented 9 years ago

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 commented 9 years ago

A big :+1: 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 commented 9 years ago

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

dereklieu commented 9 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

Document with the current status / some points to consider:

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

olafveerman commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

@olafveerman @dereklieu please check if the proposed structure works

olafveerman commented 8 years ago

Looking good.

on /road-network and /boundaries, should we be returning the names and id's of the parents?
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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

:+1: to what was said above

olafveerman commented 8 years ago

Should both parameters be mutually exclusive?

danielfdsilva commented 8 years ago

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 commented 8 years ago

:+1:

orma / openroads-api

Structure admin area endpoints #61

Single administrative areas

Subregions