rawlinp commented on issue #3296: Change TO cdns/dnsseckeys to abstract versioning URL: https://github.com/apache/trafficcontrol/pull/3296#issuecomment-460461243 > Hm. This prevents new fields from ever being null, and not omitted. If a new field is null, it will be omitted from the JSON object returned (because this scheme requires all new fields to be `omitempty`). We can define new fields to behave that way, so it's ok from a versioning standpoint. But are we ok with that limitation? Are you sure we won't ever encounter a scenario in the future, where we need to return `"newField": null`? New optional fields are meant to have a sane default in most cases (i.e. "null" should be able to be interpreted as something concrete), in which case I'd think the API should just populate the struct with whatever that sane default is. So I don't think that limitation would be a big deal, but it's hard to say for sure. In terms of API guidelines, maybe returning null where null can be interpreted as a sane default with a concrete value should be prohibited anyways. > I'm also not a fan of the `func(interface{}) interface{}`. Empty interfaces throw away all type safety. At the least, this would really need to be `func(interface{}) (interface{}, error)` to avoid panicing. But even then, it's a runtime error, instead of a compile error like it currently is. Sometimes `interface{}` is inevitable, but it'd be really nice to keep these kind of errors at compile time, if we can. I think temporarily dropping down into `interface{}` to nullify fields based on tags then type-asserting back to the concrete value after nullifying would be alright. The runtime error would be easy to avoid since there would only be one concrete struct type for a given endpoint. > It'd also be ideal if we could avoid Reflection, which is inevitable with struct tags. I'm less worried about the performance, than the readability. If someone who didn't write it comes along in the future, how difficult will it be to climb the learning curve, for the reflecting code that's parsing the struct tags? I think the function that parses struct tags to nullify fields with versions greater than the given version would be fairly self-contained. As long as you were somewhat familiar with Go reflection, I'd think it wouldn't be too steep of a learning curve. The function would essentially do one thing and do it well, and produce well-defined outputs for a given input, so it would probably be easy to test with unit tests with various structs as input. > The generated code isn't ideal, but there's no logic there, and whenever Go2 is out, we should be able to use Generics to get rid of it. > > (FYI I attempted to write Go ducktyping/interfaces to abstract the generic stuff, I think it's possible, but it'd be a mess.) > > But, you don't have to touch old versions, when adding a new version. That lets you add e.g. 1.5 without having to touch anything in 1.4, 1.3, 1.2, 1.1. Only the latest-version deliveryservices.go, and things for the version you're adding. > > What do you think of that solution? I like that solution better than having to update all the previous versions of an endpoint when adding a new version, but the deeper I get into this the more I think jumping through all these hoops just to prevent a 1.1 requester from seeing 1.3 fields in their response isn't worth all the hassle. The API should just honor the major version, and clients should be made aware that if they start seeing new fields in their responses, that means they can start using them. If they don't see new fields, then they can't start using fields they've learned about from other TO API deployments that are running the latest code. Updating an API to add a new optional field should be as easy as updating the single existing implementation with the new field. We shouldn't have to copy/paste entire files and jump through all the minor-version hoops just to hide things from the user. Plus, promising to handle all minor versions of an endpoint means maintaining and testing all those minor versions forever, when in reality we are only going to be testing the latest version of the API, and clients should always migrate to the latest available version of the API anyways. If Go 2 gets generics and we find that we can more reasonably handle every single minor version of an endpoint using generics, then we should consider doing so at that point in time. But right now I don't think hiding new, backwards-compatible fields from users is worth duplicating entire files every time a new minor version is required.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
