ocket8888 commented on code in PR #7224: URL: https://github.com/apache/trafficcontrol/pull/7224#discussion_r1035362614
########## CHANGELOG.md: ########## @@ -24,6 +24,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - [#7113](https://github.com/apache/trafficcontrol/pull/7113) *Traffic Portal* Minimize the Server Server Capability part of the *Traffic Servers* section of the Snapshot Diff ### Changed +- *Traffic Ops* Required Parameters are now a part of the `DeliveryService` structure. Review Comment: These need to have PR or issue links now ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +CREATE TABLE IF NOT EXISTS deliveryservices_required_capability ( + required_capability TEXT NOT NULL, + deliveryservice_id bigint NOT NULL, + last_updated timestamp with time zone DEFAULT now() NOT NULL, + + PRIMARY KEY (deliveryservice_id, required_capability) Review Comment: Constraints should be explicitly named so they're easy to manipulate as the need arises (except defaults and nullability, since those can be manipulated without knowing their name). ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +CREATE TABLE IF NOT EXISTS deliveryservices_required_capability ( + required_capability TEXT NOT NULL, Review Comment: Personally, I like leaving datatypes lowercased, but they are technically keywords so it's fair to uppercase them - but you should pick. If `TEXT` is in all caps, then `BIGINT` should be too. Or they can both be lowercase. ########## docs/source/api/v4/deliveryservices.rst: ########## @@ -148,6 +148,10 @@ Response Structure :regional: A boolean value defining the :ref:`ds-regional` setting on this :term:`Delivery Service` :regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on this :term:`Delivery Service` :remapText: :ref:`ds-raw-remap` +:requiredCapabilities: An array of the capabilities that this delivery service requires. + + .. versionchanged:: 4.1 Review Comment: this should be `versionadded`, not `versionchanged` ########## traffic_ops/testing/api/v4/deliveryservice_requests_test.go: ########## @@ -408,9 +408,10 @@ func validatePutDSRequestFields(expectedResp map[string]interface{}) utils.CkReq func CreateTestDeliveryServiceRequests(t *testing.T) { for _, dsr := range testData.DeliveryServiceRequests { - resetDS(dsr.Original) - resetDS(dsr.Requested) - respDSR, _, err := TOSession.CreateDeliveryServiceRequest(dsr, client.RequestOptions{}) + dsrV41 := dsr.Upgrade() Review Comment: instead of updating on-the-fly, the test data should just use the v41 structure ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.up.sql: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +ALTER TABLE public.deliveryservice + ADD COLUMN required_capabilities text[]; Review Comment: fwiw, this change doesn't strictly require a migration at all. Idk if that's easier or not, but it's worth saying now because once it's in it's pretty much just in. ########## docs/source/api/v4/deliveryservice_requests.rst: ########## @@ -163,6 +163,7 @@ The response is an array of representations of :term:`Delivery Service Requests` "regional": false, "regionalGeoBlocking": false, "remapText": null, + "requiredCapabilities": null, Review Comment: From [our API guidelines](https://traffic-control-cdn.readthedocs.io/en/latest/development/api_guidelines.html#response): > The proper value of an empty collection is an empty collection. `requiredCapabilities` is a collection, so the proper way to represent none of them is `[]`, not `null` ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -939,11 +937,9 @@ func GetDSRequiredCapabilitiesFromID(id int, tx *sql.Tx) ([]string, error) { caps := []string{} for rows.Next() { - var cap string Review Comment: this should now return exactly one row, so instead of looping over returned rows it's probably simpler to just use `QueryRow` above ########## docs/source/api/v4/deliveryservices.rst: ########## @@ -328,6 +333,10 @@ Request Structure :regional: A boolean value defining the :ref:`ds-regional` setting on this :term:`Delivery Service` :regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on this :term:`Delivery Service` :remapText: :ref:`ds-raw-remap` +:requiredCapabilities: An array of the capabilities that this delivery service requires. + + .. versionchanged:: 4.1 Review Comment: Same as above ########## docs/source/api/v4/deliveryservices_id.rst: ########## @@ -249,6 +254,10 @@ Response Structure :regexRemap: A :ref:`ds-regex-remap` :regional: A boolean value defining the :ref:`ds-regional` setting on this :term:`Delivery Service` :regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on this :term:`Delivery Service` +:requiredCapabilities: An array of the capabilities that this delivery service requires. + + .. versionchanged:: 4.1 Review Comment: same as above ########## docs/source/api/v4/deliveryservices_id.rst: ########## @@ -81,6 +81,10 @@ Request Structure :regional: A boolean value defining the :ref:`ds-regional` setting on this :term:`Delivery Service` :regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on this :term:`Delivery Service` :remapText: :ref:`ds-raw-remap` +:requiredCapabilities: An array of the capabilities that this delivery service requires. + + .. versionchanged:: 4.1 Review Comment: same as above ########## docs/source/api/v4/deliveryservices.rst: ########## @@ -495,6 +505,10 @@ Response Structure :regional: A boolean value defining the :ref:`ds-regional` setting on this :term:`Delivery Service` :regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on this :term:`Delivery Service` :remapText: :ref:`ds-raw-remap` +:requiredCapabilities: An array of the capabilities that this delivery service requires. + + .. versionchanged:: 4.1 Review Comment: same as above ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +CREATE TABLE IF NOT EXISTS deliveryservices_required_capability ( Review Comment: this should explicitly place the table in the `public` schema ########## lib/go-tc/deliveryservices.go: ########## @@ -1437,6 +1438,8 @@ type DeliveryServiceV50 struct { // use, because the input is in no way restricted, validated, or limited in // scope to the Delivery Service. RemapText *string `json:"remapText" db:"remap_text"` + // RequiredCapabilities is an array of capabilities required for this delivery service Review Comment: nit: comment should end with punctuation ########## docs/source/api/v4/servers_id_deliveryservices.rst: ########## @@ -134,6 +134,10 @@ Response Structure :regional: A boolean value defining the :ref:`ds-regional` setting on this :term:`Delivery Service` :regionalGeoBlocking: A boolean defining the :ref:`ds-regionalgeo` setting on this :term:`Delivery Service` :remapText: :ref:`ds-raw-remap` +:requiredCapabilities: An array of the capabilities that this delivery service requires. + + .. versionchanged:: 4.1 Review Comment: same as above ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +CREATE TABLE IF NOT EXISTS deliveryservices_required_capability ( + required_capability TEXT NOT NULL, + deliveryservice_id bigint NOT NULL, + last_updated timestamp with time zone DEFAULT now() NOT NULL, + + PRIMARY KEY (deliveryservice_id, required_capability) + ); + +DO $$ Review Comment: I don't think plpgsql scripting is necessary for this. You can just [use a query as a source for the `INSERT` statement](https://www.postgresql.org/docs/current/sql-insert.html) e.g. `INSERT INTO foo(bar) SELECT bar FROM foobar`. ########## traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go: ########## @@ -1584,12 +1591,37 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error { if err := validateTypeFields(tx, ds); err != nil { errs = append(errs, fmt.Errorf("type fields: %w", err)) } + if err := validateRequiredCapabilities(tx, ds); err != nil { + errs = append(errs, errors.New("required capabilities: "+err.Error())) + } if len(errs) == 0 { return nil } return util.JoinErrs(errs) } +func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) error { + var valid bool + query := `SELECT $1 <@ (SELECT ARRAY_AGG(name) FROM server_capability)` + if ds.RequiredCapabilities != nil && len(ds.RequiredCapabilities) > 0 { + rows, err := tx.Query(query, pq.Array(ds.RequiredCapabilities)) + if err != nil { + return err + } + defer rows.Close() + for rows.Next() { + err = rows.Scan(&valid) + if err != nil { + return err + } + if !valid { + return errors.New("one or more of the required capabilities do not exist") Review Comment: what would be _most_ useful for the user is to know _which_ server capabilities don't exist. Using the old method of storing the relationships in a separate table, it was more or less trivial to just add an `EXISTS` condition in a `WHERE` clause, but with arrays the easiest way is with the intarray plugin. But you could also do something like ```sql SELECT cap FROM UNNEST($1::text[]) AS caps(cap) WHERE EXISTS ( SELECT c FROM ( SELECT UNNEST(required_capabilities) FROM public.deliveryservice WHERE id = $2 ) AS dscaps(c) WHERE c = cap ); ``` (untested, just off the top of my head) ########## lib/go-tc/deliveryservice_requests.go: ########## @@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct { XMLID *string `json:"-" db:"xml_id"` } +// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40 +func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 { Review Comment: I think the receiver for this should be `DeliveryServiceRequestV41`. Because our minor-version-less structures are moving targets, if we add a 4.2 then we'd want to downgrade that to v4.1 instead of all the way down to 4.0, so that it's easy to make 4.1 responses. But that means that the call signature would change for the V4 DSR, because it now returns V41 instead of V40 but it's still tied to the V4 structure as though nothing had changed. ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.up.sql: ########## @@ -0,0 +1,30 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +ALTER TABLE public.deliveryservice + ADD COLUMN required_capabilities text[]; + +DO $$ +DECLARE temprow RECORD; +BEGIN FOR temprow IN +select deliveryservice_id, array_agg(required_capability) as required_capabilities from deliveryservices_required_capability drc group by drc.deliveryservice_id + LOOP +update deliveryservice d set required_capabilities = temprow.required_capabilities where d.id = temprow.deliveryservice_id; +END LOOP; +END $$; + +DROP TABLE public.deliveryservices_required_capability; Review Comment: basically all the same comments as the down script apply here as well. ########## traffic_ops/app/db/migrations/2022112215022300_add_required_capabilities_to_ds.down.sql: ########## @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +CREATE TABLE IF NOT EXISTS deliveryservices_required_capability ( + required_capability TEXT NOT NULL, + deliveryservice_id bigint NOT NULL, + last_updated timestamp with time zone DEFAULT now() NOT NULL, + + PRIMARY KEY (deliveryservice_id, required_capability) + ); + +DO $$ +DECLARE temprow RECORD; +BEGIN FOR temprow IN +select id as deliveryservice_id, unnest(required_capabilities) as required_capability from deliveryservice d group by d.id, d.required_capabilities + LOOP +insert into deliveryservices_required_capability ("deliveryservice_id", "required_capability") values (temprow.deliveryservice_id, temprow.required_capability); Review Comment: (Postgre)SQL keywords should be in ALL_CAPS ########## traffic_ops/testing/api/v3/deliveryservices_test.go: ########## @@ -226,8 +226,9 @@ func TestDeliveryServices(t *testing.T) { "BAD REQUEST when ADDING TOPOLOGY to DS with DS REQUIRED CAPABILITY": { EndpointID: GetDeliveryServiceId(t, "ds1"), ClientSession: TOSession, RequestBody: generateDeliveryService(t, map[string]interface{}{ - "topology": "top-for-ds-req", - "xmlId": "ds1", + "requiredCapabilities": []string{"foo"}, Review Comment: this should be unnecessary in APIv3, as that field doesn't exist there, so this is just some unrecognized superfluous data. ########## lib/go-tc/deliveryservice_requests.go: ########## @@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct { XMLID *string `json:"-" db:"xml_id"` } +// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40 +func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 { + var dsrV40 DeliveryServiceRequestV40 + dsrV40.Assignee = dsr.Assignee + dsrV40.AssigneeID = dsr.AssigneeID + dsrV40.Author = dsr.Author + dsrV40.AuthorID = dsr.AuthorID + dsrV40.ChangeType = dsr.ChangeType + dsrV40.CreatedAt = dsr.CreatedAt + dsrV40.ID = dsr.ID + dsrV40.LastEditedBy = dsr.LastEditedBy + dsrV40.LastEditedByID = dsr.LastEditedByID + dsrV40.LastUpdated = dsr.LastUpdated + if dsr.Original != nil { + dsrV40.Original = &dsr.Original.DeliveryServiceV40 + } + if dsr.Requested != nil { + dsrV40.Requested = &dsr.Requested.DeliveryServiceV40 + } + dsrV40.Status = dsr.Status + dsrV40.XMLID = dsr.XMLID + return dsrV40 +} + +// Upgrade will convert an instance of DeliveryServiceRequestV40 to DeliveryServiceRequestV4 +func (dsrV40 DeliveryServiceRequestV40) Upgrade() DeliveryServiceRequestV4 { Review Comment: Same as above; should probably return a DSRV41 instead of V4 ########## lib/go-tc/deliveryservice_requests.go: ########## @@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct { XMLID *string `json:"-" db:"xml_id"` } +// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40 +func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 { + var dsrV40 DeliveryServiceRequestV40 + dsrV40.Assignee = dsr.Assignee + dsrV40.AssigneeID = dsr.AssigneeID + dsrV40.Author = dsr.Author + dsrV40.AuthorID = dsr.AuthorID + dsrV40.ChangeType = dsr.ChangeType + dsrV40.CreatedAt = dsr.CreatedAt + dsrV40.ID = dsr.ID + dsrV40.LastEditedBy = dsr.LastEditedBy + dsrV40.LastEditedByID = dsr.LastEditedByID + dsrV40.LastUpdated = dsr.LastUpdated + if dsr.Original != nil { + dsrV40.Original = &dsr.Original.DeliveryServiceV40 + } + if dsr.Requested != nil { + dsrV40.Requested = &dsr.Requested.DeliveryServiceV40 + } + dsrV40.Status = dsr.Status + dsrV40.XMLID = dsr.XMLID + return dsrV40 +} + +// Upgrade will convert an instance of DeliveryServiceRequestV40 to DeliveryServiceRequestV4 +func (dsrV40 DeliveryServiceRequestV40) Upgrade() DeliveryServiceRequestV4 { + var dsrV4 DeliveryServiceRequestV41 + dsrV4.Assignee = dsrV40.Assignee + dsrV4.AssigneeID = dsrV40.AssigneeID + dsrV4.Author = dsrV40.Author + dsrV4.AuthorID = dsrV40.AuthorID + dsrV4.ChangeType = dsrV40.ChangeType + dsrV4.CreatedAt = dsrV40.CreatedAt + dsrV4.ID = dsrV40.ID + dsrV4.LastEditedBy = dsrV40.LastEditedBy + dsrV4.LastEditedByID = dsrV40.LastEditedByID + dsrV4.LastUpdated = dsrV40.LastUpdated + if dsrV40.Original != nil { + dsrV4.Original = &DeliveryServiceV4{DeliveryServiceV40: *dsrV40.Original} Review Comment: Ideally for these you should just store the address of the result of upgrading/downgrading the underlying DS. ########## traffic_ops/traffic_ops_golang/api/shared_handlers.go: ########## @@ -571,41 +588,59 @@ func CreateHandler(creator Creator) http.HandlerFunc { if sysErr != nil { code = http.StatusInternalServerError } - HandleErr(w, r, inf.Tx.Tx, code, userErr, sysErr) + errHandler(w, r, inf.Tx.Tx, code, userErr, sysErr) return } if t, ok := obj.(Tenantable); ok { authorized, err := t.IsTenantAuthorized(inf.User) if err != nil { - HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("checking tenant authorized: "+err.Error())) + errHandler(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, errors.New("checking tenant authorized: "+err.Error())) return } if !authorized { - HandleErr(w, r, inf.Tx.Tx, http.StatusForbidden, errors.New("not authorized on this tenant"), nil) + errHandler(w, r, inf.Tx.Tx, http.StatusForbidden, errors.New("not authorized on this tenant"), nil) return } } userErr, sysErr, errCode = obj.Create() if userErr != nil || sysErr != nil { - HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + errHandler(w, r, inf.Tx.Tx, errCode, userErr, sysErr) return } if err := CreateChangeLog(ApiChange, Created, obj, inf.User, inf.Tx.Tx); err != nil { - HandleErr(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("inserting changelog: %w", err)) + errHandler(w, r, inf.Tx.Tx, http.StatusInternalServerError, nil, fmt.Errorf("inserting changelog: %w", err)) return } alerts := tc.CreateAlerts(tc.SuccessLevel, obj.GetType()+" was created.") if alertsObj, hasAlerts := obj.(AlertsResponse); hasAlerts { alerts.AddAlerts(alertsObj.GetAlerts()) } - WriteAlertsObj(w, r, http.StatusOK, alerts, obj) + successHandler(w, r, http.StatusOK, alerts, obj) } } } +// DeprecatedCreateHandler creates a net/http.HandlerFunc for the passed Creator object, and adds a deprecation +// notice, optionally with a passed alternative route suggestion. +func DeprecatedCreateHandler(creator Creator, alternative *string) http.HandlerFunc { + return createHandlerHelper( + creator, + func(w http.ResponseWriter, r *http.Request, tx *sql.Tx, statusCode int, userErr error, sysErr error) { + HandleDeprecatedErr(w, r, tx, statusCode, userErr, sysErr, alternative) + }, + func(w http.ResponseWriter, r *http.Request, statusCode int, alerts tc.Alerts, results interface{}) { + depAlerts := CreateDeprecationAlerts(alternative) + for _, al := range alerts.Alerts { + depAlerts.Alerts = append(depAlerts.Alerts, al) + } Review Comment: you don't need to do that by hand, there's a method on `tc.Alerts` to do that for you: [`tc.Alerts.AddAlerts`](https://pkg.go.dev/github.com/apache/trafficcontrol/lib/go-tc#Alerts.AddAlerts). ########## lib/go-tc/deliveryservice_requests.go: ########## @@ -435,6 +435,54 @@ type DeliveryServiceRequestNullable struct { XMLID *string `json:"-" db:"xml_id"` } +// Downgrade will convert an instance of DeliveryServiceRequestV4 to DeliveryServiceRequestV40 +func (dsr DeliveryServiceRequestV4) Downgrade() DeliveryServiceRequestV40 { + var dsrV40 DeliveryServiceRequestV40 + dsrV40.Assignee = dsr.Assignee + dsrV40.AssigneeID = dsr.AssigneeID + dsrV40.Author = dsr.Author + dsrV40.AuthorID = dsr.AuthorID + dsrV40.ChangeType = dsr.ChangeType + dsrV40.CreatedAt = dsr.CreatedAt + dsrV40.ID = dsr.ID + dsrV40.LastEditedBy = dsr.LastEditedBy + dsrV40.LastEditedByID = dsr.LastEditedByID + dsrV40.LastUpdated = dsr.LastUpdated + if dsr.Original != nil { + dsrV40.Original = &dsr.Original.DeliveryServiceV40 + } + if dsr.Requested != nil { + dsrV40.Requested = &dsr.Requested.DeliveryServiceV40 + } + dsrV40.Status = dsr.Status + dsrV40.XMLID = dsr.XMLID + return dsrV40 +} + +// Upgrade will convert an instance of DeliveryServiceRequestV40 to DeliveryServiceRequestV4 +func (dsrV40 DeliveryServiceRequestV40) Upgrade() DeliveryServiceRequestV4 { + var dsrV4 DeliveryServiceRequestV41 + dsrV4.Assignee = dsrV40.Assignee + dsrV4.AssigneeID = dsrV40.AssigneeID + dsrV4.Author = dsrV40.Author Review Comment: the V4 -> v5 conversion and its inverse are deep copies, but this makes a shallow copy. There's no good reason I can think of why it needs to be deep, but I think if you're going to make it shallow it's best to make it clear since the behavior is going to be different depending on what version you're upgrading/downgrading to/from. ########## lib/go-tc/deliveryservices.go: ########## @@ -1726,6 +1731,11 @@ func (ds DeliveryServiceV4) Upgrade() DeliveryServiceV5 { copy(upgraded.MatchList, *ds.MatchList) } copy(upgraded.TLSVersions, ds.TLSVersions) + if ds.RequiredCapabilities == nil { + upgraded.RequiredCapabilities = nil Review Comment: same as above ########## docs/source/api/v3/deliveryservices_required_capabilities.rst: ########## @@ -19,6 +19,8 @@ ``deliveryservices_required_capabilities`` ****************************************** +.. deprecated:: ATCv7 Review Comment: A few notes on these deprecation notices - all of APIv3 is deprecated, so it's superfluous to note that for any specific endpoint/method thereof - If an entire endpoint is being deprecated, you don't also need to individually deprecate each request method it supports - Generally you want to specify the _API_ version that something was deprecated, unless it's not possible to do that for some reason or it just makes more sense to list the ATC version instead. - Ideally the notice will come with some information about why it's being deprecated and/or what to use instead. This makes generating changelogs using Sphinx much more useful. - ########## traffic_ops/traffic_ops_golang/dbhelpers/db_helpers.go: ########## @@ -850,13 +850,11 @@ func GetRequiredCapabilitiesOfDeliveryServices(ids []int, tx *sql.Tx) (map[int][ dsCaps := make(map[int][]string, len(ids)) q := ` SELECT - d.id, - ARRAY_REMOVE(ARRAY_AGG(dsrc.required_capability ORDER BY dsrc.required_capability), NULL) AS required_capabilities -FROM deliveryservice d -LEFT JOIN deliveryservices_required_capability dsrc on d.id = dsrc.deliveryservice_id -WHERE - d.id = ANY($1) -GROUP BY d.id + ds.id, + ARRAY_REMOVE((ds.required_capabilities), NULL) AS required_capabilities Review Comment: Just to further expand on what I said earlier about not needing a migration, this collection of required Server Capabilities is represented by an array most everywhere, because that's such a common structure for languages and encoding formats to implement. However, in reality, it's more like a concrete set: It should be impossible to insert `NULL` and there should be no duplicates. The easiest way to satisfy those constraints and never have to worry about it anywhere else is to use a table where the primary key includes the intersection of a Delivery Service and any one of the Server Capabilities it requires, where null entries are not allowed. Of course, that means using array functions and possibly separate queries for manipulating them, so there's a trade-off there. ########## traffic_ops/traffic_ops_golang/server/servers.go: ########## @@ -825,10 +826,20 @@ func getServers(h http.Header, params map[string]string, tx *sqlx.Tx, user *auth } var joinSubQuery string - if err = tx.QueryRow(deliveryservice.HasRequiredCapabilitiesQuery, dsID).Scan(&dsHasRequiredCapabilities); err != nil { + rows, err := tx.Query(deliveryservice.GetRequiredCapabilitiesQuery, dsID) + if err != nil { err = fmt.Errorf("unable to get required capabilities for deliveryservice %d: %s", dsID, err) return nil, 0, nil, err, http.StatusInternalServerError, nil } + for rows.Next() { + if err = rows.Scan(pq.Array(&requiredCapabilities)); err != nil { + err = fmt.Errorf("unable to scan required capabilities for deliveryservice %d: %s", dsID, err) Review Comment: creating an error from an error should wrap with the `%w` formatting verb ########## traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go: ########## @@ -1584,12 +1591,37 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error { if err := validateTypeFields(tx, ds); err != nil { errs = append(errs, fmt.Errorf("type fields: %w", err)) } + if err := validateRequiredCapabilities(tx, ds); err != nil { + errs = append(errs, errors.New("required capabilities: "+err.Error())) + } if len(errs) == 0 { return nil } return util.JoinErrs(errs) } +func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) error { + var valid bool + query := `SELECT $1 <@ (SELECT ARRAY_AGG(name) FROM server_capability)` + if ds.RequiredCapabilities != nil && len(ds.RequiredCapabilities) > 0 { Review Comment: the nil check here is superfluous, `len(nil)` is zero, so the second check covers it ########## traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go: ########## @@ -1584,12 +1591,37 @@ func Validate(tx *sql.Tx, ds *tc.DeliveryServiceV5) error { if err := validateTypeFields(tx, ds); err != nil { errs = append(errs, fmt.Errorf("type fields: %w", err)) } + if err := validateRequiredCapabilities(tx, ds); err != nil { + errs = append(errs, errors.New("required capabilities: "+err.Error())) + } if len(errs) == 0 { return nil } return util.JoinErrs(errs) } +func validateRequiredCapabilities(tx *sql.Tx, ds *tc.DeliveryServiceV5) error { Review Comment: This function returns both raw database errors and user-friendly errors, but doesn't distinguish between which is which. It should probably return two errors -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
