[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-05 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r321331054
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,111 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+   "strconv"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.CacheGroupParameterNullable
+}
+
+func (cgparam *TOCacheGroupParameter) ParamColumns() 
map[string]dbhelpers.WhereColumnInfo {
+   return map[string]dbhelpers.WhereColumnInfo{
+   CacheGroupIDQueryParam: 
dbhelpers.WhereColumnInfo{"cgp.cachegroup", api.IsInt},
+   ParameterIDQueryParam:  dbhelpers.WhereColumnInfo{"p.id", 
api.IsInt},
+   }
+}
+
+func (cgparam *TOCacheGroupParameter) GetType() string {
+   return "cachegroup_params"
+}
+
+func (cgparam *TOCacheGroupParameter) Read() ([]interface{}, error, error, 
int) {
+   queryParamsToQueryCols := cgparam.ParamColumns()
+   parameters := cgparam.APIInfo().Params
+   where, orderBy, pagination, queryValues, errs := 
dbhelpers.BuildWhereAndOrderByAndPagination(parameters, queryParamsToQueryCols)
+   if len(errs) > 0 {
+   return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+   }
+
+   cgID, err := strconv.Atoi(parameters[CacheGroupIDQueryParam])
 
 Review comment:
   You're right, I just assumed it was doing that because it was parsing it. 
That's actually happening above in 
`dbhelpers.BuildWhereAndOrderByAndPagination`, which doesn't appear to store 
the converted value anywhere... It's a real shame we can't just do that parse 
once.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-05 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r321275087
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,111 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+   "strconv"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.CacheGroupParameterNullable
+}
+
+func (cgparam *TOCacheGroupParameter) ParamColumns() 
map[string]dbhelpers.WhereColumnInfo {
+   return map[string]dbhelpers.WhereColumnInfo{
+   CacheGroupIDQueryParam: 
dbhelpers.WhereColumnInfo{"cgp.cachegroup", api.IsInt},
+   ParameterIDQueryParam:  dbhelpers.WhereColumnInfo{"p.id", 
api.IsInt},
+   }
+}
+
+func (cgparam *TOCacheGroupParameter) GetType() string {
+   return "cachegroup_params"
+}
+
+func (cgparam *TOCacheGroupParameter) Read() ([]interface{}, error, error, 
int) {
+   queryParamsToQueryCols := cgparam.ParamColumns()
+   parameters := cgparam.APIInfo().Params
+   where, orderBy, pagination, queryValues, errs := 
dbhelpers.BuildWhereAndOrderByAndPagination(parameters, queryParamsToQueryCols)
+   if len(errs) > 0 {
+   return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+   }
+
+   cgID, err := strconv.Atoi(parameters[CacheGroupIDQueryParam])
 
 Review comment:
   This is actually handled for you - the "id" in the path is parsed into the 
`APIInfo` structure's `IntParams` map. You can test that by just requesting 
`/cachegroups/notanumber/parameters`, which gives back the error "id cannot 
parse to integer"; it never hits this error handling.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r321002901
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,111 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+   "strconv"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.CacheGroupParameterNullable
+}
+
+func (cgparam *TOCacheGroupParameter) ParamColumns() 
map[string]dbhelpers.WhereColumnInfo {
+   return map[string]dbhelpers.WhereColumnInfo{
+   CacheGroupIDQueryParam: 
dbhelpers.WhereColumnInfo{"cgp.cachegroup", api.IsInt},
+   ParameterIDQueryParam:  dbhelpers.WhereColumnInfo{"p.id", 
api.IsInt},
+   }
+}
+
+func (cgparam *TOCacheGroupParameter) GetType() string {
+   return "cachegroup_params"
+}
+
+func (cgparam *TOCacheGroupParameter) Read() ([]interface{}, error, error, 
int) {
+   queryParamsToQueryCols := cgparam.ParamColumns()
+   parameters := cgparam.APIInfo().Params
+   where, orderBy, pagination, queryValues, errs := 
dbhelpers.BuildWhereAndOrderByAndPagination(parameters, queryParamsToQueryCols)
+   if len(errs) > 0 {
+   return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+   }
+
+   cgID, err := strconv.Atoi(parameters[CacheGroupIDQueryParam])
+   if err != nil {
+   return nil, nil, errors.New("cache group id must be an 
integer"), http.StatusInternalServerError
+   }
+
+   _, ok, err := getCGNameFromID(cgparam.ReqInfo.Tx.Tx, int64(cgID))
+   if err != nil {
+   return nil, nil, errors.New("getting cachegroup from id"), 
http.StatusInternalServerError
 
 Review comment:
   Oh I missed that. Sorry, reviewing on my phone atm.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r321000462
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,111 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+   "strconv"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.CacheGroupParameterNullable
+}
+
+func (cgparam *TOCacheGroupParameter) ParamColumns() 
map[string]dbhelpers.WhereColumnInfo {
+   return map[string]dbhelpers.WhereColumnInfo{
+   CacheGroupIDQueryParam: 
dbhelpers.WhereColumnInfo{"cgp.cachegroup", api.IsInt},
+   ParameterIDQueryParam:  dbhelpers.WhereColumnInfo{"p.id", 
api.IsInt},
+   }
+}
+
+func (cgparam *TOCacheGroupParameter) GetType() string {
+   return "cachegroup_params"
+}
+
+func (cgparam *TOCacheGroupParameter) Read() ([]interface{}, error, error, 
int) {
+   queryParamsToQueryCols := cgparam.ParamColumns()
+   parameters := cgparam.APIInfo().Params
+   where, orderBy, pagination, queryValues, errs := 
dbhelpers.BuildWhereAndOrderByAndPagination(parameters, queryParamsToQueryCols)
+   if len(errs) > 0 {
+   return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+   }
+
+   cgID, err := strconv.Atoi(parameters[CacheGroupIDQueryParam])
+   if err != nil {
+   return nil, nil, errors.New("cache group id must be an 
integer"), http.StatusInternalServerError
+   }
+
+   _, ok, err := getCGNameFromID(cgparam.ReqInfo.Tx.Tx, int64(cgID))
+   if err != nil {
+   return nil, nil, errors.New("getting cachegroup from id"), 
http.StatusInternalServerError
 
 Review comment:
   I think if you check here for `err == sql.ErrNoRows` you can give a better 
message back to the client. If the Cache Group doesn't exist, then the resource 
they're trying to access doesn't exist, and they should get a 404 back. If 
that's not the case, then probably throw your hands up and say something wicked 
happened on the server.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320919332
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,97 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.ParameterNullable
+}
+
+func (cgparam *TOCacheGroupParameter) ParamColumns() 
map[string]dbhelpers.WhereColumnInfo {
+   return map[string]dbhelpers.WhereColumnInfo{
+   CacheGroupIDQueryParam: 
dbhelpers.WhereColumnInfo{"cgp.cachegroup", api.IsInt},
+   ParameterIDQueryParam:  dbhelpers.WhereColumnInfo{"p.id", 
api.IsInt},
+   }
+}
+
+func (cgparam *TOCacheGroupParameter) GetType() string {
+   return "cachegroup_params"
+}
+
+func (cgparam *TOCacheGroupParameter) Read() ([]interface{}, error, error, 
int) {
+   queryParamsToQueryCols := cgparam.ParamColumns()
+   where, orderBy, pagination, queryValues, errs := 
dbhelpers.BuildWhereAndOrderByAndPagination(cgparam.APIInfo().Params, 
queryParamsToQueryCols)
+   if len(errs) > 0 {
+   return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+   }
+
+   query := selectQuery() + where + orderBy + pagination
+   rows, err := cgparam.ReqInfo.Tx.NamedQuery(query, queryValues)
+   if err != nil {
+   return nil, nil, errors.New("querying " + cgparam.GetType() + 
": " + err.Error()), http.StatusInternalServerError
+   }
+   defer rows.Close()
+
+   params := []interface{}{}
+   for rows.Next() {
+   var p tc.ParameterNullable
+   if err = rows.StructScan(); err != nil {
+   return nil, nil, errors.New("scanning " + 
cgparam.GetType() + ": " + err.Error()), http.StatusInternalServerError
+   }
+   if p.Secure != nil && *p.Secure && 
cgparam.ReqInfo.User.PrivLevel < auth.PrivLevelAdmin {
+   p.Value = 
+   }
+   params = append(params, p)
+   }
+
+   return params, nil, nil, http.StatusOK
 
 Review comment:
   That's true... odd. I guess it's just a consequence of the regex used to 
extract the ID. I would think actually checking for the Cache Group's existence 
would be best. Hopefully that's not seen as a breaking API change.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320845597
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,97 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.ParameterNullable
 
 Review comment:
   Using this inserts a new field into the response that didn't exist in the 
Perl implementation: `profiles`. That's fine for a v1.4 route, but to be a 
generic rewrite of 1.x it shouldn't add things. It also omits sub-second 
precision to the `lastUpdated` timestamp, which is likely less of a problem, 
since compliant parsers (probably?) shouldn't care. Might be nice, though.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320848632
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/cachegroup/parameters.go
 ##
 @@ -0,0 +1,97 @@
+package cachegroup
+
+/*
+ * 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 (
+   "errors"
+   "net/http"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+   "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/parameter"
+)
+
+const (
+   CacheGroupIDQueryParam = "id"
+   ParameterIDQueryParam  = "parameterId"
+)
+
+//we need a type alias to define functions on
+type TOCacheGroupParameter struct {
+   api.APIInfoImpl `json:"-"`
+   tc.ParameterNullable
+}
+
+func (cgparam *TOCacheGroupParameter) ParamColumns() 
map[string]dbhelpers.WhereColumnInfo {
+   return map[string]dbhelpers.WhereColumnInfo{
+   CacheGroupIDQueryParam: 
dbhelpers.WhereColumnInfo{"cgp.cachegroup", api.IsInt},
+   ParameterIDQueryParam:  dbhelpers.WhereColumnInfo{"p.id", 
api.IsInt},
+   }
+}
+
+func (cgparam *TOCacheGroupParameter) GetType() string {
+   return "cachegroup_params"
+}
+
+func (cgparam *TOCacheGroupParameter) Read() ([]interface{}, error, error, 
int) {
+   queryParamsToQueryCols := cgparam.ParamColumns()
+   where, orderBy, pagination, queryValues, errs := 
dbhelpers.BuildWhereAndOrderByAndPagination(cgparam.APIInfo().Params, 
queryParamsToQueryCols)
+   if len(errs) > 0 {
+   return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+   }
+
+   query := selectQuery() + where + orderBy + pagination
+   rows, err := cgparam.ReqInfo.Tx.NamedQuery(query, queryValues)
+   if err != nil {
+   return nil, nil, errors.New("querying " + cgparam.GetType() + 
": " + err.Error()), http.StatusInternalServerError
+   }
+   defer rows.Close()
+
+   params := []interface{}{}
+   for rows.Next() {
+   var p tc.ParameterNullable
+   if err = rows.StructScan(); err != nil {
+   return nil, nil, errors.New("scanning " + 
cgparam.GetType() + ": " + err.Error()), http.StatusInternalServerError
+   }
+   if p.Secure != nil && *p.Secure && 
cgparam.ReqInfo.User.PrivLevel < auth.PrivLevelAdmin {
+   p.Value = 
+   }
+   params = append(params, p)
+   }
+
+   return params, nil, nil, http.StatusOK
 
 Review comment:
   This implementation doesn't check for existence of the requested Cache Group.
   
   In Perl:
   ```http
   GET /api/1.3/cachegroups/-1/parameters HTTP/1.1
   User-Agent: python-requests/2.20.1
   Accept-Encoding: gzip, deflate
   Accept: */*
   Connection: keep-alive
   Cookie: mojolicious=...
   
   HTTP/1.1 404 Not Found
   Access-Control-Allow-Credentials: true
   Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept
   Access-Control-Allow-Methods: POST,GET,OPTIONS,PUT,DELETE
   Access-Control-Allow-Origin: *
   Cache-Control: no-cache, no-store, max-age=0, must-revalidate
   Content-Encoding: gzip
   Content-Length: 77
   Content-Type: application/json
   Date: Wed, 04 Sep 2019 16:08:48 GMT
   Server: Mojolicious (Perl)
   Set-Cookie: mojolicious=...; expires=Wed, 04 Sep 2019 20:08:48 GMT; path=/; 
HttpOnly
   Vary: Accept-Encoding
   Whole-Content-Sha512: ...
   
   {
"alerts": [
{
"level": "error",
"text": "Resource not found."
}
]
   }
   ```
   In your rewrite:
   ```http
   GET /api/1.3/cachegroups/-1/parameters HTTP/1.1
   User-Agent: python-requests/2.20.1
   Accept-Encoding: gzip, deflate
   Accept: */*
   Connection: keep-alive
   Cookie: mojolicious=...
   
   HTTP/1.1 200 OK
   

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320760370
 
 

 ##
 File path: traffic_ops/traffic_ops_golang/routing/routes.go
 ##
 @@ -119,6 +119,8 @@ func Routes(d ServerData) ([]Route, []RawRoute, 
http.Handler, error) {
{1.1, http.MethodPost, `cachegroups/{id}/queue_update$`, 
cachegroup.QueueUpdates, auth.PrivLevelOperations, Authenticated, nil},
{1.1, http.MethodPost, `cachegroups/{id}/deliveryservices/?$`, 
cachegroup.DSPostHandler, auth.PrivLevelOperations, Authenticated, nil},
 
+   {1.1, http.MethodGet, `cachegroups/{id}/parameters/?$`, 
api.ReadHandler({}), auth.PrivLevelReadOnly, 
Authenticated, nil},
 
 Review comment:
   I think this needs to support an optional `.json` suffix. In Perl this was 
silently accepted by the server, and clients may have depended on that behavior 
(they shouldn't have IMO, but it was fairly common). In fact, it looks like 
Perl would accept
   
   - `/cachegroups/{{ID}}/parameters.json`
   - `/cachegroups/{{ID}}/parameters`
   - `/cachegroups/{{ID}}/parameters/`
   - `/cachegroups/{{ID}}/parameters/.json`
   - and even `/cachegroups/{{ID}}/parameters.sntoaheusntha`
   
   Pretty sure we don't support that last one, but a lot of handlers (see the 
`cdns/name/{name}/sslkeys` route directly below for an example) support all the 
others.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320754572
 
 

 ##
 File path: traffic_ops/testing/api/v14/cachegroups_parameters_test.go
 ##
 @@ -0,0 +1,91 @@
+// /*
+
+//  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 v14
+
+import (
+   "fmt"
+   "testing"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+
+   "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+func TestCacheGroupParameters(t *testing.T) {
+   WithObjs(t, []TCObj{Types, Parameters, CacheGroups}, func() {
+   GetTestCacheGroupParameters(t)
+   })
+}
+
+func CreateTestCacheGroupParameters(t *testing.T) {
+
+   firstCacheGroup := testData.CacheGroups[0]
+   cacheGroupResp, _, err := 
TOSession.GetCacheGroupByName(*firstCacheGroup.Name)
+   if err != nil {
+   t.Errorf("cannot GET Cache Group by name: %v - %v\n", 
firstCacheGroup.Name, err)
+   }
+
+   firstParameter := testData.Parameters[0]
+   paramResp, _, err := TOSession.GetParameterByName(firstParameter.Name)
 
 Review comment:
   Same as above RE `nil` response


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320755402
 
 

 ##
 File path: traffic_ops/testing/api/v14/cachegroups_parameters_test.go
 ##
 @@ -0,0 +1,91 @@
+// /*
+
+//  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 v14
+
+import (
+   "fmt"
+   "testing"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+
+   "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+func TestCacheGroupParameters(t *testing.T) {
+   WithObjs(t, []TCObj{Types, Parameters, CacheGroups}, func() {
+   GetTestCacheGroupParameters(t)
+   })
+}
+
+func CreateTestCacheGroupParameters(t *testing.T) {
+
+   firstCacheGroup := testData.CacheGroups[0]
+   cacheGroupResp, _, err := 
TOSession.GetCacheGroupByName(*firstCacheGroup.Name)
+   if err != nil {
+   t.Errorf("cannot GET Cache Group by name: %v - %v\n", 
firstCacheGroup.Name, err)
+   }
+
+   firstParameter := testData.Parameters[0]
+   paramResp, _, err := TOSession.GetParameterByName(firstParameter.Name)
+   if err != nil {
+   t.Errorf("cannot GET Parameter by name: %v - %v\n", 
firstParameter.Name, err)
+   }
+
+   cacheGroupID := cacheGroupResp[0].ID
+   parameterID := paramResp[0].ID
+
+   resp, _, err := TOSession.CreateCacheGroupParameter(cacheGroupID, 
parameterID)
+   log.Debugln("Response: ", resp)
+   if err != nil {
+   t.Errorf("could not CREATE cache group parameter: %v\n", err)
+   }
+   testData.CacheGroupParameters = append(testData.CacheGroupParameters, 
resp.Response...)
+}
+
+func GetTestCacheGroupParameters(t *testing.T) {
+   for _, cgp := range testData.CacheGroupParameters {
+   resp, _, err := TOSession.GetCacheGroupParameters(cgp.ID)
+   if err != nil {
+   t.Errorf("cannot GET Parameter by cache group: %v - 
%v\n", err, resp)
+   }
+   }
+}
+
+func DeleteTestCacheGroupParameters(t *testing.T) {
+   for _, cgp := range testData.CacheGroupParameters {
+   DeleteTestCacheGroupParameter(t, cgp)
+   }
+}
+
+func DeleteTestCacheGroupParameter(t *testing.T, cgp tc.CacheGroupParameter) {
+
+   delResp, _, err := TOSession.DeleteCacheGroupParameter(cgp.ID, 
cgp.ParameterID)
+   if err != nil {
+   t.Errorf("cannot DELETE Parameter by cache group: %v - %v\n", 
err, delResp)
+   }
+
+   // Retrieve the Cache Group Parameter to see if it got deleted
+   queryParams := fmt.Sprintf("parameterId=%d", cgp.ParameterID)
+
+   parameters, _, err := 
TOSession.GetCacheGroupParametersByQueryParams(cgp.ID, queryParams)
 
 Review comment:
   Same as above RE `nil` response.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320753420
 
 

 ##
 File path: traffic_ops/testing/api/v14/cachegroups_parameters_test.go
 ##
 @@ -0,0 +1,91 @@
+// /*
+
+//  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 v14
+
+import (
+   "fmt"
+   "testing"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+
+   "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+func TestCacheGroupParameters(t *testing.T) {
+   WithObjs(t, []TCObj{Types, Parameters, CacheGroups}, func() {
+   GetTestCacheGroupParameters(t)
+   })
+}
+
+func CreateTestCacheGroupParameters(t *testing.T) {
+
+   firstCacheGroup := testData.CacheGroups[0]
+   cacheGroupResp, _, err := 
TOSession.GetCacheGroupByName(*firstCacheGroup.Name)
 
 Review comment:
   `cacheGroupResp` can be `nil` here, so maybe you wanna check for that if 
there's an error and just `Fatalf` out. Or actually maybe just return, because 
that might mess up object cleanup (citation needed).
   
   Also, `Session.GetCacheGroupByName` is deprecated, instead use 
[`Session.GetCacheGroupNullableByName`](https://godoc.org/github.com/apache/trafficcontrol/traffic_ops/client#Session.GetCacheGroupNullableByName)


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320748367
 
 

 ##
 File path: lib/go-tc/cachegroup_parameters.go
 ##
 @@ -0,0 +1,32 @@
+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.
+ */
+
+// CacheGroupParameter Cache Group Parameter association
+type CacheGroupParameter struct {
+   ID  int `json:"cachegroupId"`
 
 Review comment:
   Why not `CacheGroupID` to "match" the JSON?


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320754829
 
 

 ##
 File path: traffic_ops/testing/api/v14/cachegroups_parameters_test.go
 ##
 @@ -0,0 +1,91 @@
+// /*
+
+//  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 v14
+
+import (
+   "fmt"
+   "testing"
+
+   "github.com/apache/trafficcontrol/lib/go-tc"
+
+   "github.com/apache/trafficcontrol/lib/go-log"
+)
+
+func TestCacheGroupParameters(t *testing.T) {
+   WithObjs(t, []TCObj{Types, Parameters, CacheGroups}, func() {
+   GetTestCacheGroupParameters(t)
+   })
+}
+
+func CreateTestCacheGroupParameters(t *testing.T) {
+
+   firstCacheGroup := testData.CacheGroups[0]
+   cacheGroupResp, _, err := 
TOSession.GetCacheGroupByName(*firstCacheGroup.Name)
+   if err != nil {
+   t.Errorf("cannot GET Cache Group by name: %v - %v\n", 
firstCacheGroup.Name, err)
+   }
+
+   firstParameter := testData.Parameters[0]
+   paramResp, _, err := TOSession.GetParameterByName(firstParameter.Name)
+   if err != nil {
+   t.Errorf("cannot GET Parameter by name: %v - %v\n", 
firstParameter.Name, err)
+   }
+
+   cacheGroupID := cacheGroupResp[0].ID
+   parameterID := paramResp[0].ID
+
+   resp, _, err := TOSession.CreateCacheGroupParameter(cacheGroupID, 
parameterID)
 
 Review comment:
   Same as above RE nil response


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go

2019-09-04 Thread GitBox
ocket commented on a change in pull request #3900: Rewrite 
/cachegroups/{{id}}/parameters to Go
URL: https://github.com/apache/trafficcontrol/pull/3900#discussion_r320647870
 
 

 ##
 File path: docs/source/api/cachegroups_id_parameters.rst
 ##
 @@ -28,6 +28,14 @@ Gets all the :term:`Parameters` associated with a 
:term:`Cache Group`
 
 Request Structure
 -
+.. table:: Request Query Parameters
+
+   
+-+--+---+
+   | Name| Required | Description  
 |
+   
+=+==+===+
+   | parameterId | no   | Show only the :term:`Paramater` with the 
given ID |
 
 Review comment:
   Spelling error: s/Paramater/Parameter/


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:
us...@infra.apache.org


With regards,
Apache Git Services