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

Version the API #48

Open
chappjc opened this issue Jun 27, 2017 · 16 comments · May be fixed by #1953
Open

Version the API #48

chappjc opened this issue Jun 27, 2017 · 16 comments · May be fixed by #1953
Milestone

Comments

@chappjc
Copy link
Member

chappjc commented Jun 27, 2017

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.).

@chappjc chappjc added this to the 1.0.0 milestone Aug 20, 2017
@chappjc chappjc modified the milestones: 1.0.0, 1.1.0 Nov 7, 2017
@chappjc chappjc modified the milestones: 1.1.0, tbd Sep 13, 2018
@ukane-philemon
Copy link
Collaborator

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

@chappjc
Copy link
Member Author

chappjc commented Dec 24, 2022

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
Copy link
Member Author

chappjc commented Dec 24, 2022

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
Copy link
Collaborator

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

@chappjc
Copy link
Member Author

chappjc commented Dec 24, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

ukane-philemon commented Dec 24, 2022

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
Copy link
Member Author

chappjc commented Dec 24, 2022

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

@ukane-philemon
Copy link
Collaborator

ukane-philemon commented Dec 24, 2022

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
Copy link
Member Author

chappjc commented Dec 24, 2022

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
Copy link
Collaborator

ukane-philemon commented Dec 24, 2022

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
Copy link
Member Author

chappjc commented Dec 24, 2022

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
Copy link
Collaborator

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
Copy link
Member Author

chappjc commented Dec 24, 2022

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
Copy link
Collaborator

Ah, okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants