Fix Internal server error in Get Profiles by invalid Params by dmohan001c · Pull Request #6003 · apache/trafficcontrol

dmohan001c · 2021-07-08T08:54:33Z

What does this PR (Pull Request) do?

This PR fixes Unverified query parameter causes Internal Server Error #5510

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
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

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

traffic_ops/traffic_ops_golang/profile/profiles.go

srijeet0406 · 2021-07-23T18:53:03Z

traffic_ops/traffic_ops_golang/profile/profiles.go

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.

added another validation for the param and cdn to make sure it's an integer.

srijeet0406 · 2021-07-27T16:49:14Z

traffic_ops/traffic_ops_golang/profile/profiles.go

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.

srijeet0406 · 2021-08-05T17:40:41Z

traffic_ops/traffic_ops_golang/profile/profiles.go

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:

// 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) // }```

updated the DB Query to support params in GET profiles.

srijeet0406 · 2021-08-09T16:46:46Z

traffic_ops/traffic_ops_golang/profile/profiles.go

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.

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.

V2 AND V3 profiles tests are failing. I am taking a look on it.

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.

srijeet0406 · 2021-08-23T15:16:16Z

traffic_ops/testing/api/v3/profiles_test.go

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.

reverted the method signature

srijeet0406 · 2021-08-24T16:13:51Z

traffic_ops/v3-client/profile.go

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.

srijeet0406 · 2021-08-25T16:13:15Z

traffic_ops/v2-client/profile.go

 // 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))


We should probably check for the error here.

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 }

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.

Created a new client method

srijeet0406 · 2021-08-25T16:19:01Z

traffic_ops/v3-client/profile.go


 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)


Same comment as above here.

srijeet0406 · 2021-09-09T21:51:25Z

traffic_ops/testing/api/v4/profiles_test.go

 		}
 		profileID := resp.Response[0].ID

-		// TODO: figure out what the 'Parameter' field of a Profile is and how


You can probably remove these commented lines now.

srijeet0406 · 2021-09-13T15:43:10Z

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


Any reason to keep that first line in there still?

I removed this line.

srijeet0406 · 2021-09-13T15:44:27Z

traffic_ops/testing/api/v2/profiles_test.go

-			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)


We should probably check for the returned error here.

added error checks

srijeet0406 · 2021-09-13T15:44:37Z

traffic_ops/testing/api/v3/profiles_test.go

-			t.Fatalf("Expected 304 status code, got %v", reqInf.StatusCode)
+		if len(pr.Parameters) > 0 {
+			parameter := pr.Parameters[0]
+			respParameter, _, _ := TOSession.GetParameterByName(*parameter.Name)


We should probably check for the returned error here.

added error checks.

srijeet0406 · 2021-09-13T15:44:44Z

traffic_ops/testing/api/v3/profiles_test.go

-			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)


We should probably check for the returned error here.

added error checks

srijeet0406 · 2021-09-14T14:12:01Z

@dmohan001c It looks like the go format checks are failing. Once that is fixed, I can test this and approve.

srijeet0406 · 2021-09-16T20:34:37Z

traffic_ops/testing/api/v2/profiles_test.go

+			parameter := pr.Parameters[0]
+			respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+			if err != nil {
+				t.Errorf("Cannot GET Parameter by name: %v - %v", err, resp)


This resp is the wrong response here. Also, I don't think you need to print the response here, just the error is fine.

removed the response in error statement

srijeet0406 · 2021-09-16T20:34:52Z

traffic_ops/testing/api/v2/profiles_test.go

+				if parameterID > 0 {
+					resp, _, err = TOSession.GetProfileByParameterId(parameterID)
+					if err != nil {
+						t.Errorf("cannot GET Profile by param: %v - %v", err, resp)


Same comment here, no need to print the response.

removed the response from print statement

srijeet0406 · 2021-09-16T20:35:05Z

traffic_ops/testing/api/v3/profiles_test.go

+			parameter := pr.Parameters[0]
+			respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+			if err != nil {
+				t.Errorf("Cannot GET Parameter by name: %v - %v", err, respParameter)


srijeet0406 · 2021-09-16T20:35:19Z

traffic_ops/testing/api/v3/profiles_test.go

+		if len(pr.Parameters) > 0 {
+			parameter := pr.Parameters[0]
+			respParameter, _, err := TOSession.GetParameterByName(*parameter.Name)
+			if err != nil {


srijeet0406 · 2021-09-16T20:35:47Z

traffic_ops/testing/api/v4/profiles_test.go

+		if len(pr.Parameters) > 0 {
+			parameter := pr.Parameters[0]
+			opts.QueryParameters.Set("name", *parameter.Name)
+			respParameter, _, _ := TOSession.GetParameters(opts)


You should be checking the error here, as you are doing in the v2 and v3 tests.

added error checks

srijeet0406 · 2021-09-16T20:36:16Z

traffic_ops/v3-client/profile.go

 	return to.GetProfileByParameterWithHdr(param, nil)
 }

+func (to *Session) GetProfileByParameterIdWithHdr(param int, header http.Header) ([]tc.Profile, toclientlib.ReqInf, error) {


Godoc is missing.

Added godoc.

srijeet0406 · 2021-09-16T20:36:25Z

traffic_ops/v2-client/profile.go

 	return data.Response, reqInf, err
 }

+func (to *Session) GetProfileByParameterId(param int) ([]tc.Profile, ReqInf, error) {


Godoc is missing.

Added godoc,.

srijeet0406

Code changes look good.
All tests pass.
Manual testing looks good as well.
LGTM!

dmohan001c changed the title ~~Fix~~ Fix Internal server error in Get Profiles by invalid Params Jul 8, 2021

ocket8888 reviewed Jul 8, 2021

View reviewed changes

traffic_ops/traffic_ops_golang/profile/profiles.go Outdated Show resolved Hide resolved

traffic_ops/traffic_ops_golang/profile/profiles.go Outdated Show resolved Hide resolved

ocket8888 added bug something isn't working as intended low impact affects only a small portion of a CDN, and cannot itself break one Traffic Ops related to Traffic Ops labels Jul 8, 2021

srijeet0406 suggested changes Jul 23, 2021

View reviewed changes

dmohan001c force-pushed the getprofiles branch from 440722c to 516a7c9 Compare July 27, 2021 11:40

srijeet0406 suggested changes Jul 27, 2021

View reviewed changes

dmohan001c marked this pull request as draft August 4, 2021 13:48

dmohan001c marked this pull request as ready for review August 5, 2021 03:19

dmohan001c force-pushed the getprofiles branch from 69cf6fc to 5f75d2a Compare August 5, 2021 10:38

srijeet0406 suggested changes Aug 5, 2021

View reviewed changes

srijeet0406 suggested changes Aug 9, 2021

View reviewed changes

dmohan001c marked this pull request as draft August 12, 2021 14:04

dmohan001c force-pushed the getprofiles branch from e95ffe5 to 1b27620 Compare August 12, 2021 16:48

dmohan001c marked this pull request as ready for review August 12, 2021 17:09

dmohan001c marked this pull request as draft August 13, 2021 07:38

dmohan001c marked this pull request as ready for review August 23, 2021 11:28

srijeet0406 suggested changes Aug 23, 2021

View reviewed changes

dmohan001c force-pushed the getprofiles branch from 8a5a9bd to 6aeff07 Compare August 24, 2021 07:50

srijeet0406 suggested changes Aug 24, 2021

View reviewed changes

dmohan001c closed this Aug 25, 2021

dmohan001c reopened this Aug 25, 2021

dmohan001c added 7 commits August 25, 2021 12:38

merged changes

f11e050

updated error message in get profiles

4ce1761

updated error message to print the exact errors

e3ac620

changed the error status code for get profiles by invalid cdn name

96b8c67

Added validation to check integer value

eaf936c

updated DB query to support params value in getprofiles by param

771fa42

added tests for get profile by param in all versions

a3c5186

dmohan001c added 3 commits August 25, 2021 12:38

Fixed merge commits

44b583b

updated read profiles tests to support integer param values

379d729

reverted back the method signature of GetProfileByParameterWithHdr

391d26a

dmohan001c force-pushed the getprofiles branch from b533e99 to 391d26a Compare August 25, 2021 07:08

srijeet0406 suggested changes Aug 25, 2021

View reviewed changes

dmohan001c added 2 commits September 1, 2021 14:03

updated the get profiles validation

97baf2c

added new client methods for getprofilebyparameterId

dc8ebab

srijeet0406 reviewed Sep 9, 2021

View reviewed changes

added back the already available comment

f61ad9b

dmohan001c closed this Sep 13, 2021

dmohan001c reopened this Sep 13, 2021

srijeet0406 reviewed Sep 13, 2021

View reviewed changes

added error checks

98be188

formatted the code

4503e7e

srijeet0406 reviewed Sep 16, 2021

View reviewed changes

removed unnecessary review comments

8656167

srijeet0406 approved these changes Sep 20, 2021

View reviewed changes

ocket8888 approved these changes Sep 20, 2021

View reviewed changes

ocket8888 merged commit bf0df00 into apache:master Sep 20, 2021

		@@ -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

Conversation

dmohan001c commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmohan001c Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmohan001c Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srijeet0406 commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dmohan001c commented Jul 8, 2021 •

edited

Loading

dmohan001c Aug 10, 2021 •

edited

Loading

dmohan001c Sep 1, 2021 •

edited

Loading