I'd take it further, though in my opinion it shouldn't be done until we're
ready to do away with Perl entirely.
I've been working on this for a little bit, and I sorta wanted to make a wiki
page for a proposed API v2 design before sending it, but what the heck. The
conversation is here now.
I don't like the way our API is versioned. Let me be clear, it _should_ be
versioned, and the
versions _should_ be semantic, but it is my opinion that the API should be
versioned alongside
Traffic Ops rather than at its own pace. I've written essentially an article on
why this should be
the case, and it follows:
** TIMING **
The first issue I should address is timing - this needn't happen immediately.
For the time being, I
think a radical change would be much more harmful than continuing along our
current versioning
scheme. This change should be implemented with the advent of what we currently refer to
as "API v2".
Presumably, at this time all of the Perl code has been removed and we are
looking to re-tool the API
to be more sensible and standards-conforming. It's a time when we'll already be
making big, breaking
changes. I personally would love for this to be ATC 4.0, but that may not be a
realistic goal.
** REDUNDANCY **
The easiest to see - albiet simultaneously most trivial - issue with the
current API versioning
scheme is how every request path must start with '/api/1.x/'. This is annoying,
to be frank;
especially when one considers that the current plan for Traffic Ops is to
reduce it entirely to an
API, so literally every endpoint will have '/api/' in common. We _know_ the
endpoint we are trying
to query is an API endpoint because _all_ endpoints are API endpoints. When we reach
"API v2" the
'/api/' part of request paths will have lost all value entirely.
Even with that gone we are left with '/1.' (or '/2.' as the case may become) as
a common prefix,
again annoying although not totally useless in this case. However, the vast
majority of API
endpoints see no changes between minor versions, so really '/1.x' just becomes
a static, constant
prefix where 'x' is the latest version of the API.
In any case, versioning the API alongside Traffic Ops solves this problem
because our Traffic Ops
server(s) emit HTTP headers that name the server. Once Perl is gone, we'll be
free to use the HTTP
`Server:` header to name the server e.g. `Server: Traffic Ops/3.2.1`. At this
point, we could
either implement an `OPTIONS` method request at the server's root that would
just return some
headers - including `Server:` or just let people to a `GET` (or better yet
`HEAD`) request to
`/ping` and pull the server version out of the headers. The client then has all
the information it
needs to communicate effectively with the server. The alternative to this
within our current
versioning schema is to implement an unversioned API endpoint such that we have
a hundred `/api/1.x`
endpoints and one that has some other - or possibly no - prefix, or have a
versioned API version API
endpoint, which is confusing even just to say.
** TRAFFIC OPS _IS_ THE API **
As mentioned previously, the endgame of our transition from Perl to Go is that
Traffic Portal is the
only UI for ATC - and in fact that's already somewhat true. That leaves Traffic
Ops as a database
and a REST API for interacting with said database. In fact, the database is
often referred to as its
own service: "ToDb", "Traffic Ops DB" etc. That means that we have the single
most important Traffic
Control component split in two - half of its functionality is versioned sanely
with Traffic Control
while the other half is versioned separately from anything else in the world.
That's crazy, because
if you have a program that only does two things, then surely a breaking change
to one of those
things means increasing its major version? If that's the case, then why are we
not versioning the
API alongside Traffic Ops?
It may be argued (incorrectly, in my opinion) that Traffic Ops does more than
serve an API to
interact with a database. It generates configuration files and system images,
it combines data and
does heavy processing on it. But really those things shouldn't be taken into
account in a versioning
scheme except insofar as they affect the experience of some user,
administrator, or application
interfacing with Traffic Ops. If the API responses don't change in form or
content, and if the
process of setting up or maintaining the application haven't changed, then any
code changes you've
made are a patch, not a version change. Traffic Ops does big things, but at the
end of the day it
all just boils down to API inputs and outputs as far as anything and anyone
else is concerned.
** CONFUSION **
We currently live in a world where I can run a script using the Traffic Ops API
that works perfectly
fine against Traffic Ops version 3.2.1, but then if I again test it against
version 3.2.1 at some
point in the future it breaks because breaking changes were made in the API. It's
easy to say, "oh,
that just means that when we make breaking API changes we should increment the
version
appropriately," but realize that this _is versioning the API alongside Traffic
Ops_. If we're not
doing that, we're saying there is unpredictability with the behavior of our
system within releases,
and if we _are_ doing that then the only difference between the API version and
the Traffic Ops
version is that the API version is confusingly behind by about 2 major
revisions. It should just be
the same for simplicity's sake.
** THE API "PROMISE" **
The single most common argument I hear in favor of our current API versioning scheme
is "well we've
said an API version 1.x behaves in this way, and so we must uphold that promise to
the user base".
Not only do we routinely break that promise already,
(e.g. PR #3110 [https://github.com/apache/trafficcontrol/pull/3110]) but I'm
certainly not
suggesting that we don't uphold this promise. Yes, this does mean that making
breaking API changes
in TO would require a new major release and adding features/fields to the API
would require a new
minor release. I don't see that as a big deal, especially if implemented at the
time I'm suggesting
when we'd be re-designing the API - and it does sorely need to be redesigned.
* The API Needs to be Redesigned *
I'm going to go down this rabbit hole for a second, if you're already convinced
the TO API needs a
re-design then feel free to skip this section. I'm not going to touch on any
problems caused in the
API as currently implemented by the use of a standalone API version - that's
what the entire article
is for.
Currently, our API currently has three huge problems:
1. Rampant oversaturation of endpoints
We have a systemic issue of re-implementing behaviour in multiple
endpoints. This is due in part
to a lack of good documentation - so developers aren't aware of the
endpoints available to them
- and partly because of the "Mojolicious Mindset" that plagues our
oldest endpoints. The
"Mojolicious Mindset" refers to the use of URL path fragments as
request parameters, e.g.
'/users/{{ID}}' instead of/in addition to '/users?id={{ID}}'. From the
perspective of someone
who is just writing the back-end for these endpoints, there's no clear
advantage to one over the
other except that the former seems to more clearly reflect the intent
of requesting a specific
object whereas the latter could be seen as more of a "filter" on a
larger collection. That's not
incorrect, necessarily, but the two are totally separate request
paths, so having '/users' and
'/users/{{ID}}' means documenting two endpoints instead of one, and it
means two lines in the
route definitions, and it means two seperate handlers instead of one
(albiet a more complex
one).
Consider also that we have all of the following endpoints for
manipulating Cache Groups:
* /api/1.x/cachegroup/{{parameter ID}}/parameter
* /api/1.x/cachegroup_fallbacks
* /api/1.x/cachegroupparameters
* /api/1.x/cachegroupparameters/{{ID}}/{{parameter ID}}
* /api/1.x/cachegroups
* /api/1.x/cachegroups/{{ID}}
* /api/1.x/cachegroups/{{ID}}/deliveryservices
* /api/1.x/cachegroups/{{ID}}/parameters
* /api/1.x/cachegroups/{{ID}}/queue_updates
* /api/1.x/cachegroups/{{ID}}/unassigned_parameters
* /api/1.x/cachegroups/{{parameter ID}}/parameter_available
* /api/1.x/cachegroups_trimmed
These could all be collapsed neatly into one or two endpoints, but were
instead implemented
separately for whatever reason(s)
(see Issue #2934 [https://github.com/apache/trafficcontrol/issues/2934]
for details).
2. Improper/Non-existent standards conformity
We have requests that should be read-only that make server-side state
changes (Issue #3054
[https://github.com/apache/trafficcontrol/issues/3054]), we have
endpoints returning success
responses on failure (Issue #3003
[https://github.com/apache/trafficcontrol/issues/3003]) and
our "CRUDER" has failed us. `PUT` should be used to _create_ objects
(or possibly update them by
creating an alternative representation with the same identifiying
information) but is instead
used as the primary "edit this" method. `POST` is for processing
entities in a data payload, but
is instead used for object creation. `PATCH` languishes, totally
unimplemented. These problems
are systemic and stem partially from the "Mojolicious Mindset" whereby
new functionality is
introduced into the API by first considering what request method is
appropriate and then
deciding on a request path that names the operation being done.
Request methods are meant to be
the different ways in which a client interacts with a resource on the
server, and thus the
resources themselves should be considered primary. The "CRUDER"
hampers this mindset, because it
makes treating payloads and query parameters generic and isn't
receptive to injection of new
behaviour.
The "CRUDER" is a great innovation, to be sure, as it saves us quite a
bit of time and prevents
duplicating code, but the problem it solves is an emergent property of
of API problem #1 above.
With fewer endpoints we'd have much more specific handling, and the
need for and advantages of
the "CRUDER" will vanish.
3. Needing multiple queries to obtain a single piece of information
This issue is pretty deeply rooted, and is related to the way our
database is structured. But
that's part of the problem - an API needn't replicate the database,
and is therefore free from
some of the constraints that bind a database.
The way things are now, in order to e.g. create a server definition I
must make several
preliminary requests to determine the integral, unique,
non-deterministic identifiers of other
objects. I think we all agree that ideally these integral IDs would
have no part in the output
of the API, and many people I've spoken to would support wiping them
from the database entirely
given enough time and manpower.
________________________________________
From: Robert Butts <r...@apache.org>
Sent: Wednesday, February 13, 2019 6:11 PM
To: dev@trafficcontrol.apache.org
Subject: Re: [EXTERNAL] Re: Traffic Ops API versioning issues
It doesn't seem like the idea of full TO API SemVer was ever fully
discussed and voted on
Since it seems like we never truly committed to SemVer with minor versions
for the TO API
There was consensus:
https://lists.apache.org/thread.html/8f8a850c68424021a0fe06967894383a24e463f1b0cee4d652d04590@%3Cdev.trafficcontrol.apache.org%3E
https://lists.apache.org/thread.html/1a42a2192a81fc4d76639ccd10761b6b73c31345a63715bb8aa86e4e@%3Cdev.trafficcontrol.apache.org%3E
We didn't do an official [VOTE], but we rarely do that as a project, unless
there's difficulty reaching unofficial consensus. That said, there's
nothing stopping another discussion or vote, if we want to change things.
as long as we set the expectations for the user
That's precisely my point. We can always say "we set the expectations, it's
the user's fault for misreading, or misunderstanding, or not noticing a
changelog." It's not about fault, it's about providing a better user
experience.
that support has to be baked into traffic_ops_golang such that it's easy
and maintainable to support tons of minor versions. If we keep heading down
our current path we are going to be left with a giant mess.
I don't think anyone disagrees. Traffic Ops versioning has had considerable
technical debt for some time now. We just haven't been willing to spend the
time to fix it. I agree, that needs to change.
I'd also like to note, there's another option: code generation. We can keep
the structs and handlers largely the same, and generate the "duplicate"
code when a new minor version is added. There's a strong argument that this
is the "idomatic" way to solve this problem, for the Go language. I
personally feel Reflection is better here. But, I wanted everyone to be
aware that's an option, if the consensus is that it's the best option. In a
nutshell, we could generate code with a script (with a spectrum of
precision and complexity, from `cp && sed` to parsing the AST), and
`routes.go` would have a `//go:generate` header. For examples, see:
https://blog.golang.org/generate
https://godoc.org/golang.org/x/tools/cmd/stringer
I did some prototyping of that approach, if anyone is interested, just let
me know and I can provide examples of what that would look like.
On Wed, Feb 13, 2019 at 5:09 PM Rawlin Peters <rawlin.pet...@gmail.com>
wrote:
On Wed, Feb 13, 2019 at 1:20 PM Gray, Jonathan
<jonathan_g...@comcast.com> wrote:
I'm +1 on keeping full API SemVer.
On 2/13/19, 12:16 PM, "Robert Butts" <r...@apache.org> wrote:
We would be abandoning Semantic Versioning.
This is why I wanted to open this up for broader discussion within the
community. It doesn't seem like the idea of full TO API SemVer was
ever fully discussed and voted on (at least not to my knowledge),
which is why I haven't been abiding by it myself or enforcing it upon
other TO API devs either. If we're going to truly commit to SemVer
with minor versions for TO API then we should fully understand the
cost vs utility of doing so. Also, SemVer doesn't have to be an "all
or nothing" thing, as we currently choose to ignore the "patch"
version for the TO API. We could also choose to ignore the minor
version and just focus on the most important aspect of SemVer which is
not introducing backwards-incompatible changes in the same major
version.
Since it seems like we never truly committed to SemVer with minor
versions for the TO API, traffic_ops_golang wasn't designed to easily
support minor versions. So if we're going to truly commit to SemVer
with minor versions for TO API, then that support has to be baked into
traffic_ops_golang such that it's easy and maintainable to support
tons of minor versions. If we keep heading down our current path we
are going to be left with a giant mess.
The only way I can get behind supporting the "minor version promise"
is if we have a way to basically just tag fields as introduced in a
specific API version, with just a single struct per resource and a
single handler per major version of an endpoint. We should only
require a single implementation of an endpoint per major version. If a
custom JSON parser allows us to do that, then that's great. Without
that, I don't think supporting the "minor version promise" is even a
viable option.
A lot of those scenarios around user confusion due to supporting just
a major version are not really an issue as long as we set the
expectations for the user. I.e.:
- clients should request v1
- as the v1 API is enhanced in a backwards-compatible manner, clients
will begin to see new fields in the server responses
- if a client wants to modify a resource, they will typically do a GET
on the resource, modify the resource, then do a PUT back to the
server.
- in the GET response, if the client sees new fields, they can expect
to modify those fields and see it reflected on the server.
- if the client does NOT see new fields, then they cannot add fields
and expect to see that reflected on the server
- null values in optional fields will be interpreted by the server as
whatever the default for that optional field is. So if the default for
new optional field "foo" is 5, then a client sending `"foo": null`
will be interpreted by the server as `"foo": 5`, and `"foo": null`
should never have a different meaning than `"foo": 5` on the server.
The advantage of just supporting the "major version promise":
- jives better with Go's lack of metaprogramming
- no custom JSON parser in Go required
- less general overhead in development
- only have to worry about breaking changes
- no worrying about which fields belong to which minor versions
- no extra API testing to make sure fields introduced in API v1.N
aren't returned to v1.N-1 clients
- don't have to update every single client in the repo with the new
minor version every time a minor version is incremented (since clients
would just specify the major version)
So, as a community, we need to weigh these options and decide whether
or not we want to take the "major version only" route or the "major
plus minor version" route. Personally, I prefer the "major version
only" route because it means less code, less overhead, less
coordination, and less things to potentially go wrong. However, if a
relatively small custom JSON parser is all we really need to
reasonably support the "minor version promise", then I can't say I'm
completely against that route either (just that I wouldn't prefer it).
- Rawlin
