[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5545: Added filter (host name or ID) to query API route: /servercheck

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #5545:
URL: https://github.com/apache/trafficcontrol/pull/5545#discussion_r579592440



##########
File path: docs/source/api/v3/servercheck.rst
##########
@@ -31,7 +31,32 @@ Fetches identifying and meta information as well as "check" values regarding all
 
 Request Structure
 -----------------
-No parameters available.
+.. table:: Request Query Parameters
+
+	+-----------+------------------------------------------------------------------------------------+
+	| Name      | Description                                                                        |
+	+===========+====================================================================================+
+	| id        | Return only :term:`Servers` with this integral, unique identifier (id)             |
+	+-----------+------------------------------------------------------------------------------------+
+	| name      | Return only :term:`Servers` with this host_name                                    |
+	+-----------+------------------------------------------------------------------------------------+
+
+.. code-block:: http
+	:caption: Request Example
+
+	GET /api/4.0/servercheck?name=edge HTTP/1.1
+	Host: trafficops.infra.ciab.test
+	User-Agent: curl/7.47.0
+	Accept: */*
+	Cookie: mojolicious=...
+
+  OR
+
+  GET /api/4.0/servercheck?id=12 HTTP/1.1

Review comment:
       If you want to add a second request example, you should wrap the second request in another `.. code-block:: http` and make sure the captions are different, like "Request Example with 'name' Query Parameter" and "Request Example with 'id' Query Parameter"

##########
File path: traffic_ops/v4-client/servercheck.go
##########
@@ -17,6 +17,8 @@ package client
 
 import (
 	"github.com/apache/trafficcontrol/lib/go-tc"
+	"net/http"
+	"net/url"

Review comment:
       standard library imports should come before project-local imports

##########
File path: traffic_ops/v4-client/servercheck.go
##########
@@ -41,3 +43,23 @@ func (to *Session) GetServersChecks() ([]tc.GenericServerCheck, tc.Alerts, ReqIn
 	reqInf, err := to.get(API_SERVERCHECK, nil, &response)
 	return response.Response, response.Alerts, reqInf, err
 }
+
+// GetServerCheckWithParamHdr fetches the Delivery Service with the given ID.
+func (to *Session) GetServerCheckWithParamHdr(params url.Values, header http.Header) (*tc.GenericServerCheck, tc.Alerts, ReqInf, error) {

Review comment:
       Can we just name this "GetServerChecks"? If something already exists that doesn't use url.Values and a header we can just get rid of it and replace it with this one that does, since this is the new client.

##########
File path: traffic_ops/traffic_ops_golang/servercheck/servercheck.go
##########
@@ -239,6 +237,35 @@ func DeprecatedReadServersChecks(w http.ResponseWriter, r *http.Request) {
 
 func handleReadServerCheck(inf *api.APIInfo, tx *sql.Tx) ([]tc.GenericServerCheck, error, error, int) {
 	extensions := make(map[string]string)
+
+	// Query Parameters to Database Query column mappings
+	queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+		"id":   dbhelpers.WhereColumnInfo{"servercheck.server", api.IsInt},
+		"name": dbhelpers.WhereColumnInfo{"server.host_name", nil},
+	}
+
+	where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
+	if len(errs) > 0 {
+		return nil, util.JoinErrs(errs), nil, http.StatusBadRequest
+	}
+
+	// where clause is different for servercheck and server table. Also, it differs for the query param.
+	var whereSC, whereSI string
+	if where != "" {
+		for k, _ := range inf.Params {
+			if k == "id" {
+				whereSC = where
+				whereSI = "WHERE (type.name LIKE 'MID%' OR type.name LIKE 'EDGE%') AND server.id=:id "
+			} else {
+				whereSC = ""
+				whereSI = "WHERE (type.name LIKE 'MID%' OR type.name LIKE 'EDGE%') AND server.host_name=:name "
+			}

Review comment:
       It looks like if I give both the actual filtering used here will be random. I can't imagine why I'd want to, but if I did a request for `serverchecks?id=1&hostName=foo` where server \#1 is *not* 'foo', I'd expect no results returned. Based on this code, I think it would probably return results for server \#1, and if I requested `serverchecks?hostName=foo&id=1` I think it'd probably return a _different_ result set - specifically the checks for the 'foo' server - even though I passed the exact same parameters.
   
   I think it'd probably be best to check for each supported parameter's existence and add to the `where` clause appropriately based on whether it already has contents in turn




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