CRUDer deprecation

[ocket 8888 <oc...@gmail.com>]

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.

Re: CRUDer deprecation

[Rawlin Peters <ra...@apache.org>]

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 <ro...@apache.org> 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 <oc...@gmail.com> 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 <oc...@gmail.com> 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 <ra...@apache.org> 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 <oc...@gmail.com> 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.
> > >
> >

Re: CRUDer deprecation

[Robert O Butts <ro...@apache.org>]

>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 <oc...@gmail.com> 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 <oc...@gmail.com> 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 <ra...@apache.org> 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 <oc...@gmail.com> 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.
> >
>

Re: CRUDer deprecation

[ocket 8888 <oc...@gmail.com>]

> 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 <ra...@apache.org> 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 <oc...@gmail.com> 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.
>

Re: CRUDer deprecation

[Rawlin Peters <ra...@apache.org>]

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 <oc...@gmail.com> 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.