Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added catch for missing POST body when posted to profileparameters #7194

Merged

Conversation

kdamichie
Copy link
Contributor

@kdamichie kdamichie commented Nov 14, 2022

This PR closes #4428


Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops

What is the best way to verify this PR?

Make sure all of the tests pass

For manual testing, try to send a POST to the profileparameters with an empty body. The result should be a 400 error.

If this is a bugfix, which Traffic Control versions contained the bug?

-master

PR submission checklist

@kdamichie kdamichie marked this pull request as ready for review November 14, 2022 22:25
@srijeet0406 srijeet0406 self-requested a review November 15, 2022 20:35
Copy link
Contributor Author

@kdamichie kdamichie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed changed traffic-ops-test.conf. Accidentally commited

@ocket8888 ocket8888 added bug something isn't working as intended Traffic Ops related to Traffic Ops low impact affects only a small portion of a CDN, and cannot itself break one labels Nov 16, 2022
@ocket8888 ocket8888 self-assigned this Nov 16, 2022
Copy link
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cases here that claim to have empty bodies actually don't. Note that you can't fix that just by setting RequestBody to nil - nil marshals to null, not an empty request body. Sending an empty request may not be very easy to do using the same format as all the other tests. I don't think there's anything you can put in for RequestBody that will do what you want.

ClientSession: TOSession,
RequestBody: []tc.ProfileParameter{{}},
Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually test the behavior being fixed in this PR. This a list of length 1 (not 0!) whose only element has the default/"zero" values for all of its properties. The result when Go marshals that is, accordingly, an array with one object element.

ClientSession: TOSession,
RequestBody: map[string]interface{}{},
Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually test the behavior being fixed in this PR. Marshalling an empty map appropriately produces an empty object - not an empty request body!

ClientSession: TOSession,
RequestBody: map[string]interface{}{},
Expectations: utils.CkRequest(utils.HasError(), utils.HasStatus(http.StatusBadRequest)),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@ocket8888
Copy link
Contributor

ocket8888 commented Nov 16, 2022

I don't think there's anything you can put in for RequestBody that will do what you want.

Actually, I take that back. It's actually fairly easy to create a type that marshals to an empty request body.

Example
package main

import (
	"encoding/json"
	"fmt"
)

type emptyJSONTester struct{}

func (emptyJSONTester) MarshalJSON() ([]byte, error) {
	return []byte{}, nil
}

func main() {
	bts, _ := json.Marshal(emptyJSONTester{})
	fmt.Printf("'%s'\n", string(bts))
	// Output: ''
}

Copy link
Contributor

@srijeet0406 srijeet0406 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Manual and automated tests pass. Good work! :)

@ocket8888 ocket8888 merged commit e82501c into apache:master Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

profileparameters POST request with no request body returns Internal Server Error
3 participants