addOptions fails to test URL if nil options are passed

[gmlewis]

While increasing code coverage in #1202, I discovered that if nil is passed to addOptions, then the rest of the URL tests are not performed.

This means that a garbage URL could be used without detection if nil is passed as the option.
This doesn't seem correct, but this will need more investigation to see why that check was added there and if it was fixing some other problem.

[AGMETEOR]

Hello @gmlewis , was this task done? Is it also a good first issue?

[gmlewis]

This issue is still open and has not been done yet.

No, unfortunately I would not consider this to be a good first issue because it will take some investigation and testing to figure out what the correct solution is.

[paladium]

Hello, I am happy to work on this issue. As I understand the effect of moving the parsing of the URL before checking for the nil value of options might have unpredictable effects?

[gmlewis]

I apologize for not documenting this issue better.

Here is the problem... In addOptions, the code says this right now:

// addOptions adds the parameters in opt as URL query parameters to s. opt
// must be a struct whose fields may contain "url" tags.
func addOptions(s string, opts interface{}) (string, error) {
	v := reflect.ValueOf(opts)
	if v.Kind() == reflect.Ptr && v.IsNil() {
		return s, nil
	}

	u, err := url.Parse(s)
	if err != nil {
		return s, err
	}
...
}

What I realized is that if opts is nil, then this function returns on line 238 without even attempting to parse the URL.

So if the URL is can not be parsed, addOptions is still going to report success, and I'm thinking that lines 241-244 should be moved up to the very top of the function.

In retrospect, this seems like a simple solution... however I filed this issue because this change will require a good amount of testing, and I would like someone to test out this change "in real life" as well, with the most endpoints as possible against real GitHub. I'm not expecting the change to cause problems, but we just need someone to carefully check.

Does that make sense, @paladium ?

Thank you for volunteering!

[paladium]

Yes, I understand the issue better now. I will be happy to test out this change. Is there any formal way I can document the process of real-life testing?

[paladium]

I can make a separate repo, include the changed library and write e2e tests for each API endpoint by consuming the new library?

[gmlewis]

Oh, uh, wow... No need for a formal documentation process or a separate repo... thank you, though!
If you simply want to keep your notes of your testing in this GitHub issue right here, that is fine with me.

[gmlewis]

My biggest concern is breaking the large number of users of this repo without running the endpoints through their paces. I can't imagine this change affecting anything negatively, but I just want (someone) to take time and make sure I'm not suggesting something stupid. 😄

Thanks again!

[paladium]

Yes sure, no problem.

[gmlewis]

Opening up issue for an experienced Go developer.
All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!

[WillAbides]

This can be closed per the discussion in #2896. The main premise of this issue is still true, addOptions does fail to do any URL checks when opts is nil. However this isn't a real problem because the Client.New*Request methods also error on invalid urls, and in the case that those methods aren't used, http.NewRequest also returns the same error for invalid urls.

[gmlewis]

Many thanks go to @WillAbides for doing a deep dive into this issue and proving that it is really a non-issue in the above comment.

Closing as a non-issue.