4
\$\begingroup\$

I'm coming from PHP and recently started developing in Python. As a part of my self-study course, I developed a small library for access to Visa Developer Program APIs and published it on Github. It is bundled with demo Django app. Here I'm sharing a module that implements an abstract connection to VDP.

I'd appreciate hearing your comments regarding the source code. Personally, I have a feeling that modules hierarchy could have been organized differently and maybe I'm overusing OOP approaches.

visa/request.py

import jsonpickle
import os
import requests

try:
    import urllib.parse as urllib_parse
except ImportError:
    # Python 2.7 compatibility
    from urllib import parse as urllib_parse

try:
    import configparser as parser
except ImportError:
    import ConfigParser as parser

from visa.exceptions import (VisaAccessDeniedError, VisaDuplicateTransactionError, VisaGeneralError,
                         VisaMessageValidationError, VisaNotFoundError, VisaTimeoutError, VisaUnauthenticatedError)


class VisaRequest:
    """
    Constructs a request to Visa API using provided transaction, resource, method and http verb.
    Request can be submitted with request() method, which returns a dictionary in following format:
    {'code': http_response_code, 'message': json-encoded content}
    This class is not assumed to be instantiated on its own (consider it abstract). Instead, every VISA API
    implementation must provide its own request class, inheriting from VisaRequest and providing a corresponding VISA
    API resource name as a `resource` argument value.
    """
    config = parser.ConfigParser()
    config_file = os.path.join(os.path.dirname(__file__), 'configuration.ini')
    config.read(config_file)

    def __init__(self, resource, api, method, http_verb):
        """
        :param resource: VISA API resource name
        :type resource: str
        :param api: VISA API api name
        :type api: str
        :param method: VISA API endpoint method
        :type method: str
        :param http_verb: VISA API HTTP verb
        :type http_verb: str
        """
        url = self.config.get('VISA', 'url')
        version = self.config.get('VISA', 'version')
        self.user_id = self.config.get('VISA', 'username')
        self.password = self.config.get('VISA', 'password')
        self.cert = os.path.join(os.path.dirname(__file__), self.config.get('VISA', 'cert'))
        self.key = os.path.join(os.path.dirname(__file__), self.config.get('VISA', 'key'))

        self.api_endpoint = url + "/" + resource + "/" + api + "/" + version + "/" + method
        self.http_verb = http_verb

        # FIXME: Hardcoded request/response in json format
        self.headers = {
            'Content-Type': 'application/json',
            'Accept': 'application/json'
        }

        self.result = None

    def send(self, transaction=None):
        """
        Sends a Transaction object to VISA using self.api_endpoint field and corresponding http verb.
        :param transaction: instance of VisaTransaction
        :type transaction: VisaTransaction
        :return: dict
        """
        transaction = self.__to_vd(transaction)

        result = requests.request(
            self.http_verb,
            self.api_endpoint,
            data=transaction,
            cert=(self.cert, self.key),
            auth=(self.user_id, self.password),
            headers=self.headers
        )

        return self.__response({'code': result.status_code, 'message': result.json()})

    @staticmethod
    def __response(result):
        """
        Processes a response from Visa Direct API.
        Depending on http code in response, either returns a result or raises corresponding app-level exception.
        :param result: Response from VISA
        :type result: dict
        :return result: Resulting dictionary
        :raises VisaTimeoutError, VisaDuplicateTransactionError, VisaMessageValidationError, VisaUnauthenticatedError,
                VisaAccessDeniedError, VisaNotFoundError, VisaGeneralError
        """
        code = result['code']

        if code == 200:
            return result
        elif code == 202:
            raise VisaTimeoutError(result=result)
        elif code == 303:
            raise VisaDuplicateTransactionError(result=result)
        elif code == 400:
            raise VisaMessageValidationError(result=result)
        elif code == 401:
            result = {'message': {'errorMessage': str(result['message'])}}
            raise VisaUnauthenticatedError(result=result)
        elif code == 403:
            raise VisaAccessDeniedError(result=result)
        elif code == 404:
            raise VisaNotFoundError(result=result)
        else:
            raise VisaGeneralError(result=result)

    @staticmethod
    def __to_vd(transaction=None):
        """
        Serializes VisaTransaction object to json string.
        :param transaction: instance of VisaTransaction.
        :return: json
        """
        if not transaction:
            return
        else:
            return jsonpickle.encode(transaction, unpicklable=False)
\$\endgroup\$
4
\$\begingroup\$

The code overall looks clean, there will always be debates on writing classes or not (both approaches have great reasoning behind them, of course):

Some things you can improve:

  • the status code handling. Build a mapping between status codes and exception types - this would help to make the code more modular, configurable and concise, something like:

    ERROR_CODES = {
        202: VisaTimeoutError,
        303: VisaDuplicateTransactionError,
        400: VisaMessageValidationError,
        401: VisaUnauthenticatedError,
        403: VisaAccessDeniedError,
        404: VisaNotFoundError
    }
    if code == 200:
        return result
    else:
        raise ERROR_CODES.get(code, VisaGeneralError)(result=result)
    
  • you can avoid some nestedness in the __to_vd() method:

    if not transaction:
        return
    
    return jsonpickle.encode(transaction, unpicklable=False)
    
  • since you are repeatedly getting the current directory name and, it is not going to change, do it once beforehand and reuse:

    CUR_DIR_NAME = os.path.dirname(__file__)
    
  • I would also initialize a session via requests.Session() in the class constructor, define the session.headers, session.auth and other required parameters. Then, when you will make API requests using self.session, all of the parameters you've initially set will be automatically applied. This will also have a positive performance impact:

    ..if you're making several requests to the same host, the underlying TCP connection will be reused, which can result in a significant performance increase

  • follow the PEP8 import organization guidelines

\$\endgroup\$
  • \$\begingroup\$ urljoin signature in 3.6 assumes 3 arguments: def urljoin(base, url, allow_fragments=True), so I guess you have to do concat anyway? \$\endgroup\$ – paulus Feb 16 '17 at 17:48
  • \$\begingroup\$ @paulus ah, you are right, my bad. It's possible to do it "recursively" with urljoin(), but would overcomplicate things I guess, will remove the advice. Thanks. \$\endgroup\$ – alecxe Feb 16 '17 at 17:53
  • \$\begingroup\$ for the record - i don't like concat either. I'm guessing something like '/'.join([url, api, resource, method]) would be a nice thing. But I've googled it around, apparently there's no pythonic solution for proper url joining, since everyone is doing it differently. \$\endgroup\$ – paulus Feb 17 '17 at 5:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.