I'm +1 on keeping full API SemVer. It allows an API consumer I have a stronger
contract around the semantics of what we should be produce and consume. The
piece that's still missing to me is issue #2872 so as a client I can safely
verify my API contract is still valid after TO has been upgraded.
Jonathan G
On 2/13/19, 12:16 PM, "Robert Butts" <r...@apache.org> wrote:
We would be abandoning Semantic Versioning. SemVer is a set of rules for
versioning so as not to break users. We currently ignore the "patch"
because it doesn't make a lot of sense for an API, but the minor version
does. It provides a measurably better user experience. We can't reasonably
claim to be doing "Semantic Versioning" for "major versions only." That's
just "breaking changes go in major versions," not the spirit or intent of
Semantic Versioning. We would, for all practical purposes, be abandoning
Semantic Versioning and the better user experience it provides.
Yes, Go was a poor language choice for an API, but we shouldn't make
clients of a Traffic Control CDN have a poor experience, because we made a
poor technical choice.
It's easy to miss why minor versions are important. A few examples:
Scenario 1:
A user has a client with an older version, say 1.1, and there’s a struct
field added in 1.2, Foo. The user POSTs an object with no Foo. How does the
server distinguish whether the user had the latest client, and
intentionally wanted to set Foo null, or whether they had an old client,
and the Server should default it to “foo”? Not using minor versions makes
it impossible for us, on the server side, to allow people to intentionally
set a field as null, while defaulting if a user makes a request from an
older version.
Scenario 2:
User has a newer client than the server. User POSTs with Foo, the server
doesn’t know what Foo is, ignores it. User then does a GET and Foo is
missing. It just silently ignored it, and they have no idea why. Because
again, there are no minor versions, everything is sent to `
trafficops.com/api/1/foo`.
Scenario 3:
User is using the HTTP, not one of our Go/Python clients. They mistakenly
think the server is 1.1 when it’s actually 1.2, and GET and get the new Foo
field. They see it in the response, manipulate it, and PUT it back. Then
their friend User2 at another company calls them, and they tell User2
“Yeah, 1.1 has Foo now!” User2 is actually running 1.1, they do a POST with
“foo”, but it doesn’t set, it doesn’t do anything, because Server 1.1
doesn’t know about Foo. User2's CDN provider upgrades to 1.2 a week later.
Now User2 is _really_ confused why their data isn’t there.
Scenario 4:
User is using an older Python client (or any other dynamic-language
client). Same issues as Scenario 3 apply. Because the Server and Client
don't scrub fields they don't know about, the user gets and posts fields
not in their version. Then, maybe their CDN provider downgrades, or maybe
they work with multiple CDNs, or maybe they talk to someone else in the
community, and tell them Version X has the field.
At the end of the day, most “minor version” problems are _actually_ user
problems. We can always say “well, it’s the user’s fault for not knowing
their client version, their server version, etc.” As a project, we can say
"you should have known" or "you should have read the docs" or "you should
have known what exact server version you/your CDN provider was using." But
it happens, even smart people make mistakes, and it makes for a poor user
experience. Especially as we move toward Self Service, and TC CDNs have
more clients with less technical experience. This will not only make our
users' lives better, but it will make our lives better, in both Development
and Support, not having to answer confused phone calls, and just doing the
right thing in the first place.
> What I'm getting at is that it is going to be a massive amount of dev
work to implement and maintain unique structs and handlers
I have a proposal, to reduce the development work. I'd been hoping to have
a few more weeks to make this a little more polished, but:
https://github.com/rob05c/apiver
This Go package parses struct tags, and marshals and unmarshals JSON per
the minor version specified in the struct tag. This solves 3 problems at
once:
1. Versions. Minor versions newer than the current request are not
serialized or deserialized. Default-populating code continues to exist as
always.
2. String-numbers. It allows us to specify number fields that should
unmarshal from strings, to fix @jhg03a's strings working in Perl but not Go
breaking scripts, with a ",str" tag field.
3. Nullable fields. This lets us get rid of all our "FooNullable" vs
non-nullable structs, and have non-pointers for required fields. Fields
which aren't pointers are automatically required (this is impossible with
the standard Go json, which can't distinguish default from missing
non-pointers).
The struct looks like:
```
type DeliveryService struct {
XMLID string `json:"xmlId" tc:"1.1"`
TypeID int `json:"typeId" tc:"1.1,str"`
DeepCachingType DeepCachingType `json:"deepCachingType" tc:"1.4"`
}
```
There's only 1 struct, only 1 handler, and the only versioning code
anywhere in the handler is a single `json := apiver.NewJSON(1.1)` or
`apiver.UnmarshalJSON(b, &v, 1.1)` call (which is actually already
abstracted by both the "CRUDer" framework and the api helper funcs).
The cost is, it uses Reflection. In a nutshell, it parses the tags and
dynamically creates an object which will serialize/deserialize how we want,
and passes that to the standard encoding/json library. For Unmarshal, it
also takes that object after unmarshalling and converts it back to the real
type. It's currently about 300 lines of Reflection. Which isn't ideal, I
dislike Reflection as much as anyone, but I think this is the best option
with Go's lack of metaprogramming. At the cost of a little self-contained
Reflection, we can have a single struct and handler, no version logic or
boilerplate, while still providing a better user experience with Semantic
Versioning.
I've also written ~4x as many test lines as code, 130 tests in +1100 lines
of test code verifying correct behavior, and 'go test -cover' reports 90.7%.
Thoughts?
On Wed, Feb 13, 2019 at 10:12 AM Rawlin Peters <rawlin.pet...@gmail.com>
wrote:
> Hey all,
>
> I currently have a number of proposals for the Traffic Ops API in
> terms of development, API versioning, and API client "promises".
>
> ======
> TL;DR:
> ======
>
> 1. We should only honor the major version of the API w.r.t. Semantic
> Versioning. That is, we should do our best to not break v1 by
> introducing backwards-incompatible changes.
> 2. We should accept that the TO API is now strongly-typed due to being
> implemented in Golang, in contrast to the Perl implementation which
> happily accepted numbers as strings and anything "falsey" or "truthy"
> as booleans. Yes, this goes against item 1, but in this case I think
> the breaking change is somewhat of an edge-case and necessary for the
> sake of progress in Go. Please make sure all your TO API clients are
> using numbers where they should be numbers and not strings. Please use
> real booleans True or False rather than things that are "truthy" or
> "falsey". For more information about "truthy" and "falsey" values in
> Perl, see https://perlmaven.com/boolean-values-in-perl.
> 3. We should not require specifying a minor version of the API. That
> is, if a client requests v1.2 as they currently do today, it will just
> be handled as a v1 request on the server.
> 4. TO API clients should expect to receive new, backwards-compatible
> fields in responses as new features are added to API v1. That is, they
> should not explicitly break themselves when getting optional, unknown
> fields in a JSON response.
>
> Goals:
> 1. Keep it easy to add new features to Traffic Ops.
> 2. Keep Traffic Ops maintainable.
> 3. Keep v1 clients compatible with v1 Traffic Ops.
>
> We need existing v1 clients to work against v1 Traffic Ops, but we
> also need to keep Traffic Ops maintainable and easy to work on since
> it is the foundation for nearly all new features in Traffic Control.
>
> Please let me know your thoughts and concerns about what I'm proposing.
>
> ===================
> LONGER EXPLANATION:
> ===================
>
> There seems to be this idea that the TO API should stick to Semantic
> Versioning (https://semver.org/), and for the most part we have been
> trying really hard to avoid breaking the 1.x API with
> backwards-incompatible changes.
>
> However, the reality of the matter is that we _have_ already broken
> the 1.x API due to things like Perl accepting strings as ints (among
> other things), and in order to fix that breaking API change we will
> have to do backflips in Go in order to accept both strings and numbers
> wherever the API accepts numbers (or accept basically anything
> "truthy" or "falsey" where the API accepts booleans).
>
> I think everyone can agree that a strongly-typed API is a good thing,
> but I also understand that breaking clients is bad. However, in this
> case, I don't think we should have to do backflips in
> traffic_ops_golang just to get it to accept strings in places that
> should really be numbers or booleans. That is, we shouldn't have to
> build and use custom types in Go that replicate the behavior of Perl
> types which have both string and scalar values.
>
> The TO API also currently specifies the minor version (i.e.
> <major>.<minor>). Per Semantic Versioning, that means if a client
> makes a 1.2 request to a 1.4 server, the server should really only
> provide 1.2 fields in the response (and ignore/default any 1.4 fields
> in the request) -- what I call the "minor version promise".
>
> I don't know why the project stuck to having minor versions in the API
> since that predates my involvement, but my guess is that the TO API
> version was tied to the overall TC project version at one point but
> then the project version moved on without the API version following
> it. It doesn't seem like we've really honored the "minor version
> promise" until some very recent attempts, and speaking from my own
> experience I've been operating on the "major version promise" when
> developing the TO API. That is, I've added new, optional fields to
> existing 1.2 endpoints in a backwards-compatible manner, and these new
> fields which probably should be marked as 1.3 will show up in
> responses to clients even if the client specifically requests version
> 1.2.
>
> IMO that should be totally fine unless a client has specifically
> chosen to break itself when it receives unknown fields, and none of
> the in-repo TO clients behave that way. I'm not sure what the
> advantage of building a TO API client with that behavior would be,
> since a client can safely ignore new, optional fields that are
> backwards compatible.
>
> However, we currently do have some API endpoints that do their best to
> honor the requested minor version, but it is not an easy and
> maintainable thing to do in Go to have the API return only fields that
> existed at the client's requested API version, due to the lack of
> things like generics/metaprogramming. Currently, for those endpoints
> we have "versioned structs" which declare a different struct for every
> minor version and use struct embedding to specify which fields belong
> to which minor version of the API. This still requires having a
> separate handler for every minor version of an API endpoint, and
> currently the handlers are heavily duplicated which is bad practice
> and will not scale with the amount of additions we make to existing
> API endpoints.
>
> Ideally we would have:
> - 1 handler per endpoint which serves requests within the same major
> version (i.e. if a client requests 1.2, 1.3, or 1.5, this same handler
> would handle all of those requests)
> - 1 struct per "resource", e.g. one for cdn, one for deliveryservice,
> one for server, etc.
>
> What it's really starting to look like is:
> - N handlers for the same endpoint, where N is the number of minor
> versions, all of which are mostly duplicated up to the point where
> "N+1" functionality was added
> - N structs per resource, depending on how many minor versions there
> are, embedded all the way down, with potentially unique methods
> attached to each version
>
> This can be especially bad for things like deliveryservices which are
> returned from multiple different endpoints:
>
> /deliveryservices
> /cachegroups/{id}/deliveryservices
> /users/{id}/deliveryservices
> /users/{id}/deliveryservices/available
> /servers/{id}/deliveryservices
>
> What I'm getting at is that it is going to be a massive amount of dev
> work to implement and maintain unique structs and handlers for every
> single minor version of an API endpoint. We don't currently have a
> great solution in Go that easily allows us to honor the "minor version
> promise" without a huge amount of unnecessary overhead. So we should
> take a step back and contemplate the amount of utility we will get by
> honoring the "minor version promise" in comparison to the amount of
> development, maintenance, and overhead it will require.
>
> Relevant previous threads about TO API versioning:
>
>
https://lists.apache.org/thread.html/8f8a850c68424021a0fe06967894383a24e463f1b0cee4d652d04590@%3Cdev.trafficcontrol.apache.org%3E
>
>
https://lists.apache.org/thread.html/1a42a2192a81fc4d76639ccd10761b6b73c31345a63715bb8aa86e4e@%3Cdev.trafficcontrol.apache.org%3E
>
> - Rawlin
>
