[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4665: Support IMS header in Get endpoints

[GitBox <gi...@apache.org>]

rob05c commented on a change in pull request #4665:
URL: https://github.com/apache/trafficcontrol/pull/4665#discussion_r416000982



##########
File path: lib/go-rfc/http.go
##########
@@ -36,6 +36,7 @@ const (
 	ContentDisposition     = "Content-Disposition"      // RFC6266
 	ApplicationOctetStream = "application/octet-stream" // RFC2046§4.5.2
 	Vary                   = "Vary"                     // RFC7231§7.1.4
+	IfModifiedSince        = "If-Modified-Since"        // RFC1945

Review comment:
       RFC1945 is obsolete, should be RFC7232§3.3
   
   https://tools.ietf.org/html/rfc7232#section-3.3

##########
File path: traffic_ops/app/db/migrations/20200424000000_add_deleted_tables.sql
##########
@@ -0,0 +1,16 @@
+-- +goose Up
+-- SQL in section 'Up' is executed when this migration is applied
+
+-- deleted_type
+CREATE TABLE IF NOT EXISTS deleted_type (
+    id bigint NOT NULL,
+    name text,
+    description text,
+    use_in_table text,
+    last_updated timestamp with time zone DEFAULT now(),
+    deleted_time timestamp with time zone NOT NULL DEFAULT now()

Review comment:
       I think I suggested `deleted_time`, but thinking about it some more, deleted rows should be immutable, so `last_updated` and `deleted_time` will always be identical, right? And while `deleted_time` might be a little more intuitive, `last_updated` matches the convention in every other table, and will probably make it easier to re-use some existing query building stuff.
   
   What do you think about removing `deleted_time` and just using `last_updated` for everything?

##########
File path: traffic_ops/traffic_ops_golang/types/types.go
##########
@@ -158,6 +174,34 @@ func (tp *TOType) loadUseInTable() (error, error, string) {
 	return nil, nil, useInTable
 }
 
+func selectMaxLastUpdatedQuery(where, orderBy, pagination, where2, orderBy2, pagination2 string) string {

Review comment:
       Will `where2`, `orderBy2`, and `pagination2` ever be different? I can't think of a reason they would be. If not, should we just have `selectMaxLastUpdatedQuery(where, orderBy, pagination` and use them for both the `type` and `deleted_type` parts of the query?

##########
File path: traffic_ops/traffic_ops_golang/api/generic_crud.go
##########
@@ -191,8 +271,28 @@ func GenericOptionsDelete(val GenericOptionsDeleter) (error, error, int) {
 	return nil, nil, http.StatusOK
 }
 
+func InsertInDeletedTable(val GenericDeleter, v interface{}) (error, error, int) {
+	err := val.InsertIntoDeletedQuery(v, val.APIInfo().Tx)
+	if err != nil {
+		return ParseDBError(err)
+	}
+	return nil, nil, http.StatusOK
+}
+
 // GenericDelete does a Delete (DELETE) for the given GenericDeleter object and type. This exists as a generic function, for the common use case of a simple delete with query parameters defined in the sqlx struct tags.
 func GenericDelete(val GenericDeleter) (error, error, int) {
+	var v interface{}
+	res, err := val.APIInfo().Tx.NamedQuery(val.SelectBeforeDeleteQuery(), val)

Review comment:
       Instead of selecting, deleting, then inserting the selected, can we just insert right here, and save a query? E.g. `insert into foo_deleted (id) values ($1)` ?

##########
File path: traffic_ops/traffic_ops_golang/types/types.go
##########
@@ -158,6 +174,34 @@ func (tp *TOType) loadUseInTable() (error, error, string) {
 	return nil, nil, useInTable
 }
 
+func selectMaxLastUpdatedQuery(where, orderBy, pagination, where2, orderBy2, pagination2 string) string {

Review comment:
       Also, because all tables need a `last_updated`, and all `deleted_` tables should also be identical, we should be able to use a func to build the query to reduce duplication, e.g. 
   ```
   func MakeLastUpdatedQuery(table string) string {
       return `
   SELECT MAX(t) from (
     SELECT MAX(last_updated) t FROM ` + table + ` ` + where + orderBy + pagination + `
     UNION ALL
     SELECT MAX(last_updated) t from deleted_` + table + ` ` + where + orderBy + pagination + `
   ) as res
   `
   }
   ```
   
   And then all endpoints can use that




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org