While I see the value in PATCH, Rawlin is spot on: we need defined behaviour around null and missing fields in the patches. (One alternative: jsonpatch. It's more verbose, but clearly defines the edge cases.)
PATCH is also very dangerous unless you support If-Match, which we don't. But that's a problem we should also fix everywhere. It's not unique to this endpoint. On Tue, Jul 30, 2019 at 4:49 PM Rawlin Peters <[email protected]> wrote: > > In my opinion, introducing PATCH methods seems like unnecessary > complexity. We don't really have a good way in TO-Go to support > partial object updates in a holistic manner today, and there are some > difficulties around determining which fields were actually sent by a > client with a null value (e.g. `"foo": null`) vs fields that were > entirely omitted by the client. It would also add to the burden of > testing and maintenance (when a simple PUT implementation would > suffice), and I don't think there's a great way for the TO Go client > to marshal a lib/go-tc struct into a PATCH request that only contains > the fields you'd like to update (sometimes with null/empty values). > > As for PUT, I think we could get by with a POST and a DELETE without a > PUT for this particular endpoint, but I'm not sure I really feel > strongly about that. Providing the ability to PUT kind of encourages > the idea that you don't really have to get your invalidations right > the first time, or that you can just update an existing invalidation > job to change the regex instead of creating a new invalidation with a > different regex (when really they should be created as separate jobs). > If you have a bad revalidation deployed, your first priority should > probably be to get rid of it as quickly as possible (via DELETE) > instead of trying to replace it with a different regex (via PUT). In > that case, I'd think it would be advantageous to only provide the > DELETE option instead of both DELETE and PUT. First delete the bad > invalidation ASAP, then work on a better regex. > > - Rawlin > > On Tue, Jul 30, 2019 at 10:31 AM ocket8888 <[email protected]> wrote: > > > > I have had this PR open for a while: > > https://github.com/apache/trafficcontrol/pull/3744 > > > > I meant to bring this to the mailing list earlier, but I forgot :P > > > > The reason this merits discussion is that the PR adds several method > > handlers to the /jobs endpoint that didn't exist in Perl: > > > > - POST > > > > lets users create new jobs directly at this endpoint. My hope is > > that the /user/current/jobs endpoint will fall into disuse, and we can > > consolidate some functionality in one place. Obviously, this > > necessitates a CDN-wide queue of reval updates. > > > > - PUT > > > > allows jobs to be replaced. This queues reval updates CDN-wide. > > > > - PATCH > > > > allows jobs to be edited. This also queues reval updates CDN-wide > > > > - DELETE > > > > deletes jobs. This, too, queues reval updates CDN-wide > > > > > > Which I think is a good idea. Without any way to mutate created jobs, a > > typo can have dire consequences that can't be taken back. But since > > POST->DELETE->POST is really just editing with more steps, a PUT/PATCH > > seemed to make sense. > > > > > > thoughts? > >
