[GitHub] rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning
rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning URL: https://github.com/apache/trafficcontrol/pull/2364#discussion_r196461033 ## File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv12.go ## @@ -133,35 +96,45 @@ func getDSTenantIDByName(db *sql.DB, name string) (*int, bool, error) { } // GetXMLID loads the DeliveryService's xml_id from the database, from the ID. Returns whether the delivery service was found, and any error. -func (ds *TODeliveryServiceV12) GetXMLID(db *sqlx.DB) (string, bool, error) { +func (ds *TODeliveryServiceV12) GetXMLID(tx *sql.Tx) (string, bool, error) { if ds.ID == nil { return "", false, errors.New("missing ID") } + return GetXMLID(tx, *ds.ID) +} + +// GetXMLID loads the DeliveryService's xml_id from the database, from the ID. Returns whether the delivery service was found, and any error. +func GetXMLID(tx *sql.Tx, id int) (string, bool, error) { xmlID := "" - if err := db.QueryRow(`SELECT xml_id FROM deliveryservice where id = $1`, ds.ID).Scan(); err != nil { + if err := tx.QueryRow(`SELECT xml_id FROM deliveryservice where id = $1`, id).Scan(); err != nil { if err == sql.ErrNoRows { return "", false, nil } - return "", false, fmt.Errorf("querying xml_id for delivery service ID '%v': %v", *ds.ID, err) + return "", false, fmt.Errorf("querying xml_id for delivery service ID '%v': %v", id, err) } return xmlID, true, nil } // IsTenantAuthorized checks that the user is authorized for both the delivery service's existing tenant, and the new tenant they're changing it to (if different). -func (ds *TODeliveryServiceV12) IsTenantAuthorized(user auth.CurrentUser, db *sqlx.DB) (bool, error) { - return isTenantAuthorized(user, db, ) +func (ds *TODeliveryServiceV12) IsTenantAuthorized(user *auth.CurrentUser, db *sqlx.DB) (bool, error) { + tx, err := db.DB.Begin() // must be last, MUST not return an error if this suceeds, without closing the tx + if err != nil { + return false, errors.New("beginning transaction: " + err.Error()) + } + defer dbhelpers.FinishTx(tx, util.BoolPtr(true)) + return isTenantAuthorized(user, tx, (*tc.DeliveryServiceNullableV12)(ds)) } -func isTenantAuthorized(user auth.CurrentUser, db *sqlx.DB, ds *tc.DeliveryServiceNullableV12) (bool, error) { +func isTenantAuthorized(user *auth.CurrentUser, tx *sql.Tx, ds *tc.DeliveryServiceNullableV12) (bool, error) { if ds.ID == nil && ds.XMLID == nil { return false, errors.New("delivery service has no ID or XMLID") } existingID, err := (*int)(nil), error(nil) Review comment: Changed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning
rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning URL: https://github.com/apache/trafficcontrol/pull/2364#discussion_r196448115 ## File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv12.go ## @@ -133,35 +96,45 @@ func getDSTenantIDByName(db *sql.DB, name string) (*int, bool, error) { } // GetXMLID loads the DeliveryService's xml_id from the database, from the ID. Returns whether the delivery service was found, and any error. -func (ds *TODeliveryServiceV12) GetXMLID(db *sqlx.DB) (string, bool, error) { +func (ds *TODeliveryServiceV12) GetXMLID(tx *sql.Tx) (string, bool, error) { if ds.ID == nil { return "", false, errors.New("missing ID") } + return GetXMLID(tx, *ds.ID) +} + +// GetXMLID loads the DeliveryService's xml_id from the database, from the ID. Returns whether the delivery service was found, and any error. +func GetXMLID(tx *sql.Tx, id int) (string, bool, error) { xmlID := "" - if err := db.QueryRow(`SELECT xml_id FROM deliveryservice where id = $1`, ds.ID).Scan(); err != nil { + if err := tx.QueryRow(`SELECT xml_id FROM deliveryservice where id = $1`, id).Scan(); err != nil { if err == sql.ErrNoRows { return "", false, nil } - return "", false, fmt.Errorf("querying xml_id for delivery service ID '%v': %v", *ds.ID, err) + return "", false, fmt.Errorf("querying xml_id for delivery service ID '%v': %v", id, err) } return xmlID, true, nil } // IsTenantAuthorized checks that the user is authorized for both the delivery service's existing tenant, and the new tenant they're changing it to (if different). -func (ds *TODeliveryServiceV12) IsTenantAuthorized(user auth.CurrentUser, db *sqlx.DB) (bool, error) { - return isTenantAuthorized(user, db, ) +func (ds *TODeliveryServiceV12) IsTenantAuthorized(user *auth.CurrentUser, db *sqlx.DB) (bool, error) { + tx, err := db.DB.Begin() // must be last, MUST not return an error if this suceeds, without closing the tx + if err != nil { + return false, errors.New("beginning transaction: " + err.Error()) + } + defer dbhelpers.FinishTx(tx, util.BoolPtr(true)) + return isTenantAuthorized(user, tx, (*tc.DeliveryServiceNullableV12)(ds)) } -func isTenantAuthorized(user auth.CurrentUser, db *sqlx.DB, ds *tc.DeliveryServiceNullableV12) (bool, error) { +func isTenantAuthorized(user *auth.CurrentUser, tx *sql.Tx, ds *tc.DeliveryServiceNullableV12) (bool, error) { if ds.ID == nil && ds.XMLID == nil { return false, errors.New("delivery service has no ID or XMLID") } existingID, err := (*int)(nil), error(nil) Review comment: Correct, I didn't change it. I agree. In fact, using a single declaration syntax everywhere is even better, ``` existingID := (*int)(nil) err := error(nil) ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning
rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning URL: https://github.com/apache/trafficcontrol/pull/2364#discussion_r196448115 ## File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservicesv12.go ## @@ -133,35 +96,45 @@ func getDSTenantIDByName(db *sql.DB, name string) (*int, bool, error) { } // GetXMLID loads the DeliveryService's xml_id from the database, from the ID. Returns whether the delivery service was found, and any error. -func (ds *TODeliveryServiceV12) GetXMLID(db *sqlx.DB) (string, bool, error) { +func (ds *TODeliveryServiceV12) GetXMLID(tx *sql.Tx) (string, bool, error) { if ds.ID == nil { return "", false, errors.New("missing ID") } + return GetXMLID(tx, *ds.ID) +} + +// GetXMLID loads the DeliveryService's xml_id from the database, from the ID. Returns whether the delivery service was found, and any error. +func GetXMLID(tx *sql.Tx, id int) (string, bool, error) { xmlID := "" - if err := db.QueryRow(`SELECT xml_id FROM deliveryservice where id = $1`, ds.ID).Scan(); err != nil { + if err := tx.QueryRow(`SELECT xml_id FROM deliveryservice where id = $1`, id).Scan(); err != nil { if err == sql.ErrNoRows { return "", false, nil } - return "", false, fmt.Errorf("querying xml_id for delivery service ID '%v': %v", *ds.ID, err) + return "", false, fmt.Errorf("querying xml_id for delivery service ID '%v': %v", id, err) } return xmlID, true, nil } // IsTenantAuthorized checks that the user is authorized for both the delivery service's existing tenant, and the new tenant they're changing it to (if different). -func (ds *TODeliveryServiceV12) IsTenantAuthorized(user auth.CurrentUser, db *sqlx.DB) (bool, error) { - return isTenantAuthorized(user, db, ) +func (ds *TODeliveryServiceV12) IsTenantAuthorized(user *auth.CurrentUser, db *sqlx.DB) (bool, error) { + tx, err := db.DB.Begin() // must be last, MUST not return an error if this suceeds, without closing the tx + if err != nil { + return false, errors.New("beginning transaction: " + err.Error()) + } + defer dbhelpers.FinishTx(tx, util.BoolPtr(true)) + return isTenantAuthorized(user, tx, (*tc.DeliveryServiceNullableV12)(ds)) } -func isTenantAuthorized(user auth.CurrentUser, db *sqlx.DB, ds *tc.DeliveryServiceNullableV12) (bool, error) { +func isTenantAuthorized(user *auth.CurrentUser, tx *sql.Tx, ds *tc.DeliveryServiceNullableV12) (bool, error) { if ds.ID == nil && ds.XMLID == nil { return false, errors.New("delivery service has no ID or XMLID") } existingID, err := (*int)(nil), error(nil) Review comment: Correct, I didn't change it. I agree. In fact, using a single declaration syntax everywhere is even better, ``` existingID := (*int)(nil) err := error(nil) ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning
rob05c commented on a change in pull request #2364: Fix Traffic Ops Go to use transactions for deliveryservices, sslkeys, urisigning URL: https://github.com/apache/trafficcontrol/pull/2364#discussion_r196446230 ## File path: lib/go-tc/deliveryservices.go ## @@ -205,6 +216,213 @@ type DeliveryServiceNullableV13 struct { TRRequestHeaders *string `json:"trRequestHeaders,omitempty"` } +// NewDeliveryServiceNullableV13FromV12 creates a new V13 DS from a V12 DS, filling new fields with appropriate defaults. +func NewDeliveryServiceNullableV13FromV12(ds DeliveryServiceNullableV12) DeliveryServiceNullableV13 { + newDS := DeliveryServiceNullableV13{DeliveryServiceNullableV12: ds} + newDS.Sanitize() + return newDS +} + +func (ds *DeliveryServiceNullableV12) Sanitize() { + if ds.GeoLimitCountries != nil { + *ds.GeoLimitCountries = strings.ToUpper(strings.Replace(*ds.GeoLimitCountries, " ", "", -1)) + } + if ds.ProfileID != nil && *ds.ProfileID == -1 { + ds.ProfileID = nil + } + if ds.EdgeHeaderRewrite != nil && strings.TrimSpace(*ds.EdgeHeaderRewrite) == "" { + ds.EdgeHeaderRewrite = nil + } + if ds.MidHeaderRewrite != nil && strings.TrimSpace(*ds.MidHeaderRewrite) == "" { + ds.MidHeaderRewrite = nil + } + if ds.RoutingName == nil || *ds.RoutingName == "" { + ds.RoutingName = util.StrPtr(DefaultRoutingName) + } +} + +func getTypeName(tx *sql.Tx, id int) (string, bool, error) { + name := "" + if err := tx.QueryRow(`SELECT name from type where id=$1`, id).Scan(); err != nil { + if err == sql.ErrNoRows { + return "", false, nil + } + return "", false, errors.New("querying type name: " + err.Error()) + } + return name, true, nil +} + +func requiredIfMatchesTypeName(patterns []string, typeName string) func(interface{}) error { + return func(value interface{}) error { + switch v := value.(type) { + case *int: + if v != nil { + return nil + } + case *bool: + if v != nil { + return nil + } + case *string: + if v != nil { + return nil + } + case *float64: + if v != nil { + return nil + } + default: + return fmt.Errorf("validation failure: unknown type %T", value) + } + pattern := strings.Join(patterns, "|") Review comment: I didn't write, just moved This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services