You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/08/25 06:50:09 UTC
[GitHub] [trafficcontrol] dmohan001c opened a new pull request #6003: Fix Internal server error in Get Profiles by invalid Params
dmohan001c opened a new pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003
## What does this PR (Pull Request) do?
- [x] This PR fixes #5510 <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
This updates the Error message in GET profiles?param=test and profiles?cdn=test
## Which Traffic Control components are affected by this PR?
- CDN in a Box
- Traffic Control Client <!-- Please specify which; e.g. 'Python', 'Go', 'Java' -->
- Traffic Ops
- CI tests
## What is the best way to verify this PR?
Execute all the Integration tests and make sure the tests are passed.
## If this is a bug fix, what versions of Traffic Control are affected?
* Master
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [x] This PR includes tests OR I have explained why tests are unnecessary
- [x] This PR includes documentation OR I have explained why documentation is unnecessary
- [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r695906448
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
We should probably check for the error here.
##########
File path: traffic_ops/v3-client/profile.go
##########
@@ -127,7 +127,7 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, toclientlib.ReqI
}
func (to *Session) GetProfileByParameterWithHdr(param string, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", APIProfiles, url.QueryEscape(param))
+ URI := fmt.Sprintf("%s?param=%s", APIProfiles, param)
Review comment:
Same comment as above here.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
Also, as I suggested earlier, instead of breaking the way pre existing client methods work, why not create a new client method that takes in a param ID instead of a param name, and then make the `GET` call? Something like this:
```
func (to *Session) GetProfileByParameterID(paramID string, header http.Header) ([]tc.Profile, ReqInf, error) {
uri := fmt.Sprintf("%s?param=%d", APIProfiles, paramID)
var data tc.ProfilesResponse
reqInf, err := to.get(uri, header, &data)
return data.Response, reqInf, err
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700010192
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
If we keep the existing methods as per your advice, if someone accidentally calls the existing method, it will throw an error, since that API is not anymore supporting string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r710455567
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -269,10 +288,28 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
Review comment:
same
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -83,12 +83,31 @@ func GetTestProfilesIMS(t *testing.T) {
if reqInf.StatusCode != http.StatusNotModified {
t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
}
- _, reqInf, err = TOSession.GetProfileByParameterWithHdr(pr.Parameter, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- if reqInf.StatusCode != http.StatusNotModified {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
+ t.Errorf("Cannot GET Parameter by name: %v - %v", err, respParameter)
Review comment:
Same.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -178,6 +178,21 @@ func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, er
return data.Response, reqInf, err
}
+func (to *Session) GetProfileByParameterId(param int) ([]tc.Profile, ReqInf, error) {
Review comment:
Godoc is missing.
##########
File path: traffic_ops/testing/api/v2/profiles_test.go
##########
@@ -210,12 +210,29 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
+ t.Errorf("Cannot GET Parameter by name: %v - %v", err, resp)
Review comment:
This `resp` is the wrong response here. Also, I don't think you need to print the response here, just the error is fine.
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -517,12 +517,31 @@ func GetTestProfiles(t *testing.T) {
}
profileID := resp.Response[0].ID
- // TODO: figure out what the 'Parameter' field of a Profile is and how
- // passing it to this is supposed to work.
- // resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- // if err != nil {
- // t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
- // }
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ opts.QueryParameters.Set("name", *parameter.Name)
+ respParameter, _, _ := TOSession.GetParameters(opts)
Review comment:
You should be checking the error here, as you are doing in the v2 and v3 tests.
##########
File path: traffic_ops/testing/api/v2/profiles_test.go
##########
@@ -210,12 +210,29 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
+ t.Errorf("Cannot GET Parameter by name: %v - %v", err, resp)
+ }
+ if len(respParameter) > 0 {
+ parameterID := respParameter[0].ID
+ if parameterID > 0 {
+ resp, _, err = TOSession.GetProfileByParameterId(parameterID)
+ if err != nil {
+ t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
Review comment:
Same comment here, no need to print the response.
##########
File path: traffic_ops/v3-client/profile.go
##########
@@ -139,6 +139,19 @@ func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, toclientli
return to.GetProfileByParameterWithHdr(param, nil)
}
+func (to *Session) GetProfileByParameterIdWithHdr(param int, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
Review comment:
Godoc is missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700000453
##########
File path: traffic_ops/v3-client/profile.go
##########
@@ -127,7 +127,7 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, toclientlib.ReqI
}
func (to *Session) GetProfileByParameterWithHdr(param string, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", APIProfiles, url.QueryEscape(param))
+ URI := fmt.Sprintf("%s?param=%s", APIProfiles, param)
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r682633505
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +149,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, err, errors.New("profile read querying: " + err.Error()), http.StatusBadRequest, nil
Review comment:
added another validation for the param and cdn to make sure it's an integer.
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +149,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, err, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
Review comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c closed pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c closed pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r708196913
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -518,11 +518,31 @@ func GetTestProfiles(t *testing.T) {
profileID := resp.Response[0].ID
// TODO: figure out what the 'Parameter' field of a Profile is and how
Review comment:
I haven't added this line. You only asked me keep this line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r708211420
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -269,10 +285,25 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)
Review comment:
added error checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r711587016
##########
File path: traffic_ops/testing/api/v2/profiles_test.go
##########
@@ -210,12 +210,29 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
+ t.Errorf("Cannot GET Parameter by name: %v - %v", err, resp)
+ }
+ if len(respParameter) > 0 {
+ parameterID := respParameter[0].ID
+ if parameterID > 0 {
+ resp, _, err = TOSession.GetProfileByParameterId(parameterID)
+ if err != nil {
+ t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
Review comment:
removed the response from print statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700000359
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't think a new client is required, and I verified all the existing call to this method are passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r675791183
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +149,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, err, errors.New("profile read querying: " + err.Error()), http.StatusBadRequest, nil
Review comment:
This is an internal database error and should probably remain as an internal server error.
Instead, what you can do is add another validation for the `param` query parameter to make sure it's an integer.
Also, you can add the `JOIN` clause for the `profile_parameter` to the main select query (just like the `cdn` join part), instead of checking for it separately.
Additionally, as mentioned in the GH issue, there are a lot of client methods that use the profile param as a string(name), instead of an int(id). We should probably fix those in this PR as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r682633505
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +149,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, err, errors.New("profile read querying: " + err.Error()), http.StatusBadRequest, nil
Review comment:
added another validation for the param and cdn to make sure it's an integer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r682633703
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +149,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, err, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
Review comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r708209911
##########
File path: traffic_ops/testing/api/v2/profiles_test.go
##########
@@ -210,12 +210,26 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)
Review comment:
added error checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r711588169
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -517,12 +517,31 @@ func GetTestProfiles(t *testing.T) {
}
profileID := resp.Response[0].ID
- // TODO: figure out what the 'Parameter' field of a Profile is and how
- // passing it to this is supposed to work.
- // resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- // if err != nil {
- // t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
- // }
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ opts.QueryParameters.Set("name", *parameter.Name)
+ respParameter, _, _ := TOSession.GetParameters(opts)
Review comment:
added error checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r711590046
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -269,10 +288,28 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
Review comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r703176446
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
Created a new client method
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r687735824
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685702611
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
If I didn't change the where as per above statement, the query looks incorrect.
SELECT prof.description, prof.id, prof.last_updated, prof.name, prof.routing_disabled, prof.type, c.id as cdn, c.name as cdn_name FROM profile prof
LEFT JOIN cdn c ON prof.cdn = c.id
WHERE pp.parameter=:param
LEFT JOIN profile_parameter pp ON prof.id = pp.profile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r705722672
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -517,12 +517,31 @@ func GetTestProfiles(t *testing.T) {
}
profileID := resp.Response[0].ID
- // TODO: figure out what the 'Parameter' field of a Profile is and how
Review comment:
You can probably remove these commented lines now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r684958283
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -123,6 +124,10 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
+ //Check for interger value
Review comment:
updated the DB Query to support params in GET profiles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r707461349
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -269,10 +285,25 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)
Review comment:
We should probably check for the returned error here.
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -83,12 +83,28 @@ func GetTestProfilesIMS(t *testing.T) {
if reqInf.StatusCode != http.StatusNotModified {
t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
}
- _, reqInf, err = TOSession.GetProfileByParameterWithHdr(pr.Parameter, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- if reqInf.StatusCode != http.StatusNotModified {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)
Review comment:
We should probably check for the returned error here.
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -518,11 +518,31 @@ func GetTestProfiles(t *testing.T) {
profileID := resp.Response[0].ID
// TODO: figure out what the 'Parameter' field of a Profile is and how
Review comment:
Any reason to keep that first line in there still?
##########
File path: traffic_ops/testing/api/v2/profiles_test.go
##########
@@ -210,12 +210,26 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)
Review comment:
We should probably check for the returned error here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685691936
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
with the current changes the query comes out to be like this:
`SELECT prof.description, prof.id, prof.last_updated, prof.name, prof.routing_disabled, prof.type, c.id as cdn, c.name as cdn_name FROM profile prof
LEFT JOIN cdn c ON prof.cdn = c.id LEFT JOIN profile_parameter pp ON prof.id = pp.profile
WHERE pp.parameter=:param AND pp.parameter=:parameter_id`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r666302695
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -21,11 +21,12 @@ package profile
import (
"errors"
- "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
"net/http"
"strconv"
"time"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
+
Review comment:
extraneous space
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +150,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, errors.New("bad request found"), errors.New("profile read querying: " + err.Error()), http.StatusBadRequest, nil
Review comment:
So if you use `github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api.ParseDBError` you can get possibly some more information that will help the client correct their request, rather than just telling them it was bad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685703156
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
V2 AND V3 profiles tests are failing. I am taking a look on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685357558
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
Once you add the `ParamQueryParam` to the `queryParamsToQueryCols`, the `BuildWhereAndOrderByAndPagination` function will automatically add the `WHERE` clause to the query, you don't need to modify the `where` variable above. Instead, what you can do is add just the ` LEFT JOIN profile_parameter pp ON prof.id = pp.profile` part if the `parameter_id` param is present.
With the current changes, the query comes out to be this:
``
SELECT
prof.description,
prof.id,
prof.last_updated,
prof.name,
prof.routing_disabled,
prof.type,
c.id as cdn,
c.name as cdn_name
FROM profile prof
LEFT JOIN cdn c ON prof.cdn = c.id LEFT JOIN profile_parameter pp ON prof.id = pp.profile
WHERE pp.parameter=:param AND pp.parameter=:parameter_id
``
We don't need the two `WHERE` clauses in our query. Also, I think the tests are still failing for API v2 and v3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700000359
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't think a new client is required, and I verified all the existing call to this method are passed.
##########
File path: traffic_ops/v3-client/profile.go
##########
@@ -127,7 +127,7 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, toclientlib.ReqI
}
func (to *Session) GetProfileByParameterWithHdr(param string, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", APIProfiles, url.QueryEscape(param))
+ URI := fmt.Sprintf("%s?param=%s", APIProfiles, param)
Review comment:
Fixed.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't want to create a new method, since the bug fixes requires the existing method to be changed to accept int parameters, and I verified the existing methods call are updated as per new fix.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't think a new client is required, and I verified all the existing call to this method are passed.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
If we keep the existing methods as per your advice, if someone accidentally calls the existing method, it will throw an error, since that API is not anymore supported string.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
If we keep the existing methods as per your advice, if someone accidentally calls the existing method, it will throw an error, since that API is not anymore supporting string.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't want to create a new method, since the bug fixes require the API to accept int parameters(there is no change in method signature), If we keep the existing methods as per your advice, if someone accidentally calls the method, it will throw an error since that API is not anymore supporting string.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
If we keep the existing methods as per your advice, if someone accidentally calls the existing method, it will throw an error, since that API is not anymore supporting string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700010192
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
If we keep the existing methods as per your advice, if someone accidentally calls the existing method, it will throw an error, since that API is not anymore supported string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685726633
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
No, the `WHERE` clause will be created by the `BuildWhereAndOrderByAndPagination` function and you dont need to account for it, if you're passing in all the right query params into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r683660749
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -123,6 +124,10 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
+ //Check for interger value
Review comment:
Instead of checking for individual query parameters like this, we should just add them as part of the checkers on lines 116-120.
You should also add the checker for the `ParamQueryParam` in there, something like this:
```
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
```
Then, on line 126, if `ParamQueryParam` is present, you add the `Left Join...` subquery to the main select query.
This way, we can have one single place where we check for all the query params.
The API v2 and v3 tests currently fail because they are passing in a param name as an ID to the client methods. These will need to be fixed. Also, there is this section of code in the v4 tests, which can be uncommented after the fix:
``` // TODO: figure out what the 'Parameter' field of a Profile is and how
// passing it to this is supposed to work.
// resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
// if err != nil {
// t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
// }```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685702611
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
If I didn't change the `WHERE` as per above statement, the query looks incorrect.
`SELECT prof.description, prof.id, prof.last_updated, prof.name, prof.routing_disabled, prof.type, c.id as cdn, c.name as cdn_name FROM profile prof
LEFT JOIN cdn c ON prof.cdn = c.id
WHERE pp.parameter=:param
LEFT JOIN profile_parameter pp ON prof.id = pp.profile.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685702611
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
If I didn't change the where as per above statement, the query looks incorrect.
`SELECT prof.description, prof.id, prof.last_updated, prof.name, prof.routing_disabled, prof.type, c.id as cdn, c.name as cdn_name FROM profile prof
LEFT JOIN cdn c ON prof.cdn = c.id
WHERE pp.parameter=:param
LEFT JOIN profile_parameter pp ON prof.id = pp.profile.`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c closed pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c closed pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r708196913
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -518,11 +518,31 @@ func GetTestProfiles(t *testing.T) {
profileID := resp.Response[0].ID
// TODO: figure out what the 'Parameter' field of a Profile is and how
Review comment:
I haven't added this line. You only asked me keep this line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r711590621
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -178,6 +178,21 @@ func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, er
return data.Response, reqInf, err
}
+func (to *Session) GetProfileByParameterId(param int) ([]tc.Profile, ReqInf, error) {
Review comment:
Added godoc,.
##########
File path: traffic_ops/v3-client/profile.go
##########
@@ -139,6 +139,19 @@ func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, toclientli
return to.GetProfileByParameterWithHdr(param, nil)
}
+func (to *Session) GetProfileByParameterIdWithHdr(param int, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
Review comment:
Added godoc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r711586931
##########
File path: traffic_ops/testing/api/v2/profiles_test.go
##########
@@ -210,12 +210,29 @@ func GetTestProfiles(t *testing.T) {
t.Errorf("cannot GET Profile by name: %v - %v", err, resp)
}
profileID := resp[0].ID
-
- resp, _, err = TOSession.GetProfileByParameter(pr.Parameter)
- if err != nil {
- t.Errorf("cannot GET Profile by param: %v - %v", err, resp)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
+ t.Errorf("Cannot GET Parameter by name: %v - %v", err, resp)
Review comment:
removed the response in error statement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r694067290
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -90,8 +89,7 @@ func GetTestProfilesIMS(t *testing.T) {
if len(respParameter) > 0 {
parameterID := respParameter[0].ID
if parameterID > 0 {
- t.Errorf("Parameter ID %d", parameterID)
- resp, _, err := TOSession.GetProfileByParameterWithHdr(strconv.Itoa(parameterID), header)
+ resp, _, err := TOSession.GetProfileByParameterWithHdr(parameterID, nil)
Review comment:
You can't actually change the existing client signatures because that breaks the clients that are using them. I suggest creating a new method that takes in the parameterID, instead of the parameter name, and then using that method in the tests. You can leave the `GetProfileByParameterWithHdr` method as it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r708208504
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -518,11 +518,31 @@ func GetTestProfiles(t *testing.T) {
profileID := resp.Response[0].ID
// TODO: figure out what the 'Parameter' field of a Profile is and how
Review comment:
I removed this line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r666698170
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +150,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, errors.New("bad request found"), errors.New("profile read querying: " + err.Error()), http.StatusBadRequest, nil
Review comment:
I tried using ParseDBError function getting the same 500 Internal error. Instead, I updated the code to print the exact errors. It returns 400 Bad Request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700009615
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't want to create a new method, since the bug fixes require the API to accept int parameters(there is no change in method signature), If we keep the existing methods as per your advice, if someone accidentally calls the method, it will throw an error since that API is not anymore supporting string.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
If we keep the existing methods as per your advice, if someone accidentally calls the existing method, it will throw an error, since that API is not anymore supporting string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r685702611
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -113,19 +113,19 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
// Query Parameters to Database Query column mappings
// see the fields mapped in the SQL query
queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
- CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id"},
- NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
- IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ CDNQueryParam: dbhelpers.WhereColumnInfo{Column: "c.id", Checker: api.IsInt},
+ NameQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.name"},
+ IDQueryParam: dbhelpers.WhereColumnInfo{Column: "prof.id", Checker: api.IsInt},
+ ParamQueryParam: dbhelpers.WhereColumnInfo{Column: "pp.parameter", Checker: api.IsInt},
}
where, orderBy, pagination, queryValues, errs := dbhelpers.BuildWhereAndOrderByAndPagination(prof.APIInfo().Params, queryParamsToQueryCols)
// Narrow down if the query parameter is 'param'
-
// TODO add generic where clause to api.GenericRead
if paramValue, ok := prof.APIInfo().Params[ParamQueryParam]; ok {
queryValues["parameter_id"] = paramValue
if len(paramValue) > 0 {
- where += " LEFT JOIN profile_parameter pp ON prof.id = pp.profile where pp.parameter=:parameter_id"
+ where = " LEFT JOIN profile_parameter pp ON prof.id = pp.profile " + where + " AND pp.parameter=:parameter_id"
Review comment:
If I didn't change the `WHERE` as per above statement, the query looks incorrect.
`SELECT prof.description, prof.id, prof.last_updated, prof.name, prof.routing_disabled, prof.type, c.id as cdn, c.name as cdn_name FROM profile prof
LEFT JOIN cdn c ON prof.cdn = c.id
WHERE pp.parameter=:param
LEFT JOIN profile_parameter pp ON prof.id = pp.profile.`
getting internal server error. So, the above changes look good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r711588040
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -83,12 +83,31 @@ func GetTestProfilesIMS(t *testing.T) {
if reqInf.StatusCode != http.StatusNotModified {
t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
}
- _, reqInf, err = TOSession.GetProfileByParameterWithHdr(pr.Parameter, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- if reqInf.StatusCode != http.StatusNotModified {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+ if err != nil {
+ t.Errorf("Cannot GET Parameter by name: %v - %v", err, respParameter)
Review comment:
Removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#issuecomment-919191096
@dmohan001c It looks like the `go format` checks are failing. Once that is fixed, I can test this and approve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r707076296
##########
File path: traffic_ops/testing/api/v4/profiles_test.go
##########
@@ -517,12 +517,31 @@ func GetTestProfiles(t *testing.T) {
}
profileID := resp.Response[0].ID
- // TODO: figure out what the 'Parameter' field of a Profile is and how
Review comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r677364963
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -21,11 +21,12 @@ package profile
import (
"errors"
- "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
"net/http"
"strconv"
"time"
+ "github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/util/ims"
+
Review comment:
removed the extraneous space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r677629569
##########
File path: traffic_ops/traffic_ops_golang/profile/profiles.go
##########
@@ -149,7 +149,7 @@ func (prof *TOProfile) Read(h http.Header, useIMS bool) ([]interface{}, error, e
rows, err := prof.ReqInfo.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, nil, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
+ return nil, err, errors.New("profile read querying: " + err.Error()), http.StatusInternalServerError, nil
Review comment:
As I mentioned in the previous comment, this should just stay the same as it was before.
You could add another validation for the param being an int.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r700009615
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't want to create a new method, since the bug fixes requires the existing method to be changed to accept int parameters, and I verified the existing methods call are updated as per new fix.
##########
File path: traffic_ops/v2-client/profile.go
##########
@@ -164,7 +164,8 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, ReqInf, error) {
// GetProfileByParameter GETs a Profile by the Profile "param".
func (to *Session) GetProfileByParameter(param string) ([]tc.Profile, ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", API_PROFILES, url.QueryEscape(param))
+ paramId, _ := strconv.Atoi(url.QueryEscape(param))
Review comment:
I don't think a new client is required, and I verified all the existing call to this method are passed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r695002299
##########
File path: traffic_ops/v3-client/profile.go
##########
@@ -126,16 +126,16 @@ func (to *Session) GetProfileByName(name string) ([]tc.Profile, toclientlib.ReqI
return to.GetProfileByNameWithHdr(name, nil)
}
-func (to *Session) GetProfileByParameterWithHdr(param string, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
- URI := fmt.Sprintf("%s?param=%s", APIProfiles, url.QueryEscape(param))
+func (to *Session) GetProfileByParameterWithHdr(param int, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {
Review comment:
You can't change the signatures of these methods, if this code is not a part of a major release. There might be other client scripts using these methods, which will break if you change the signature. Instead, I would suggest doing something like you did in the `v2` client method above. That way, the method signature stays the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r708210680
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -83,12 +83,28 @@ func GetTestProfilesIMS(t *testing.T) {
if reqInf.StatusCode != http.StatusNotModified {
t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
}
- _, reqInf, err = TOSession.GetProfileByParameterWithHdr(pr.Parameter, header)
- if err != nil {
- t.Fatalf("Expected no error, but got %v", err.Error())
- }
- if reqInf.StatusCode != http.StatusNotModified {
- t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+ if len(pr.Parameters) > 0 {
+ parameter := pr.Parameters[0]
+ respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)
Review comment:
added error checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dmohan001c commented on a change in pull request #6003: Fix Internal server error in Get Profiles by invalid Params
Posted by GitBox <gi...@apache.org>.
dmohan001c commented on a change in pull request #6003:
URL: https://github.com/apache/trafficcontrol/pull/6003#discussion_r695410856
##########
File path: traffic_ops/testing/api/v3/profiles_test.go
##########
@@ -90,8 +89,7 @@ func GetTestProfilesIMS(t *testing.T) {
if len(respParameter) > 0 {
parameterID := respParameter[0].ID
if parameterID > 0 {
- t.Errorf("Parameter ID %d", parameterID)
- resp, _, err := TOSession.GetProfileByParameterWithHdr(strconv.Itoa(parameterID), header)
+ resp, _, err := TOSession.GetProfileByParameterWithHdr(parameterID, nil)
Review comment:
reverted the method signature
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org