>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. > > >
