Version the API

[chappjc]

I've wrestled with this for a while, and it seems like using the Accept header with version in a MIME type parameter (e.g. Accept: application/json; version=1), is perhaps the "right way" to do this. The thing is, forget that and all the headaches that go with it. I want to go with version in the URL.

Much like described in this blog post: https://cloudplatform.googleblog.com/2017/06/versioning-APIs-at-Google.html

The API (not the app) should have a X.Y version where the URL contains /vX. Changes in X are thus breaking (removed or renamed fields, etc.), while changes in Y are not breaking (added fields or endpoints, etc.).

[ukane-philemon]

@chappjc, this seems to be overdue. I'd like to pick this up.

[chappjc]

OK, but there's not much to do for this, except mirror the entire /api paths on /api/v1. Obviously all /api links have to keep working.
When we make breaking changes, we'll add /api/v2.

[chappjc]

Please use Chi's routing capabilities and sub-routers as needed to avoid logic our API handler implementations where possible. Review the chi methods and consider a single Router that can be shared between the two parent paths /api and /api/vX

https://pkg.go.dev/github.com/go-chi/chi/v5#Mux.Route
https://pkg.go.dev/github.com/go-chi/chi/v5#Mux.Mount

[ukane-philemon]

we can mount the same router on different paths. on /api/ as default and /api/v{version}/.

[chappjc]

we can mount the same router on different paths. on /api/ as default and /api/v{version}/.

Right. That will probably do it.

For planning for the future, here's an interesting approach: https://github.com/go-chi/chi/tree/v5.0.8/_examples/versions
...subrouters, common handlers, and an api.version context value for handlers to modify their behavior.

Regarding the api/types package, we could consider making that it's own module (again, since it was previously), but the module version would already be at v6 since we were at v5 before we reabsorbed it into the main dcrdata module. Alternatively, we could just have subfolders like api/types/v1, which seems better for a REST API's data type versioning.

[ukane-philemon]

For planning for the future, here's an interesting approach: https://github.com/go-chi/chi/tree/v5.0.8/_examples/versions
...subrouters, common handlers, and an api.version context value for handlers to modify their behavior.

Yes, this makes sense. This will enable us to use one entry point for requests and handle them differently.

[ukane-philemon]

Alternatively, we could just have subfolders like api/types/v1, which seems better for a REST API's data type versioning.

I agree. If we want to version the api/types module, It makes sense to follow the REST API versioning too.

[ukane-philemon]

But then are these changes are included in this issue, no? Or these would be for future planning(maybe when we are about to add version 2)?

[chappjc]

#1951 is good for now, but feel free to follow up with concepts to facilitate new versions

[ukane-philemon]

Okay. (will do this in a follow-up PR to keep #1951 small).

While this is unrelated, I've noticed the response from the insight API's status path is different from the response from the normal API status path and there is currently no way to see the insight API's actual status. While this could break existing consumers, I'm thinking it would make more sense to display the insight API's actual status when the insight/api/status path is hit and then allow callers to specify a query if they want the node status(i.e insight/api/status?q=getinfo or insight/api/status?q=nodeStatus).

This is because I'd like to see the behaviours across the various dcrdata API's to be the same when certain routes are hit (e.g root path and status path). Currently, the insight API root path does a redirect while the normal dcrdata API returns a message.

[chappjc]

there is currently no way to see the insight API's actual status

What do you mean by this? Here's what I see at https://dcrdata.decred.org/insight/api/status:

{
  "version": 1070000,
  "protocolversion": 8,
  "blocks": 723240,
  "timeoffset": 0,
  "connections": 8,
  "proxy": "",
  "difficulty": 5671496136.197787,
  "testnet": false,
  "relayfee": 0.0001,
  "errors": ""
}

We cannot redefine the Insight API, so that has to stay like it is.

We can change our own API as we like, but not Insight's

[ukane-philemon]

What do you mean by this?

Yes, It didn't blend with the pattern we have on the https://dcrdata.decred.org/api/status path. But then there could be a way to display the actual insight API status(example). Maybe use the https://dcrdata.decred.org/insight/api path for this instead of redirecting?

[chappjc]

I still don't know what you mean by "actual insight API status". The Insight API is a common block explorer API used by many blockchains and wallets, and defined by Bitpay, and thus we cannot change it. https://github.com/bitpay/insight-api/blob/45ebf7a152c1abfd179bf1b0d32734a2bd36e105/README.md#api-http-endpoints

Also, what is redirecting?

[ukane-philemon]

I was hoping the insight API could provide a response like this too, if not at least include other relevant fields like the network etc

{
"ready": true,
"db_height": 723240,
"db_block_time": 1671912542,
"node_height": 723240,
"node_connections": 8,
"api_version": 1,
"dcrdata_version": "",
"network_name": "mainnet"
}

[chappjc]

Oh. No, we cannot touch the Insight API's path or payloads. bitpay/bitcore defines them, and wallets like Exodus consume them. If any of it changes, large pieces of the ecosystem break.

[ukane-philemon]

Ah, okay.