[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go
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
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
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
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
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(&p); 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 = ¶meter.HiddenField
+ }
+ 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
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
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(&p); 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 = ¶meter.HiddenField
+ }
+ 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=...
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3900: Rewrite /cachegroups/{{id}}/parameters to Go
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(&cachegroup.TOCacheGroupParameter{}), 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
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
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
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
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
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
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
