If you're deploying the head of master, API minor versioning doesn't really solve that consistent API problem unless we start saying that every single new field added to an API endpoint is a new minor version instead of just incrementing an API's version once per TC release.
Even if we do minor versioning correctly, if we only increment the minor version once per TC release, then consumers of the head of master will code against 1.5, we will add some new fields to master, the consumer will pull master again, and the 1.5 API will now have new fields that weren't there when the consumer first started coding against an earlier version of 1.5. In order to never break API consumers of the head of master that use the latest minor version of the API, we'd have to say that every single new field added on master is a new minor version of the API, even though there haven't been any TC releases in between. In the current state of the code, minor API versioning doesn't even work how it's supposed to, and IMO even having to go through the entire process of incrementing the minor version once per release is too much to ask in its current state. It basically turns a small change that adds a single field into a massive, multi-file change with a lot of duplication, _and_ after doing all that it still won't work for old clients because they will overwrite/default any existing values in later API versions (due to bug #3497). - Rawlin On Thu, Apr 18, 2019 at 10:37 AM Gray, Jonathan <[email protected]> wrote: > > At the end of the day, what I want is a consistent API that I can code > against in the head of master that's treated like a contract. As an API user > outside of the ATC repo it's incredibly frustrating to have my stuff break > all the time. It basically encourages never developing using the latest API > versions (regardless of how they're defined and even then things still break > retroactively) or a non-official OSS release alltogether. It's a catch22 to > be forced to either not vendor the go/python/bash libraries which leads to > constant develop/recompile/deploys in lockstep with ATC or vendor and still > have to do these things when stuff breaks anyway in the API. Really debating > the native client libraries at all is just a red herring because the root > issue is the HTTP API itself which is the real thing to care about since not > all integrations use one of the client libraries, nor can be forced to do so, > and may require a rigid API definition. > > Jonathan G > > > On 4/18/19, 10:12 AM, "Rawlin Peters" <[email protected]> wrote: > > > The UPDATE statements need modified to fix #3497 even if we get rid of > > versioning. Unless we decide to permanently break all clients older than > > the newest server field, with every new server upgrade. The only other > > option is to fix the updates. Unless you know of a way to fix missing > > fields without changing the update statements, that I'm not seeing? > > By removing minor versioning, only certain clients that don't handle > new unknown fields would potentially be broken, and I believe only the > TO Go client has that problem in our repo. However, the TO Go client > happens to use the same Go structs as traffic_ops_golang, so whenever > new fields are added to the API, all the client has to do is recompile > with the up-to-date structs. Unless we made breaking changes to the > client, in most cases all that would be needed for those clients is a > recompile. Traffic Portal, the Python TO client, and I'm pretty sure > the Java TO client all handle unknown fields properly. > > Without minor versions, #3497 would not even an issue. It's only an > issue because of the attempt to support minor versioning. If we just > support the major version, all client requests would be treated as v1, > and there would only ever be one SQL UPDATE statement per major > version. We wouldn't need to "upgrade" 1.2 requests into a 1.4 struct > (thus preventing the bug in #3497) by selecting and inserting all 1.4 > values from the DB into the struct before handling the request or > dynamically generating the SQL UPDATE statement to use based on the > requested minor version. > > > So, this solution actually gives us > > this bug fix almost for free. All that's required is another small > function > > to iterate over the object fields to create the update query. It's by > far > > the easiest and simplest fix for #3497; unless we also permanently break > > all older clients on every server upgrade along with the minor version > > removal. > > Switching all the endpoints over to your "apiver" library would not be > as trivial to implement or remove as you make it sound. It would > require lots of added API test coverage and a non-trivial amount of > code modifications to all API endpoints. Certain UPDATE queries might > be easy to generate from a given struct if the struct only uses a > single table, but I don't think something like that would work for a > field like `cachegroup.LocalizationMethods` which doesn't come from > the cachegroups table and is updated separately from the rest of the > cachegroup fields. > > - Rawlin > >
