Dynamic building of URLs

Question

I'm teaching myself Python and wanted to share this problem as my current solution feels inelegant but I'm not sure of the best way to refactor. If this sort of question isn't appropriate, leave a comment and I'll remove it.

I'm writing a generic library to handle web API requests. The aim is to have a generic core that can be fed with URL templates and parameters - so each new API is just a wrapper rather than duplicating core logic like paging, parsing, etc.

Trouble is, each URL has a completely different structure:

• Facebook feeds have an ID variable in the URL endpoint - /{id}/feed/
• Facebook insights require per-request parameters as well eg /{id}/insights/page_fans?since={timestamp}&until={timestamp}
• Instagram user search requires the ID to be a query parameter - /users/search?q={}

Then all URLs need additional query parameters like access_token which is static.

I had started with a simple string.format() to replace the variables, but this doesn't work with dynamic parameters.

New model is to build the URL from a mixture of endpoint, static parameters and dynamic parameters. I'm then using itertools.product to work out the permutations and then encode the URLs.

Current thinking is to create a set of methods for each common problem (date ranges, etc) and just call each one as required during the setup wrapper.

Here's an example of a working wrapper - can you see any more Pythonic ways to handle this situation?

def insights_impressions_unique(ids, access_token, date_start=None,
                                date_end=None, period="day"):
    if period not in ["day", "week", "days_28"]:
        raise ValueError("Period must be 'day','week' or 'days_28'")

    # Build basic URLs
    _api_domain = "https://graph.facebook.com"
    _api_endpoint = "/v2.2/{}/insights/page_impressions_unique"
    url_template = "{}{}".format(_api_domain, _api_endpoint)

    _formatted_urls = [url_template.format(_id) for _id in ids]

    # Move on to paramaters
    _base_params = [{"access_token": access_token,
                    "period": period}, ]

    # Build date periods
    _utcnow = datetime.datetime.utcnow().replace(tzinfo=pytz.utc)
    if date_end is None:
        date_end = _utcnow
    if date_start is None:
        date_start = _utcnow - datetime.datetime.timedelta(days=-7)
    _date_windows = utils.api_call_time_windows(date_start, date_end, freq=7)
    _date_params = [{'since': x[0], 'until': x[1]} for x in _date_windows]

    _combined_params = product(_base_params, _date_params)

    list_of_params = list()
    for i in _combined_params:
        params = {}
        for d in i:
            for k, v in d.iteritems():
                params[k] = v
        list_of_params.append(urllib.urlencode(params))

    _components = product(_formatted_urls, list_of_params)
    _finalised_urls = ["{0}?{1}".format(_u, _p) for _u, _p in _components]

    manager = RequestManager(urls=_finalised_urls, pagination=False)
    return manager

Full code on this dev branch on GitHub.

Answer 1

By and large, I think this approach makes sense and has a fine grasp of Python idiom. Nonetheless, if I'm going to go straight CR on this (maybe this should be at https://codereview.stackexchange.com/), I think there is a short list of things that're a little funky.

Local variables don't need to be underscore prefixed -- since they're closed over by the function and can't be published, there's no need to mark them as private. The underscore prefix is normally only used for module-level or class-level names, or, occasionally, for a name that would otherwise shadow a built-in (for example, _id or _list -- but really, you can just rename stuff in that situation).

Your construction of list_of_params (which should be declared with a literal [], btw) adds a couple of extra lines when it should just use dict.update.

list_of_params = []
for combined_param_product in combined_param_products:
    params = {}
    for param_dict in combined_param_product:
        params.update(param_dict) 
    list_of_params.add(urllib.urlencode(params))

pytz.UTC.localize is preferable to datetime.replace, although it makes no difference in this situation.

This is totally inconsequential, but it bothers me when I see people doing a if value in CONSTANT_LIST. I just see the O(n) time over and over in my head, and it could just be O(1) if two characters were changed. Of no real importance, but could probably be {"day", "week", "days_28"}.

Oh, and don't raise ValueError -- raise a subclass of it. Your client code will love you forever.

Lastly, this isn't Python-specific, but it seems like there's some factoring out to do. Pretty much any one of these wrappers is going to construct a variable number of param option lists and some urls. I propose:

def _finalize_urls(base_urls, *param_options_lists):
    """
    :type base_urls: list[str]
    :type param_options_lists: tuple[list[dict[str]]]
    :rtype: list[str]
    """
    pass

This function would contain pretty much all of your code after your declaration of date_params, and be invoked by wrappers like so:

urls = finalize_urls(base_urls, base_params, date_params)

Actually, I'd probably even make it a static constructor of RequestManager

return RequestManager.for_urls_and_params(base_urls, base_params, date_params)

Anyway, by and large totally solid.