I don't think it really makes sense to deprecate things that will live on perpetually. To me, deprecation means there is a plan to remove/replace, and I don't know if I can really get on board removing/replacing the usage of all of that code. Even deprecated, it will still need updates to support new cross-cutting features that we want to provide -- e.g. features akin to If-Modified-Since -- which means we'd be improving/maintaining the deprecated code. Plus, I don't really want to see striked-through code all over the place in GoLand for the rest of TO's life. It's a constant nudge to get rid of it, yet getting rid of it is mostly a waste of time at present.
The nice thing about frameworks is that they force you to be disciplined and consistent and help shape implementations into certain patterns through which the framework can provide a lot of free stuff. It would be a shame if every single route was implemented in such a way that we could no longer find common patterns between them in order to extract them into shared code and provide new cross-cutting features with ease. What are some of the main reasons people "don't like" the framework? If we can nail down specific problems to address, maybe we can make it better instead of deciding to simply get rid of it. - Rawlin On Thu, Oct 15, 2020 at 2:16 PM Robert O Butts <[email protected]> wrote: > > >It's hard to agree to the deprecation of "the Generic CRUDer framework" > when it's not clear what that actually includes > > There are 3 concepts in the `api` package: > > 1. The "CRUDer" framework, the functions which "give" you an endpoint if > you implement an interface: > `func ReadHandler(reader Reader) http.HandlerFunc` > `type Reader interface` > https://github.com/apache/trafficcontrol/blob/83d301/traffic_ops/traffic_ops_golang/api/shared_handlers.go > https://github.com/apache/trafficcontrol/blob/83d301/traffic_ops/traffic_ops_golang/api/shared_interfaces.go > > 2. The "Generic" functions, which actually implement logic for inserting > into SQL, loading from the database, etc: > `func GenericRead(h http.Header, val GenericReader, useIMS bool) ([] > interface{}, error, error, int, *time.Time)` > https://github.com/apache/trafficcontrol/blob/83d301/traffic_ops/traffic_ops_golang/api/generic_crud.go > > 3. The independent, composable functions, which integrate with `net/http`: > `func Parse(r io.Reader, tx *sql.Tx, v ParseValidator) error` > `func NewInfo(r *http.Request, requiredParams []string, intParamNames [] > string) (*APIInfo, error, error, int)` > `func WriteResp(w http.ResponseWriter, r *http.Request, v interface{})` > `func HandleErr(w http.ResponseWriter, r *http.Request, tx *sql.Tx, > statusCode int, userErr error, sysErr error)` > https://github.com/apache/trafficcontrol/blob/83d301/traffic_ops/traffic_ops_golang/api/api.go > > @ocket8888 <[email protected]> Can you clarify which one(s) you're > talking about? And if all 3, the entire package, how new routes will be > written? > > > On Thu, Oct 15, 2020 at 1:58 PM ocket 8888 <[email protected]> wrote: > > > > There is a lot of code in that package... it might be helpful to > > precisely pint out what exactly would be deprecated > > > > Fair enough. I just figured anyone who'd want to vote would already know > > what it is, but the exact exported symbols from that package that we're > > talking about here are: > > > > • GenericCreator > > • GenericReader > > • GenericUpdater > > • GenericDeleter > > • GenericOptionsDeleter > > • GenericCreate > > • GenericCreateNameBasedID > > • TryIfModifiedSinceQuery > > • GenericRead > > • GenericUpdate > > • GenericOptionsDelete > > • GenericDelete > > • KeyFieldInfo > > • GetIntKey > > • GetStringKey > > • ReadHandler > > • DeprecatedReadHandler > > • UpdateHandler > > • DeleteHandler > > • DeprecatedDeleteHandler > > • CreateHandler > > • CRUDer > > • AlertsResponse > > • Identifier > > • Creator > > • MultipleCreator > > • Reader > > • Updater > > • Deleter > > • OptionsDeleter > > • Tenantable > > • APIInfoer > > > > > Does it make sense to ever rewrite simple > > > APIs like those as-is? I strongly agree routes shouldn't be rewritten > > > until they're sufficiently complex to warrant converting it from > > > "Generic CRUDer", but with simple routes like those that will likely > > > never see any new additions, wouldn't that mean "Generic CRUDer" will > > > live on as perpetually deprecated? > > > > yeah, probably. I don't really have a problem with that. If that code never > > has to change, then the fact that it uses this old, deprecated framework > > shouldn't have the chance to bother anyone. > > > > On Wed, Oct 14, 2020 at 3:54 PM Rawlin Peters <[email protected]> wrote: > > > > > There is a lot of code in that package [1]. It's hard to agree to the > > > deprecation of "the Generic CRUDer framework" when it's not clear what > > > that actually includes (especially for people who don't have detailed > > > knowledge of that code). Just so everyone is on the same page, it > > > might be helpful to precisely point out what exactly would be > > > deprecated per this proposal. Then, we can more easily discuss the > > > pros/cons of using "the Generic CRUDer framework" vs. the other > > > method(s). Otherwise, I'm really not sure what we're discussing. > > > > > > I will say, the "framework"-y stuff in TO has its pros and cons, but > > > it does tend to take care of a decent amount of boilerplate code. If > > > we're talking about the same "framework" (we might not be), it does > > > pretty well at handling simple routes like /divisions [2]. For about > > > ~100 LOC you get four fully-functioning routes, fully equipped with > > > decoding, validation, error handling, change logging, etc. Basically > > > all you have to provide is your SQL queries, and you get most of the > > > API functionality for free. Does it make sense to ever rewrite simple > > > APIs like those as-is? I strongly agree routes shouldn't be rewritten > > > until they're sufficiently complex to warrant converting it from > > > "Generic CRUDer", but with simple routes like those that will likely > > > never see any new additions, wouldn't that mean "Generic CRUDer" will > > > live on as perpetually deprecated? > > > > > > - Rawlin > > > > > > [1] > > > > > https://github.com/apache/trafficcontrol/tree/master/traffic_ops/traffic_ops_golang/api > > > [2] > > > > > https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/division/divisions.go > > > > > > On Wed, Oct 14, 2020 at 1:59 PM ocket 8888 <[email protected]> wrote: > > > > > > > > At yesterday's working group, we agreed that none of us like the > > "Generic > > > > CRUDer" framework provided by the > > > > github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api > > > > package. So if nobody has any objections, we'd like to deprecate it. > > > > > > > > That would entail GoDoc deprecation notices, a note in the > > > > documentation/README, and advising authors of new API endpoints against > > > > using it. Existing endpoints would be rewritten to not use the CRUDer > > as > > > > new features are requested of them that rise to a level of complexity > > > that > > > > warrants "CRUDer stripping" at the author's and reviewer's discretions. > > > > >
