pyvdp - Python library for Visa Developer Program

Question

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)

Answer 1

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

• Stop Writing Classes (PyCon 2012)
• Start Writing More Classes (2013)

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

• paulus: urljoin signature in 3.6 assumes 3 arguments: def urljoin(base, url, allow_fragments=True), so I guess you have to do concat anyway?
• alecxe: @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.
• paulus: 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.