Implementation of Categories Http fetcher

[VeronikaSolovei9]

-Added code to fetch data using http
-Added previously loaded categories to run http request just at the first time

[jmaynardxandr]

We should move the file_fetcher.Category type into a common module so that http_fetcher doesn't have to depend on file_fetcher.

[VeronikaSolovei9]

Agree. Done

[jmaynardxandr]

It mostly looks fine to me, just a couple of minor issues I commented on that should be addressed before we merge. Thanks.

[jmaynardxandr]

Probably just removing the ? isn't going to work if the fetcher.Endpoint contains query params in the URL, we should just have this function throw an appropriate error if the endpoint has a ? here (instead of trying to handle that case).

[VeronikaSolovei9]

I made it because in NewFetcher function contains this code:

func NewFetcher(client *http.Client, endpoint string) *HttpFetcher {
	// Do some work up-front to figure out if the (configurable) endpoint has a query string or not.
	// When we build requests, we'll either want to add `?request-ids=...&imp-ids=...` _or_
	// `&request-ids=...&imp-ids=...`, depending.
	urlPrefix := endpoint
	if strings.Contains(endpoint, "?") {
		urlPrefix = urlPrefix + "&"
	} else {
		urlPrefix = urlPrefix + "?"
	}

	...

	return &HttpFetcher{
		client:   client,
		Endpoint: urlPrefix,
	}
} 

because of this there is always ? at the end of url.
In my understanding in case of Categories fetcher we expect to have url without parameters, so I have to handle this explicitly.
Do you think I need to refactor existing functionality?

[jmaynardxandr]

Oh ok I see why that's there then. I guess the difference is category fetcher is expecting it's variable parts to be in the URL path where as other fetchers are passing them in URL query params. Maybe this is fine then, but please add a comment to the effect that you are stripping away the question mark that gets added in NewFetcher code. Also we're assuming endpoints used for category mappings aren't going to contain query params, which maybe is ok since they'll probably just be statically hosted files.

[VeronikaSolovei9]

Comment added.

[guscarreon]

Looks good to me

[hhhjort]

This means we could have something like freewheel_freewheel as the dataName below. Is this correct, or should it resolve to just freewheel?

[VeronikaSolovei9]

You're right, fixed!

[mansinahar]

Overall the PR LGTM. I just have one small question though: Are we sure that the data will always have a value for iabCategory? Should we rather just do a small check before we return? Both here and towards the end of the function.

[VeronikaSolovei9]

@mansinahar yes, it's good to have this check. Thank you!

[mansinahar]

Thanks @Kalypsonika !

[josephveach]

A few comments about this code:

1. NewFetcher() is capable of returning "&" as the last character as well as "?" (in which case there will be a "?" somewhere earlier in the string). I'm guessing from the above comment that this will never happen for categories. Is that correct? If so, might it be better (i.e., probably more efficient and more readable) to use strings.TrimSuffix() to simply remove the "?" from the end of the string? If "?" is not guaranteed to be at the end of the string, then shouldn't everything from the "?" in the line forward be removed? It might be safest to allow for the possibility of "?" not being at the end of the line (in case of future changes) and use strings.IndexByte() to find the "?" and chop off the end of the line from there; this works regardless of the location of "?".

2. The url variable is assigned twice when publisherId has a value, both times making the same call to strings.Replace(). The data name variable is also assigned a value twice. It would be a little more efficient to simply declare url and dataName as strings, then use an if-statement to assign each variable once.

3. Taking efficiency considerations to an extreme, if it's likely that fetcher.Categories[dataName] is likely to be set more often than not (I don't know how one would know this), it might be even more efficient to put off setting the value of url until it is needed, in case we never get there. Drawback to this is it's less efficient (two if-statements needed to check publisherId) when fetcher.Categories[dataName] is unset.

[VeronikaSolovei9]

Hi Joe, @josephveach thank you for your comment!

1. Please see above discussion with Jason about this. There is a code snipped with logic of adding "?" at the end of URL: Implementation of Categories Http fetcher #882 (comment)
   We don't expect to have URL with parameters for http fetcher, here is how url builder looks like:

url = fmt.Sprintf("%s/%s/%s.json", strings.Replace(fetcher.Endpoint, "?", "", -1), primaryAdServer, publisherId)

2. Refactored

3. URL builder is just a simple string concatenation. We can separate it and have 2 publisher id checks. I would leave it as it is now.

Please let me know what do you think.

[josephveach]

Hi Veronika, Let me clarify my thoughts regarding (1):
Looking purely at the NewFetcher() code, it seems that the returned value of Endpoint can be in two forms:
(a) if there is no query string in the original "endpoint" argument to NewFetcher() (i.e., endpoint was of form someURL), then Endpoint is simply
someURL?
which works fine with the given code, but
(b) if "endpoint" contained a query string, i.e., was of form someURL?someQuery, then Endpoint would be
someURL?someQuery&
which I believe does not work properly with the given code.

The comment simply states "we don't expect to have any parameters" in the category case, which does not seem absolute or clear to me (e.g., does "don't expect" mean it's impossible? does "parameters" equate to what would be in the query?). Furthermore, as I've already indicated, the use of strings.Replace() probably doesn't achieve the desired effect on the second form (b) above of the returned value of Endpoint.

IMHO, it would be more clear to someone who is not terribly familiar with the underlying code that gets us here if (1) this code used strings.TrimSuffix() instead of strings.Replace(), which would emphasize the '?' occurs only as the last character and, possibly, (2) if the comment were a little more strong, i.e., something like "in case of categories, there is never a query string in the endpoint so it always ends with '?'", which is more certain than "we don't expect". Actually, looking at previous discussion, I prefer Jason's explanation above: "category fetcher is expecting its variable parts to be in the URL path whereas other fetchers are passing them in URL query params", though maybe "expecting" could be replaced by something stronger in a code comment.
I also expect strings.TrimSuffix() to run faster than strings.Replace() because it doesn't need to examine the entire string, though I haven't confirmed that.

BTW, I see that NewFetcher() also does an unnecessary second assignment to urlPrefix. The first portion of the function could be changed to simply:

   var urlPrefix string
   if strings.Contains(endpoint, "?") {
	urlPrefix = endpoint + "&"
} else {
	urlPrefix = endpoint + "?"
}

[VeronikaSolovei9]

Hi Joe, strings.Replace is changed to strings.TrimSuffix.
Refactoring existing code in NewFetcher is beyond the scope of this PR and will require to refactor buildRequest function as well.
Also there are no specific requirements to categories URL, so maybe url builder will be changed in future.

[josephveach]

My original comment suggested to chop off the end of the line starting at the "?" if there was a chance of the "?" ever not being the last character. You seem to be saying that could be a possibility in the future, in which case this solution could lead to a problem at that time. In any case, I've made my suggestion and I have no other objections so I'll give my approval of the PR.

[VeronikaSolovei9]

Thank you Joe!
With current implementation we just want to delete last "?". If it's not there - url is returned unchanged. Please see this documentation: https://golang.org/pkg/strings/#TrimSuffix

[josephveach]

My original comment suggested to chop off the end of the line starting at the "?" if there was a chance of the "?" ever not being the last character. You seem to be saying that could be a possibility in the future, in which case this solution could lead to a problem at that time. In any case, I've made my suggestion and I have no other objections so I'll give my approval of the PR.