Singleton Pattern Considered Harmful

[ryneeverett]

Apologies for ignoring the issue templates but this would fall more closely into a [question] or [design] category.

To me, the primary appeal of declarative clients can be summed up as encapsulation of the logic of interacting with endpoints and improved ergonomics in terms of readability and making it easy to implement correctly while making it difficult to make errors.

My thesis is that when apiron is pushed beyond simple use cases, these twin virtues are often threatened by apiron's use of the singleton pattern.

If this is correct, I would further propose that this limitation is unnecessary, could be addressed with fairly minimal changes, and could likely be eliminated without breaking backwards compatibility.

However, first I believe I should try to establish that there is room for improvement by going through a few use cases. All code examples I will be discussing can be found on my repo https://github.com/ryneeverett/quasi-apiron, which contains a series of example modules implementing the same functionality using both apiron and quasi_apiron. The quasi_apiron.py module does not attempt to replicate all apiron functionality but implements the bare minimum to illustrate what a non-singleton design might look like. All the modules in that repo should run successfully with the exception of the *_example_multiple_caller.py modules which reference fictional infrastructure in order to illustrate a point.

Singleton Encourages Global Variables

I believe a common use case would be to pass arguments like auth, session, and headers to any and all endpoints of a given service. (Side note: I believe most of the call arguments are undocumented at the moment.) In apiron_example_caller_endpoint_args.py we have an example of how one might do this:

import requests
from apiron import Service, JsonEndpoint


class GitHub(Service):
    domain = 'https://api.github.com'
    user = JsonEndpoint(path='/users/{username}')
    repo = JsonEndpoint(path='/repos/{org}/{repo}')


SESSION = requests.Session()
response = GitHub.user(username='defunkt', session=SESSION)
print(response)

You could imagine us reusing this SESSION variable in subsequent requests and the global variable wouldn't be a big deal. However, it becomes cumbersome if you imagine this in the context of a program that, say, defines the service in one module and calls it from several other modules. These modules would have to also import the SESSION variable and now we're dragging this throughout our program when it really just wants to be owned by this one class.

In quasi_example_caller_endpoint_args.py we have an example of what this api could look like with an instantiated Service:

import requests
from quasi_apiron import Service, JsonEndpoint


class GitHub(Service):
    domain = 'https://api.github.com'
    user = JsonEndpoint(path='/users/{username}')
    repo = JsonEndpoint(path='/repos/{org}/{repo}')


service = GitHub(session=requests.Session())
response = service.user(username='defunkt')
print(response)

Alternatives: We could eliminate the need for global variables by allowing callers to overwrite a class attribute to pass in default arguments so, for example, you could write GitHub.endpoint_kwargs = {'session': SESSION}.

Endpoint Customization is Unintuitive and Cumbersome

The most obvious way to write an endpoint that wraps the default functionality would be to subclass Endpoint and write a __call__ method that calls super(), but not so fast!

class Endpoint:
    ...
    def __call__(self):
        raise TypeError("Endpoints are only callable in conjunction with a Service class.")

You really have to dig into the code to figure out what's going on before you can do something like we find in apiron_example_pagination.py:

import requests
from apiron import Service, Endpoint


class PaginatedEndpoint(Endpoint):

    def __get__(self, *args):
        def paging_caller(*fargs, **kwargs):
            # Use one session for all pages.
            kwargs['session'] = kwargs.get('session', requests.Session())

            response = super(type(self), self).__get__(*args)(*fargs, **kwargs)
            yield from response.json()

            method = kwargs.get('method', 'GET')

            while 'next' in response.links:
                url = response.links['next']['url']
                response = kwargs['session'].request(method, url)
                yield from response.json()

        return paging_caller

    def format_response(self, response):
        return response


class GitHub(Service):
    domain = 'https://api.github.com'
    issues = PaginatedEndpoint(
        path='/repos/{username}/{repo}/issues',
        default_params={'per_page': '5', 'state': 'all'})
    pulls = PaginatedEndpoint(
        path='/repos/{username}/{repo}/pulls',
        default_params={'per_page': '20', 'state': 'all'})


response = GitHub.issues(username='ithaka', repo='apiron')
for issue in response:
    print(issue['title'])

Contrast this with quasi_example_pagination.py, in which we can use __call__ and can just do what we want without wrapping it in a function:

import requests
from quasi_apiron import Service, Endpoint


class PaginatedEndpoint(Endpoint):
    def __call__(self, *args, **kwargs):
        # Use one session for all pages.
        kwargs['session'] = kwargs.get('session', requests.Session())

        response = super().__call__(*args, **kwargs)
        yield from response.json()

        method = kwargs.get('method', 'GET')

        while 'next' in response.links:
            url = response.links['next']['url']
            response = kwargs['session'].request(method, url)
            yield from response.json()


class GitHub(Service):
    domain = 'https://api.github.com'
    issues = PaginatedEndpoint(
        path='/repos/{username}/{repo}/issues',
        params={'per_page': '5', 'state': 'all'})
    pulls = PaginatedEndpoint(
        path='/repos/{username}/{repo}/pulls',
        params={'per_page': '20', 'state': 'all'})


service = GitHub()
response = service.issues(username='ithaka', repo='apiron')
for issue in response:
    print(issue['title'])

Alternative: Maybe we could simulate this behavior by having the caller call the __call__ method of subclasses?

Multiple Callers must Reset Class State

This is the only case in which the examples don't actually run and just serve as an illustration. It may not be the strongest point, but I believe it's the only one with no viable alternatives.

I would suggest that a valid use case would be using a single declarative client to access multiple services and have tried to illustrate what this might look like in a realistic scenario in apiron_example_multiple_callers.py:

import threading
from apiron import Service, JsonEndpoint


class GitHub(Service):
    repos = JsonEndpoint(path='/repos/{org}')
    issues = JsonEndpoint(path='/repos/{org}/{repo}/issues')
    pulls = JsonEndpoint(path='/repos/{org}/{repo}/pulls')


class GitHubOrg:
    def __init__(self, org, client, domain, auth):
        self.org = org
        self.client = client
        self.domain = domain
        self.auth = auth

    def repos(self):
        with threading.Lock:
            self.client.domain = self.domain
            return self.client.repos(org=self.org, auth=self.auth)

    def issues(self, repo):
        with threading.Lock:
            self.client.domain = self.domain
            return self.client.issues(org=self.org, repo=repo, auth=self.auth)

    def pulls(self, repo):
        with threading.Lock:
            self.client.domain = self.domain
            return self.client.pulls(org=self.org, repo=repo, auth=self.auth)


foo_repo = GitHubOrg('foo', GitHub, 'https://foo.com/api/v3', 'foo_auth_key')
bar_repo = GitHubOrg('bar', GitHub, 'https://bar.com/api/v3', 'bar_auth_key')

Because the service is a singleton, the caller must reset the domain before every request. And because our user may want to support multithreading, they need to take extra measures to make sure another thread doesn't change the domain before the request. With instantiated services, the author of quasi_example_multiple_callers.py doesn't need to worry about such things:

from quasi_apiron import Service, JsonEndpoint


class GitHub(Service):
    repos = JsonEndpoint(path='/repos/{org}')
    issues = JsonEndpoint(path='/repos/{org}/{repo}/issues')
    pulls = JsonEndpoint(path='/repos/{org}/{repo}/pulls')


class GitHubOrg:
    def __init__(self, org, client, domain, auth):
        self.org = org
        self.client = client

    def repos(self):
        return self.client.repos(org=self.org)

    def issues(self, repo):
        return self.client.issues(org=self.org, repo=repo)

    def pulls(self, repo):
        return self.client.pulls(org=self.org, repo=repo)


foo_repo = GitHubOrg(
    'foo', GitHub(domain='https://foo.com/api/v3', auth='foo_auth_key'))
bar_repo = GitHubOrg(
    'bar', GitHub(domain='https://bar.com/api/v3', auth='bar_auth_key'))

Summary

Thanks for your consideration. Comments, critiques, and corrections welcome.

[daneah]

Thanks for this detailed outline @ryneeverett! Will need some time to reflect on the suggestion and its nuances, as well as the impact on the team here at ITHAKA who is likely the most extensive user base.