ocket8888 commented on a change in pull request #4518: URL: https://github.com/apache/trafficcontrol/pull/4518#discussion_r428329119
########## File path: lib/go-tc/service_category.go ########## @@ -0,0 +1,70 @@ +package tc + +/* + * 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. + */ + +// ServiceCategoriesResponse is a list of Service Categories as a response. +// swagger:response ServiceCategoriesResponse +type ServiceCategoriesResponse struct { + // in: body + Response []ServiceCategory `json:"response"` +} + +// ServiceCategoryResponse is a single Service Category response for Update and Create to +// depict what changed. +// swagger:response ServiceCategoryResponse +// in: body +type ServiceCategoryResponse struct { + // in: body + Response ServiceCategory `json:"response"` +} + +// ServiceCategory ... +type ServiceCategory struct { + + // ServiceCategory ID + // + ID int `json:"id" db:"id"` + + // LastUpdated + // + LastUpdated TimeNoMod `json:"lastUpdated" db:"last_updated"` + + // Division Name + // + // required: true + Name string `json:"name" db:"name"` + + // ServiceCategory Tenant Id + // + TenantID int `json:"tenantId" db:"tenant_id"` + + // ServiceCategory Tenant Name + // + TenantName string `json:"tenant"` +} + +// ServiceCategoryNullable is a nullable struct that holds info about a service category +type ServiceCategoryNullable struct { + ID *int `json:"id" db:"id"` + LastUpdated *TimeNoMod `json:"lastUpdated" db:"last_updated"` + Name *string `json:"name" db:"name"` + TenantID *int `json:"tenantId" db:"tenant_id"` + TenantName *string `json:"tenant"` +} Review comment: We shouldn't have both `ServiceCategory` and `ServiceCategoryNullable`. The "nullable" structs we have today are an artifact of older structures not being designed to be "nullable" - meaning their properties can be "null" - and therefore we had to create new "nullable" structures to get the desired functionality without making breaking changes. tl;dr: we only need `ServiceCategory` and its properties should be "nullable" (if indeed they can be null - it's fine for e.g. Name to be a string if there's no context in which its allowed to be null and we never need to distinguish between a null and empty value). ########## File path: traffic_ops/app/db/migrations/20200519000000_add_deliveryservice_service_category.sql ########## @@ -0,0 +1,26 @@ +/* + Licensed 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. +*/ + +-- +goose Up +-- SQL in section 'Up' is executed when this migration is applied +CREATE TABLE IF NOT EXISTS service_category ( + id SERIAL PRIMARY KEY, + name TEXT NOT NULL, Review comment: Just want to clarify: the name doesn't need to be unique, and can be an empty string? I think that would break a lot of things on the content delivery side. ########## File path: docs/source/api/v2/deliveryservices.rst ########## @@ -31,37 +31,37 @@ Request Structure ----------------- .. table:: Request Query Parameters - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | Name | Required | Description | - +==============+==========+=========================================================================================================================================+ - | cdn | no | Show only the :term:`Delivery Services` belonging to the :ref:`ds-cdn` identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | id | no | Show only the :term:`Delivery Service` that has this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | logsEnabled | no | Show only the :term:`Delivery Services` that have :ref:`ds-logs-enabled` set or not based on this boolean | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | profile | no | Return only :term:`Delivery Services` using the :term:`Profile` that has this :ref:`profile-id` | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | tenant | no | Show only the :term:`Delivery Services` belonging to the :term:`Tenant` identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | type | no | Return only :term:`Delivery Services` of the :term:`Delivery Service` :ref:`ds-types` identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | accessibleTo | no | Return the :term:`Delivery Services` accessible from a :term:`Tenant` *or it's children* identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | xmlId | no | Show only the :term:`Delivery Service` that has this text-based, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | orderby | no | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` | - | | | array | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | sortOrder | no | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | limit | no | Choose the maximum number of results to return | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | offset | no | The number of results to skip before beginning to return results. Must use in conjunction with limit | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | page | no | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long and the first page is 1. | - | | | If ``offset`` was defined, this query parameter has no effect. ``limit`` must be defined to make use of ``page``. | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | Name | Required | Description | + +=================+==========+=========================================================================================================================================+ + | cdn | no | Show only the :term:`Delivery Services` belonging to the :ref:`ds-cdn` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | id | no | Show only the :term:`Delivery Service` that has this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | logsEnabled | no | Show only the :term:`Delivery Services` that have :ref:`ds-logs-enabled` set or not based on this boolean | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | profile | no | Return only :term:`Delivery Services` using the :term:`Profile` that has this :ref:`profile-id` | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | tenant | no | Show only the :term:`Delivery Services` belonging to the :term:`Tenant` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | type | no | Return only :term:`Delivery Services` of the :term:`Delivery Service` :ref:`ds-types` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | accessibleTo | no | Return the :term:`Delivery Services` accessible from a :term:`Tenant` *or it's children* identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | xmlId | no | Show only the :term:`Delivery Service` that has this text-based, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | orderby | no | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` | + | | | array | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | sortOrder | no | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | limit | no | Choose the maximum number of results to return | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | offset | no | The number of results to skip before beginning to return results. Must use in conjunction with limit | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | page | no | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long and the first page is 1. | + | | | If ``offset`` was defined, this query parameter has no effect. ``limit`` must be defined to make use of ``page``. | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ Review comment: Why did you change this table? I don't see any changes to its content, just the structure. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil Review comment: same as above RE: returning vs ignoring errors ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID Review comment: Same as above RE: complete sentence ########## File path: traffic_ops/testing/api/v3/servicecategories_test.go ########## @@ -0,0 +1,137 @@ +package v3 + +/* + + Licensed 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. +*/ + +import ( + "testing" + "time" + + "github.com/apache/trafficcontrol/lib/go-tc" + toclient "github.com/apache/trafficcontrol/traffic_ops/client" +) + +func TestServiceCategories(t *testing.T) { + WithObjs(t, []TCObj{ServiceCategories, Tenants, Users}, func() { + UpdateTestServiceCategories(t) + GetTestServiceCategories(t) + ServiceCategoryTenancyTest(t) + }) +} + +func CreateTestServiceCategories(t *testing.T) { + // loop through service categories, assign FKs and create + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.CreateServiceCategory(sc) + if err != nil { + t.Errorf("could not CREATE service category: %v", err) + } + t.Log("Response: ", resp.Alerts) + } +} + +func GetTestServiceCategories(t *testing.T) { + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.GetServiceCategoryByName(sc.Name) + if err != nil { + t.Errorf("cannot GET Service Category by name: %v - %v", err, resp) + } + } +} + +func UpdateTestServiceCategories(t *testing.T) { + firstServiceCategory := testData.ServiceCategories[0] + // Retrieve the Service Category by service category so we can get the id for the Update + resp, _, err := TOSession.GetServiceCategoryByName(firstServiceCategory.Name) + if err != nil { + t.Errorf("cannot GET Service Category by service category: %v - %v", firstServiceCategory.Name, err) + } + if len(resp) > 0 { + remoteServiceCategory := resp[0] + expectedServiceCategory := "service-category-test" + remoteServiceCategory.Name = expectedServiceCategory + var alert tc.Alerts + alert, _, err = TOSession.UpdateServiceCategoryByID(remoteServiceCategory.ID, remoteServiceCategory) + if err != nil { + t.Errorf("cannot UPDATE Service Category by id: %v - %v", err, alert) + } + + // Retrieve the Service Category to check service category got updated + resp, _, err = TOSession.GetServiceCategoryByID(remoteServiceCategory.ID) + if err != nil { + t.Errorf("cannot GET Service Category by service category: %v - %v", firstServiceCategory.Name, err) + } + + respServiceCategory := resp[0] + if respServiceCategory.Name != expectedServiceCategory { + t.Errorf("results do not match actual: %s, expected: %s", respServiceCategory.Name, expectedServiceCategory) + } + + // Set the name back to the fixture value so we can delete it after + remoteServiceCategory.Name = firstServiceCategory.Name + alert, _, err = TOSession.UpdateServiceCategoryByID(remoteServiceCategory.ID, remoteServiceCategory) + if err != nil { + t.Errorf("cannot UPDATE Service Category by id: %v - %v", err, alert) + } + } +} + +func ServiceCategoryTenancyTest(t *testing.T) { + + toReqTimeout := time.Second * time.Duration(Config.Default.Session.TimeoutInSecs) + tenant4TOClient, _, err := toclient.LoginWithAgent(TOSession.URL, "tenant4user", "pa$$word", true, "to-api-v2-client-tests/tenant4user", true, toReqTimeout) + if err != nil { + t.Fatalf("failed to log in with tenant4user: %v", err.Error()) + } + + serviceCategoriesReadableByTenant4, _, err := tenant4TOClient.GetServiceCategories() + if err != nil { + t.Error("tenant4user cannot GET service categories") + } + + // assert that tenant4user cannot read service categories outside of its tenant + for _, sc := range serviceCategoriesReadableByTenant4 { + if sc.Name == "serviceCategory1" { + t.Error("expected tenant4 to be unable to read service categories from tenant 3") + } + } Review comment: This whole function relies on a specific structure for the test data. If I change something to do some other test, this will break even though the endpoint itself may work fine. Instead, any specifically desired data structure should be set up and torn down by the test. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories +func (to *Session) GetServiceCategories() ([]tc.ServiceCategory, ReqInf, error) { + resp, remoteAddr, err := to.request(http.MethodGet, API_SERVICE_CATEGORIES, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + err = json.NewDecoder(resp.Body).Decode(&data) + return data.Response, reqInf, nil +} + +// GetServiceCategoryByID gets a service category by the service category id +func (to *Session) GetServiceCategoryByID(id int) ([]tc.ServiceCategory, ReqInf, error) { + route := fmt.Sprintf("%s?id=%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodGet, route, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} + +// GetServiceCategoryByName gets a service category by the service category name +func (to *Session) GetServiceCategoryByName(name string) ([]tc.ServiceCategory, ReqInf, error) { + url := fmt.Sprintf("%s?name=%s", API_SERVICE_CATEGORIES, url.QueryEscape(name)) + resp, remoteAddr, err := to.request(http.MethodGet, url, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} Review comment: Per a recent mailing list discussion, this should instead just be one function that takes in optional `url.Values` (i.e. a `*url.Values` so that it can be `nil`) so that callers can filter as they please without needing to maintain multiple functions. ########## File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go ########## @@ -0,0 +1,199 @@ +package servicecategory + +/* + * 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. + */ + +import ( + "fmt" + "github.com/jmoiron/sqlx" + "net/http" + "strconv" + + "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/lib/go-tc/tovalidate" + "github.com/apache/trafficcontrol/lib/go-util" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant" + + validation "github.com/go-ozzo/ozzo-validation" +) + +//we need a type alias to define functions on +type TOServiceCategory struct { + api.APIInfoImpl `json:"-"` + tc.ServiceCategoryNullable +} Review comment: I just wanna say, I think using the "CRUDer" machinery is in general a bigger pain than it's worth. Just my two cents on that. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories +func (to *Session) GetServiceCategories() ([]tc.ServiceCategory, ReqInf, error) { + resp, remoteAddr, err := to.request(http.MethodGet, API_SERVICE_CATEGORIES, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + err = json.NewDecoder(resp.Body).Decode(&data) + return data.Response, reqInf, nil +} + +// GetServiceCategoryByID gets a service category by the service category id +func (to *Session) GetServiceCategoryByID(id int) ([]tc.ServiceCategory, ReqInf, error) { + route := fmt.Sprintf("%s?id=%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodGet, route, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} + +// GetServiceCategoryByName gets a service category by the service category name +func (to *Session) GetServiceCategoryByName(name string) ([]tc.ServiceCategory, ReqInf, error) { + url := fmt.Sprintf("%s?name=%s", API_SERVICE_CATEGORIES, url.QueryEscape(name)) + resp, remoteAddr, err := to.request(http.MethodGet, url, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} + +// DeleteServiceCategoryByID deletes a service category by service category id +func (to *Session) DeleteServiceCategoryByID(id int) (tc.Alerts, ReqInf, error) { + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodDelete, route, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil Review comment: Same as above RE: returning vs ignoring errors ########## File path: docs/source/overview/delivery_services.rst ########## @@ -697,6 +697,12 @@ Servers ------- Servers can be assigned to Delivery Services using the :ref:`tp-configure-servers` and :ref:`tp-services-delivery-service` Traffic Portal sections, or by directly using the :ref:`to-api-deliveryserviceserver` endpoint. Only :term:`Edge-tier cache servers` can be assigned to a Delivery Service, and once they are so assigned they will begin to serve content for the Delivery Service (after updates are queued and then applied). Any servers assigned to a Delivery Service must also belong to the same CDN_ as the Delivery Service itself. At least one server must be assigned to a Delivery Service in order for it to serve any content. +.. _ds-service-category: + +Service Category +---------------- +Defines the type of content being delivered by the Delivery Service. Some example values are: "Linear", "VOD", and "OTT" Review comment: Paragraphs should consist of complete sentences (missing punctuation) Also, I think this explanation could be clearer. This doesn't "define" the type of content because it has no semantic value. This is essentially just like a "tag" for the Delivery Service that more qualitatively "describes" the type of its content. (also also: pls drop that last example as it's very Comcast-specific and nobody should need to know or even wonder what that could be) ########## File path: docs/source/api/v3/servicecategories.rst ########## @@ -0,0 +1,168 @@ +.. +.. +.. Licensed 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. +.. + +.. _to-api-service-categories: + +********************** +``service categories`` +********************** + + +``GET`` +======= +Get all requested :term:`Service Categories`. + +:Auth. Required: Yes +:Roles Required: None +:Response Type: Array + +Request Structure +----------------- +.. table:: Request Query Parameters + + +-----------+---------------------------------------------------------------------------------------------------------------+ + | Name | Description | + +===========+===============================================================================================================+ + | id | Filter for :term:`Service Categories` having this integral, unique identifier | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | name | Filter for :term:`Service Categories` with this name | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | tenant | Return only :term:`Service Categories` belonging to the tenant identified by this integral, unique identifier | | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | orderby | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` | + | | array | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | sortOrder | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | limit | Choose the maximum number of results to return | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | offset | The number of results to skip before beginning to return results. Must use in conjunction with limit | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | page | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long | + | | and the first page is 1. If ``offset`` was defined, this query parameter has no effect. ``limit`` must be | + | | defined to make use of ``page``. | + +-----------+---------------------------------------------------------------------------------------------------------------+ + +.. code-block:: http + :caption: Request Example + + GET /api/3.0/servicecategories?name=SERVICE_CATEGORY_NAME HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + +Response Structure +------------------ +:id: The integral, unique identifier of this :term:`Service Category` +:name: This :term:`Service Category`'s name +:lastUpdated: The date and time at which this :term:`Service Category` was last modified, in ISO format +:tenantId: An integral, unique identifier for the :term:`Tenant` that owns this :term:`Service Category` +:tenant: The name of the :term:`Tenant` that owns this :term:`Service Category` + +.. code-block:: http + :caption: Response Example + + HTTP/1.1 200 OK + Access-Control-Allow-Credentials: true + Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie + Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE + Access-Control-Allow-Origin: * + Content-Type: application/json + Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 18 Nov 2019 17:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: Yzr6TfhxgpZ3pbbrr4TRG4wC3PlnHDDzgs2igtz/1ppLSy2MzugqaGW4y5yzwzl5T3+7q6HWej7GQZt1XIVeZQ== + X-Server-Name: traffic_ops_golang/ + Date: Wed, 11 Mar 2020 20:02:47 GMT + Content-Length: 102 + + { + "response": [ + { + "id": 5, + "lastUpdated": "2020-03-04 15:46:20-07", + "name": "SERVICE_CATEGORY_NAME", + "tenantId": 1, + "tenant": "TENANT_NAME" + } + ] + } + +``POST`` +======== +Create a new service category. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Response Type: Object + +Request Structure +----------------- +:name: This :term:`Service Category`'s name +:tenantId: An integral, unique identifier for the :term:`Tenant` that owns this :term:`Service Category` + +.. code-block:: http + :caption: Request Example + + POST /api/3.0/servicecategories HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 48 + Content-Type: application/json + + { + "name": "SERVICE_CATEGORY_NAME", + "tenantId": 1, Review comment: Same as above RE: indentation. ########## File path: docs/source/api/v3/servicecategories.rst ########## @@ -0,0 +1,168 @@ +.. +.. +.. Licensed 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. +.. + +.. _to-api-service-categories: + +********************** +``service categories`` +********************** + + +``GET`` +======= +Get all requested :term:`Service Categories`. + +:Auth. Required: Yes +:Roles Required: None +:Response Type: Array + +Request Structure +----------------- +.. table:: Request Query Parameters + + +-----------+---------------------------------------------------------------------------------------------------------------+ + | Name | Description | + +===========+===============================================================================================================+ + | id | Filter for :term:`Service Categories` having this integral, unique identifier | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | name | Filter for :term:`Service Categories` with this name | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | tenant | Return only :term:`Service Categories` belonging to the tenant identified by this integral, unique identifier | | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | orderby | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` | + | | array | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | sortOrder | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | limit | Choose the maximum number of results to return | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | offset | The number of results to skip before beginning to return results. Must use in conjunction with limit | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | page | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long | + | | and the first page is 1. If ``offset`` was defined, this query parameter has no effect. ``limit`` must be | + | | defined to make use of ``page``. | + +-----------+---------------------------------------------------------------------------------------------------------------+ + +.. code-block:: http + :caption: Request Example + + GET /api/3.0/servicecategories?name=SERVICE_CATEGORY_NAME HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + +Response Structure +------------------ +:id: The integral, unique identifier of this :term:`Service Category` +:name: This :term:`Service Category`'s name +:lastUpdated: The date and time at which this :term:`Service Category` was last modified, in ISO format +:tenantId: An integral, unique identifier for the :term:`Tenant` that owns this :term:`Service Category` +:tenant: The name of the :term:`Tenant` that owns this :term:`Service Category` + +.. code-block:: http + :caption: Response Example + + HTTP/1.1 200 OK + Access-Control-Allow-Credentials: true + Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie + Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE + Access-Control-Allow-Origin: * + Content-Type: application/json + Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 18 Nov 2019 17:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: Yzr6TfhxgpZ3pbbrr4TRG4wC3PlnHDDzgs2igtz/1ppLSy2MzugqaGW4y5yzwzl5T3+7q6HWej7GQZt1XIVeZQ== + X-Server-Name: traffic_ops_golang/ + Date: Wed, 11 Mar 2020 20:02:47 GMT + Content-Length: 102 + + { + "response": [ + { + "id": 5, + "lastUpdated": "2020-03-04 15:46:20-07", + "name": "SERVICE_CATEGORY_NAME", + "tenantId": 1, + "tenant": "TENANT_NAME" + } + ] + } Review comment: Same as above RE: indentation ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category Review comment: GoDoc should be a complete sentence (end with a <kbd>.</kbd>). ########## File path: traffic_ops/app/db/migrations/20200519000000_add_deliveryservice_service_category.sql ########## @@ -0,0 +1,26 @@ +/* + Licensed 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. +*/ + +-- +goose Up +-- SQL in section 'Up' is executed when this migration is applied +CREATE TABLE IF NOT EXISTS service_category ( + id SERIAL PRIMARY KEY, + name TEXT NOT NULL, + tenant_id INT NOT NULL, Review comment: I think this is meant to be a `bigint` and reference the `tenant` table. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil Review comment: I know you probably copied this from a different method, but please don't just ignore the error on the above line and just return `nil` here - we should return the error. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories Review comment: Same as above RE: complete sentence. ########## File path: lib/go-tc/service_category.go ########## @@ -0,0 +1,70 @@ +package tc + +/* + * 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. + */ + +// ServiceCategoriesResponse is a list of Service Categories as a response. +// swagger:response ServiceCategoriesResponse Review comment: I'm not gonna make you take these out or anything, but just be aware that swagger isn't hooked up to our API in general. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories +func (to *Session) GetServiceCategories() ([]tc.ServiceCategory, ReqInf, error) { + resp, remoteAddr, err := to.request(http.MethodGet, API_SERVICE_CATEGORIES, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + err = json.NewDecoder(resp.Body).Decode(&data) + return data.Response, reqInf, nil Review comment: Same as above RE: returning vs ignoring errors. ########## File path: traffic_ops/app/db/migrations/20200519000000_add_deliveryservice_service_category.sql ########## @@ -0,0 +1,26 @@ +/* + Licensed 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. +*/ + +-- +goose Up +-- SQL in section 'Up' is executed when this migration is applied +CREATE TABLE IF NOT EXISTS service_category ( + id SERIAL PRIMARY KEY, Review comment: If you really need to use a numeric identifier (which, pending an answer to my above question, I hope isn't true), you should use `bigserial` instead of just `serial` - it's what we do everywhere else. ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories +func (to *Session) GetServiceCategories() ([]tc.ServiceCategory, ReqInf, error) { + resp, remoteAddr, err := to.request(http.MethodGet, API_SERVICE_CATEGORIES, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + err = json.NewDecoder(resp.Body).Decode(&data) + return data.Response, reqInf, nil +} + +// GetServiceCategoryByID gets a service category by the service category id Review comment: Same as above RE: complete sentence ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories +func (to *Session) GetServiceCategories() ([]tc.ServiceCategory, ReqInf, error) { + resp, remoteAddr, err := to.request(http.MethodGet, API_SERVICE_CATEGORIES, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + err = json.NewDecoder(resp.Body).Decode(&data) + return data.Response, reqInf, nil +} + +// GetServiceCategoryByID gets a service category by the service category id +func (to *Session) GetServiceCategoryByID(id int) ([]tc.ServiceCategory, ReqInf, error) { + route := fmt.Sprintf("%s?id=%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodGet, route, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} + +// GetServiceCategoryByName gets a service category by the service category name Review comment: Same as above RE: complete sentence ########## File path: traffic_ops/testing/api/v3/servicecategories_test.go ########## @@ -0,0 +1,137 @@ +package v3 + +/* + + Licensed 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. +*/ + +import ( + "testing" + "time" + + "github.com/apache/trafficcontrol/lib/go-tc" + toclient "github.com/apache/trafficcontrol/traffic_ops/client" +) + +func TestServiceCategories(t *testing.T) { + WithObjs(t, []TCObj{ServiceCategories, Tenants, Users}, func() { + UpdateTestServiceCategories(t) + GetTestServiceCategories(t) + ServiceCategoryTenancyTest(t) + }) +} + +func CreateTestServiceCategories(t *testing.T) { + // loop through service categories, assign FKs and create + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.CreateServiceCategory(sc) + if err != nil { + t.Errorf("could not CREATE service category: %v", err) + } + t.Log("Response: ", resp.Alerts) + } +} + +func GetTestServiceCategories(t *testing.T) { + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.GetServiceCategoryByName(sc.Name) + if err != nil { + t.Errorf("cannot GET Service Category by name: %v - %v", err, resp) + } + } +} + +func UpdateTestServiceCategories(t *testing.T) { + firstServiceCategory := testData.ServiceCategories[0] + // Retrieve the Service Category by service category so we can get the id for the Update + resp, _, err := TOSession.GetServiceCategoryByName(firstServiceCategory.Name) + if err != nil { + t.Errorf("cannot GET Service Category by service category: %v - %v", firstServiceCategory.Name, err) + } + if len(resp) > 0 { + remoteServiceCategory := resp[0] + expectedServiceCategory := "service-category-test" Review comment: This is a bit of a nitpick, but this test could give false positives if the category's name was already "service-category-test". Instead it's generally better to concatenate something to the original value, so that you know for sure it's different no matter what it was initially. ########## File path: docs/source/api/v3/deliveryservices.rst ########## @@ -507,6 +516,7 @@ Response Structure "regionalGeoBlocking": false, "remapText": null, "routingName": "test", + "serviceCategory": null, Review comment: Same as above RE: indentation ########## File path: traffic_ops/testing/api/v3/servicecategories_test.go ########## @@ -0,0 +1,137 @@ +package v3 + +/* + + Licensed 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. +*/ + +import ( + "testing" + "time" + + "github.com/apache/trafficcontrol/lib/go-tc" + toclient "github.com/apache/trafficcontrol/traffic_ops/client" +) + +func TestServiceCategories(t *testing.T) { + WithObjs(t, []TCObj{ServiceCategories, Tenants, Users}, func() { + UpdateTestServiceCategories(t) + GetTestServiceCategories(t) + ServiceCategoryTenancyTest(t) + }) +} + +func CreateTestServiceCategories(t *testing.T) { + // loop through service categories, assign FKs and create + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.CreateServiceCategory(sc) + if err != nil { + t.Errorf("could not CREATE service category: %v", err) + } + t.Log("Response: ", resp.Alerts) + } +} + +func GetTestServiceCategories(t *testing.T) { + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.GetServiceCategoryByName(sc.Name) + if err != nil { + t.Errorf("cannot GET Service Category by name: %v - %v", err, resp) + } + } +} + +func UpdateTestServiceCategories(t *testing.T) { + firstServiceCategory := testData.ServiceCategories[0] + // Retrieve the Service Category by service category so we can get the id for the Update + resp, _, err := TOSession.GetServiceCategoryByName(firstServiceCategory.Name) + if err != nil { + t.Errorf("cannot GET Service Category by service category: %v - %v", firstServiceCategory.Name, err) + } + if len(resp) > 0 { + remoteServiceCategory := resp[0] + expectedServiceCategory := "service-category-test" + remoteServiceCategory.Name = expectedServiceCategory + var alert tc.Alerts + alert, _, err = TOSession.UpdateServiceCategoryByID(remoteServiceCategory.ID, remoteServiceCategory) + if err != nil { + t.Errorf("cannot UPDATE Service Category by id: %v - %v", err, alert) + } + + // Retrieve the Service Category to check service category got updated + resp, _, err = TOSession.GetServiceCategoryByID(remoteServiceCategory.ID) + if err != nil { + t.Errorf("cannot GET Service Category by service category: %v - %v", firstServiceCategory.Name, err) + } + + respServiceCategory := resp[0] Review comment: possible segfault if `len(resp)` is 0. ########## File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go ########## @@ -0,0 +1,199 @@ +package servicecategory + +/* + * 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. + */ + +import ( + "fmt" + "github.com/jmoiron/sqlx" + "net/http" + "strconv" + + "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/lib/go-tc/tovalidate" + "github.com/apache/trafficcontrol/lib/go-util" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant" + + validation "github.com/go-ozzo/ozzo-validation" +) + +//we need a type alias to define functions on Review comment: GoDoc should start with the name of the object it describes and be a complete sentence with proper punctuation. ########## File path: docs/source/api/v3/servicecategories.rst ########## @@ -0,0 +1,168 @@ +.. +.. +.. Licensed 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. +.. + +.. _to-api-service-categories: + +********************** +``service categories`` +********************** + + +``GET`` +======= +Get all requested :term:`Service Categories`. + +:Auth. Required: Yes +:Roles Required: None +:Response Type: Array + +Request Structure +----------------- +.. table:: Request Query Parameters + + +-----------+---------------------------------------------------------------------------------------------------------------+ Review comment: This table is malformed because you used spaces for indentation instead of tabs ########## File path: traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go ########## @@ -184,6 +183,29 @@ func CreateV15(w http.ResponseWriter, r *http.Request) { api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Deliveryservice creation was successful.", []tc.DeliveryServiceNullableV15{*res}) } +// TODO allow users to post names (type, cdn, etc) and get the IDs from the names. This isn't trivial to do in a single query, without dynamically building the entire insert query, and ideally inserting would be one query. But it'd be much more convenient for users. Alternatively, remove IDs from the database entirely and use real candidate keys. +func CreateV30(w http.ResponseWriter, r *http.Request) { + inf, userErr, sysErr, errCode := api.NewInfo(r, nil, nil) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, errCode, userErr, sysErr) + return + } + defer inf.Close() + + ds := tc.DeliveryServiceNullableV30{} + if err := json.NewDecoder(r.Body).Decode(&ds); err != nil { + api.HandleErr(w, r, inf.Tx.Tx, http.StatusBadRequest, errors.New("decoding: "+err.Error()), nil) + return + } + + res, status, userErr, sysErr := createV30(w, r, inf, ds) + if userErr != nil || sysErr != nil { + api.HandleErr(w, r, inf.Tx.Tx, status, userErr, sysErr) + return + } + api.WriteRespAlertObj(w, r, tc.SuccessLevel, "Deliveryservice creation was successful.", []tc.DeliveryServiceNullableV30{*res}) Review comment: Per API guidelines this should use a `201 Created` status code, ideally setting a `Location` HTTP Header with a URL that points to the location from whence the new object may be fetched. ########## File path: docs/source/api/v3/deliveryservices.rst ########## @@ -345,6 +352,7 @@ Request Structure "rangeRequestHandling": 0, "regionalGeoBlocking": false, "routingName": "test", + "serviceCategory": null, Review comment: same as above RE: indentation ########## File path: traffic_ops/client/serviceCategory.go ########## @@ -0,0 +1,133 @@ +/* + + Licensed 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. +*/ + +package client + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + + "github.com/apache/trafficcontrol/lib/go-tc" +) + +const ( + API_SERVICE_CATEGORIES = apiBase + "/servicecategories" +) + +// CreateServiceCategory creates a service category +func (to *Session) CreateServiceCategory(serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + resp, remoteAddr, err := to.request(http.MethodPost, API_SERVICE_CATEGORIES, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// UpdateServiceCategoryByID updates a service category by ID +func (to *Session) UpdateServiceCategoryByID(id int, serviceCategory tc.ServiceCategory) (tc.Alerts, ReqInf, error) { + + var remoteAddr net.Addr + reqBody, err := json.Marshal(serviceCategory) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return tc.Alerts{}, reqInf, err + } + route := fmt.Sprintf("%s/%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodPut, route, reqBody) + if err != nil { + return tc.Alerts{}, reqInf, err + } + defer resp.Body.Close() + var alerts tc.Alerts + err = json.NewDecoder(resp.Body).Decode(&alerts) + return alerts, reqInf, nil +} + +// GetServiceCategories returns a list of service categories +func (to *Session) GetServiceCategories() ([]tc.ServiceCategory, ReqInf, error) { + resp, remoteAddr, err := to.request(http.MethodGet, API_SERVICE_CATEGORIES, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + err = json.NewDecoder(resp.Body).Decode(&data) + return data.Response, reqInf, nil +} + +// GetServiceCategoryByID gets a service category by the service category id +func (to *Session) GetServiceCategoryByID(id int) ([]tc.ServiceCategory, ReqInf, error) { + route := fmt.Sprintf("%s?id=%d", API_SERVICE_CATEGORIES, id) + resp, remoteAddr, err := to.request(http.MethodGet, route, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} + +// GetServiceCategoryByName gets a service category by the service category name +func (to *Session) GetServiceCategoryByName(name string) ([]tc.ServiceCategory, ReqInf, error) { + url := fmt.Sprintf("%s?name=%s", API_SERVICE_CATEGORIES, url.QueryEscape(name)) + resp, remoteAddr, err := to.request(http.MethodGet, url, nil) + reqInf := ReqInf{CacheHitStatus: CacheHitStatusMiss, RemoteAddr: remoteAddr} + if err != nil { + return nil, reqInf, err + } + defer resp.Body.Close() + + var data tc.ServiceCategoriesResponse + if err := json.NewDecoder(resp.Body).Decode(&data); err != nil { + return nil, reqInf, err + } + + return data.Response, reqInf, nil +} + +// DeleteServiceCategoryByID deletes a service category by service category id Review comment: Same as above RE: complete sentence ########## File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go ########## @@ -0,0 +1,199 @@ +package servicecategory + +/* + * 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. + */ + +import ( + "fmt" + "github.com/jmoiron/sqlx" + "net/http" + "strconv" + + "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/lib/go-tc/tovalidate" + "github.com/apache/trafficcontrol/lib/go-util" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant" + + validation "github.com/go-ozzo/ozzo-validation" +) + +//we need a type alias to define functions on +type TOServiceCategory struct { + api.APIInfoImpl `json:"-"` + tc.ServiceCategoryNullable +} + +func (v *TOServiceCategory) SetLastUpdated(t tc.TimeNoMod) { v.LastUpdated = &t } +func (v *TOServiceCategory) InsertQuery() string { return insertQuery() } +func (v *TOServiceCategory) NewReadObj() interface{} { return &tc.ServiceCategory{} } +func (v *TOServiceCategory) SelectQuery() string { return selectQuery() } +func (v *TOServiceCategory) UpdateQuery() string { return updateQuery() } +func (v *TOServiceCategory) DeleteQuery() string { return deleteQuery() } + +func (serviceCategory TOServiceCategory) GetAuditName() string { + if serviceCategory.Name != nil { + return *serviceCategory.Name + } + if serviceCategory.ID != nil { + return strconv.Itoa(*serviceCategory.ID) + } + if serviceCategory.TenantID != nil { + return strconv.Itoa(*serviceCategory.TenantID) + } + return "unknown" +} + +func (serviceCategory TOServiceCategory) GetKeyFieldsInfo() []api.KeyFieldInfo { + return []api.KeyFieldInfo{{"id", api.GetIntKey}} +} + +//Implementation of the Identifier, Validator interface functions +func (serviceCategory TOServiceCategory) GetKeys() (map[string]interface{}, bool) { + if serviceCategory.ID == nil { + return map[string]interface{}{"id": 0}, false + } + return map[string]interface{}{"id": *serviceCategory.ID}, true +} + +func (serviceCategory *TOServiceCategory) SetKeys(keys map[string]interface{}) { + i, _ := keys["id"].(int) //this utilizes the non panicking type assertion, if the thrown away ok variable is false i will be the zero of the type, 0 here. + serviceCategory.ID = &i +} + +func (serviceCategory TOServiceCategory) GetType() string { + return "serviceCategory" +} + +func (serviceCategory TOServiceCategory) Validate() error { + errs := validation.Errors{ + "name": validation.Validate(serviceCategory.Name, validation.NotNil, validation.Required), + "tenantId": validation.Validate(serviceCategory.TenantID, validation.Min(1)), Review comment: This isn't everything that needs to be validated here. Specifically, that tenant has to exist. btw, I've been mostly holding off on mentioning things like this until it's fixed in the database, but there are some other places where this kind of thing needs to be checked. ########## File path: docs/source/api/v3/deliveryservices.rst ########## @@ -31,37 +31,39 @@ Request Structure ----------------- .. table:: Request Query Parameters - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | Name | Required | Description | - +==============+==========+=========================================================================================================================================+ - | cdn | no | Show only the :term:`Delivery Services` belonging to the :ref:`ds-cdn` identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | id | no | Show only the :term:`Delivery Service` that has this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | logsEnabled | no | Show only the :term:`Delivery Services` that have :ref:`ds-logs-enabled` set or not based on this boolean | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | profile | no | Return only :term:`Delivery Services` using the :term:`Profile` that has this :ref:`profile-id` | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | tenant | no | Show only the :term:`Delivery Services` belonging to the :term:`Tenant` identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | type | no | Return only :term:`Delivery Services` of the :term:`Delivery Service` :ref:`ds-types` identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | accessibleTo | no | Return the :term:`Delivery Services` accessible from a :term:`Tenant` *or it's children* identified by this integral, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | xmlId | no | Show only the :term:`Delivery Service` that has this text-based, unique identifier | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | orderby | no | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` | - | | | array | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | sortOrder | no | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | limit | no | Choose the maximum number of results to return | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | offset | no | The number of results to skip before beginning to return results. Must use in conjunction with limit | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ - | page | no | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long and the first page is 1. | - | | | If ``offset`` was defined, this query parameter has no effect. ``limit`` must be defined to make use of ``page``. | - +--------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | Name | Required | Description | + +=================+==========+=========================================================================================================================================+ + | cdn | no | Show only the :term:`Delivery Services` belonging to the :ref:`ds-cdn` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | id | no | Show only the :term:`Delivery Service` that has this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | logsEnabled | no | Show only the :term:`Delivery Services` that have :ref:`ds-logs-enabled` set or not based on this boolean | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | profile | no | Return only :term:`Delivery Services` using the :term:`Profile` that has this :ref:`profile-id` | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | tenant | no | Show only the :term:`Delivery Services` belonging to the :term:`Tenant` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | type | no | Return only :term:`Delivery Services` of the :term:`Delivery Service` :ref:`ds-types` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | accessibleTo | no | Return the :term:`Delivery Services` accessible from a :term:`Tenant` *or it's children* identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ + | serviceCategory | no | Show only the :term:`Delivery Services` belonging to the :term:`Service Category` identified by this integral, unique identifier | + +-----------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------+ Review comment: This table is malformed because you used spaces instead of tabs for indentation. ########## File path: traffic_ops/testing/api/v3/servicecategories_test.go ########## @@ -0,0 +1,137 @@ +package v3 + +/* + + Licensed 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. +*/ + +import ( + "testing" + "time" + + "github.com/apache/trafficcontrol/lib/go-tc" + toclient "github.com/apache/trafficcontrol/traffic_ops/client" +) + +func TestServiceCategories(t *testing.T) { + WithObjs(t, []TCObj{ServiceCategories, Tenants, Users}, func() { + UpdateTestServiceCategories(t) + GetTestServiceCategories(t) + ServiceCategoryTenancyTest(t) + }) +} + +func CreateTestServiceCategories(t *testing.T) { + // loop through service categories, assign FKs and create + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.CreateServiceCategory(sc) + if err != nil { + t.Errorf("could not CREATE service category: %v", err) + } + t.Log("Response: ", resp.Alerts) + } +} + +func GetTestServiceCategories(t *testing.T) { + for _, sc := range testData.ServiceCategories { + resp, _, err := TOSession.GetServiceCategoryByName(sc.Name) + if err != nil { + t.Errorf("cannot GET Service Category by name: %v - %v", err, resp) + } + } +} + +func UpdateTestServiceCategories(t *testing.T) { + firstServiceCategory := testData.ServiceCategories[0] Review comment: this will segfault if the testdata has no service categories. Which _shouldn't_ be true, but all the same it's best not to make assumptions about what's in the test data. ########## File path: traffic_ops/traffic_ops_golang/servicecategory/servicecategories.go ########## @@ -0,0 +1,199 @@ +package servicecategory + +/* + * 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. + */ + +import ( + "fmt" + "github.com/jmoiron/sqlx" + "net/http" + "strconv" + + "github.com/apache/trafficcontrol/lib/go-log" + "github.com/apache/trafficcontrol/lib/go-tc" + "github.com/apache/trafficcontrol/lib/go-tc/tovalidate" + "github.com/apache/trafficcontrol/lib/go-util" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers" + "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant" + + validation "github.com/go-ozzo/ozzo-validation" +) + +//we need a type alias to define functions on +type TOServiceCategory struct { + api.APIInfoImpl `json:"-"` + tc.ServiceCategoryNullable +} + +func (v *TOServiceCategory) SetLastUpdated(t tc.TimeNoMod) { v.LastUpdated = &t } +func (v *TOServiceCategory) InsertQuery() string { return insertQuery() } +func (v *TOServiceCategory) NewReadObj() interface{} { return &tc.ServiceCategory{} } +func (v *TOServiceCategory) SelectQuery() string { return selectQuery() } +func (v *TOServiceCategory) UpdateQuery() string { return updateQuery() } +func (v *TOServiceCategory) DeleteQuery() string { return deleteQuery() } + +func (serviceCategory TOServiceCategory) GetAuditName() string { + if serviceCategory.Name != nil { + return *serviceCategory.Name + } + if serviceCategory.ID != nil { + return strconv.Itoa(*serviceCategory.ID) + } + if serviceCategory.TenantID != nil { + return strconv.Itoa(*serviceCategory.TenantID) + } + return "unknown" +} + +func (serviceCategory TOServiceCategory) GetKeyFieldsInfo() []api.KeyFieldInfo { + return []api.KeyFieldInfo{{"id", api.GetIntKey}} +} + +//Implementation of the Identifier, Validator interface functions +func (serviceCategory TOServiceCategory) GetKeys() (map[string]interface{}, bool) { + if serviceCategory.ID == nil { + return map[string]interface{}{"id": 0}, false + } + return map[string]interface{}{"id": *serviceCategory.ID}, true +} + +func (serviceCategory *TOServiceCategory) SetKeys(keys map[string]interface{}) { + i, _ := keys["id"].(int) //this utilizes the non panicking type assertion, if the thrown away ok variable is false i will be the zero of the type, 0 here. + serviceCategory.ID = &i +} + +func (serviceCategory TOServiceCategory) GetType() string { + return "serviceCategory" +} + +func (serviceCategory TOServiceCategory) Validate() error { + errs := validation.Errors{ + "name": validation.Validate(serviceCategory.Name, validation.NotNil, validation.Required), + "tenantId": validation.Validate(serviceCategory.TenantID, validation.Min(1)), + } + return util.JoinErrs(tovalidate.ToErrors(errs)) +} + +func (serviceCategory *TOServiceCategory) Create() (error, error, int) { + return api.GenericCreate(serviceCategory) Review comment: Per API guidelines, this should return a `201 Created` status code, ideally with a `Location` HTTP Header set to a URL from whence the newly created object may be requested. Note that this is impossible with the CRUDer as-is, but you could edit the CRUDer. Of course, that'd change it globally for a ton of endpoints across all API versions. ... aaand that's why I don't recommend using the CRUDer. ########## File path: docs/source/api/v3/servicecategories.rst ########## @@ -0,0 +1,168 @@ +.. +.. +.. Licensed 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. +.. + +.. _to-api-service-categories: + +********************** +``service categories`` +********************** + + +``GET`` +======= +Get all requested :term:`Service Categories`. + +:Auth. Required: Yes +:Roles Required: None +:Response Type: Array + +Request Structure +----------------- +.. table:: Request Query Parameters + + +-----------+---------------------------------------------------------------------------------------------------------------+ + | Name | Description | + +===========+===============================================================================================================+ + | id | Filter for :term:`Service Categories` having this integral, unique identifier | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | name | Filter for :term:`Service Categories` with this name | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | tenant | Return only :term:`Service Categories` belonging to the tenant identified by this integral, unique identifier | | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | orderby | Choose the ordering of the results - must be the name of one of the fields of the objects in the ``response`` | + | | array | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | sortOrder | Changes the order of sorting. Either ascending (default or "asc") or descending ("desc") | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | limit | Choose the maximum number of results to return | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | offset | The number of results to skip before beginning to return results. Must use in conjunction with limit | + +-----------+---------------------------------------------------------------------------------------------------------------+ + | page | Return the n\ :sup:`th` page of results, where "n" is the value of this parameter, pages are ``limit`` long | + | | and the first page is 1. If ``offset`` was defined, this query parameter has no effect. ``limit`` must be | + | | defined to make use of ``page``. | + +-----------+---------------------------------------------------------------------------------------------------------------+ + +.. code-block:: http + :caption: Request Example + + GET /api/3.0/servicecategories?name=SERVICE_CATEGORY_NAME HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + +Response Structure +------------------ +:id: The integral, unique identifier of this :term:`Service Category` +:name: This :term:`Service Category`'s name +:lastUpdated: The date and time at which this :term:`Service Category` was last modified, in ISO format +:tenantId: An integral, unique identifier for the :term:`Tenant` that owns this :term:`Service Category` +:tenant: The name of the :term:`Tenant` that owns this :term:`Service Category` + +.. code-block:: http + :caption: Response Example + + HTTP/1.1 200 OK + Access-Control-Allow-Credentials: true + Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie + Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE + Access-Control-Allow-Origin: * + Content-Type: application/json + Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 18 Nov 2019 17:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: Yzr6TfhxgpZ3pbbrr4TRG4wC3PlnHDDzgs2igtz/1ppLSy2MzugqaGW4y5yzwzl5T3+7q6HWej7GQZt1XIVeZQ== + X-Server-Name: traffic_ops_golang/ + Date: Wed, 11 Mar 2020 20:02:47 GMT + Content-Length: 102 + + { + "response": [ + { + "id": 5, + "lastUpdated": "2020-03-04 15:46:20-07", + "name": "SERVICE_CATEGORY_NAME", + "tenantId": 1, + "tenant": "TENANT_NAME" + } + ] + } + +``POST`` +======== +Create a new service category. + +:Auth. Required: Yes +:Roles Required: "admin" or "operations" +:Response Type: Object + +Request Structure +----------------- +:name: This :term:`Service Category`'s name +:tenantId: An integral, unique identifier for the :term:`Tenant` that owns this :term:`Service Category` + +.. code-block:: http + :caption: Request Example + + POST /api/3.0/servicecategories HTTP/1.1 + Host: trafficops.infra.ciab.test + User-Agent: curl/7.47.0 + Accept: */* + Cookie: mojolicious=... + Content-Length: 48 + Content-Type: application/json + + { + "name": "SERVICE_CATEGORY_NAME", + "tenantId": 1, + } + +Response Structure +------------------ +:id: The integral, unique identifier of this :term:`Service Category` +:name: This :term:`Service Category`'s name +:lastUpdated: The date and time at which this :term:`Service Category` was last modified, in ISO format +:tenantId: An integral, unique identifier for the :term:`Tenant` that owns this :term:`Service Category` +:tenant: The name of the :term:`Tenant` that owns this :term:`Service Category` + +.. code-block:: http + :caption: Response Example + + HTTP/1.1 200 OK + Access-Control-Allow-Credentials: true + Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept, Set-Cookie, Cookie + Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE + Access-Control-Allow-Origin: * + Content-Type: application/json + Set-Cookie: mojolicious=...; Path=/; Expires=Mon, 18 Nov 2019 17:40:54 GMT; Max-Age=3600; HttpOnly + Whole-Content-Sha512: +pJm4c3O+JTaSXNt+LP+u240Ba/SsvSSDOQ4rDc6hcyZ0FIL+iY/WWrMHhpLulRGKGY88bM4YPCMaxGn3FZ9yQ== + X-Server-Name: traffic_ops_golang/ + Date: Wed, 11 Mar 2020 20:12:20 GMT + Content-Length: 154 + + { + "alerts": [ + { + "text": "serviceCategory was created.", + "level": "success" + } + ], + "response": { + "id": 1, + "lastUpdated": "2020-03-11 14:12:20-06", + "name": "SERVICE_CATEGORY_NAME", + "tenantId": 1, + "tenant": null + } + } Review comment: Same as above RE: indentation. ########## File path: docs/source/api/v3/deliveryservices.rst ########## @@ -234,7 +238,9 @@ Response Structure ], "maxOriginConnections": 0, "ecsEnabled": false, - "rangeSliceBlockSize": null + "rangeSliceBlockSize": null, + "serviceCategory": null, + "serviceCategoryName": null Review comment: Use tabs for indentation. Code blocks may contain space indents for their contents, but syntactically the indentation to associate the content with the code block must be tabs. So it's easier to just use tabs throughout. ########## File path: docs/source/api/v3/servicecategories.rst ########## @@ -0,0 +1,168 @@ +.. +.. +.. Licensed 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. +.. + +.. _to-api-service-categories: + +********************** +``service categories`` Review comment: This should be the name of the endpoint. Also, from our API guidelines: > _"Request paths MUST use 'snake_case' to separate words whenever necessary..."_ So really the route name ought to be `service_categories`. ---------------------------------------------------------------- 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: [email protected]
