Housekeeping: Refactor all duplicated calls to Request to a base class with a method

[jathanism]

Proposal

I propose that a serious refactor is performed to reduce all of the code duplication in this library predominantly around how Request objects are repeatedly constructed and the code to do so is duplicated all over the library.

This was exposed when we recently began the work on adding support for API versioning by adding the api_version= argument, which requires updating the calls in numerous places when it should be done in ONE and only one place.

Justfication

For example, consider the source for pynautobot.core.api where the Api class has three properties: Api.version vs. Api.openapi vs Api.status. Each of these is making the exact same duplicated call to Request(base=self.base_url, http_session=self.http_session, api_version=self.api_version) and then calling a method on the Request object (get_version(), get_openapi(), and get_status() respectively):

pynautobot/pynautobot/core/api.py

Lines 91 to 165 in 3956bd9

	@property
	def version(self):
	"""Gets the API version of Nautobot.
	Can be used to check the Nautobot API version if there are
	version-dependent features or syntaxes in the API.
	:Returns: Version number as a string.
	:Example:
	>>> import pynautobot
	>>> nb = pynautobot.api(
	... 'http://localhost:8000',
	... token='d6f4e314a5b5fefd164995169f28ae32d987704f'
	... )
	>>> nb.version
	'1.0'
	>>>
	"""
	version = Request(
	base=self.base_url, http_session=self.http_session, api_version=self.api_version
	).get_version()
	return version
	def openapi(self):
	"""Returns the OpenAPI spec.
	Quick helper function to pull down the entire OpenAPI spec.
	:Returns: dict
	:Example:
	>>> import pynautobot
	>>> nb = pynautobot.api(
	... 'http://localhost:8000',
	... token='d6f4e314a5b5fefd164995169f28ae32d987704f'
	... )
	>>> nb.openapi()
	{...}
	>>>
	"""
	return Request(base=self.base_url, http_session=self.http_session, api_version=self.api_version,).get_openapi()
	def status(self):
	"""Gets the status information from Nautobot.
	Available in Nautobot 2.10.0 or newer.
	:Returns: Dictionary as returned by Nautobot.
	:Raises: :py:class:`.RequestError` if the request is not successful.
	:Example:
	>>> pprint.pprint(nb.status())
	{'django-version': '3.1.3',
	'installed-apps': {'cacheops': '5.0.1',
	'debug_toolbar': '3.1.1',
	'django_filters': '2.4.0',
	'django_prometheus': '2.1.0',
	'django_rq': '2.4.0',
	'django_tables2': '2.3.3',
	'drf_yasg': '1.20.0',
	'mptt': '0.11.0',
	'rest_framework': '3.12.2',
	'taggit': '1.3.0',
	'timezone_field': '4.0'},
	'nautobot-version': '1.0.0',
	'plugins': {},
	'python-version': '3.7.3',
	'rq-workers-running': 1}
	>>>
	"""
	status = Request(
	base=self.base_url, token=self.token, http_session=self.http_session, api_version=self.api_version,
	).get_status()
	return status

Following the code path to the source for pynautobot.core.query.Request, you can then see that each of these methods ALSO has nearly the exact same code duplicated for each instead of calling a centralized method that already sets the same header value and input parameters:

pynautobot/pynautobot/core/query.py

Lines 145 to 198 in 3956bd9

	def get_openapi(self):
	""" Gets the OpenAPI Spec """
	headers = {
	"Content-Type": "application/json;",
	}
	if self.api_version:
	headers["accept"] = f"application/json; version={self.api_version}"
	req = self.http_session.get("{}docs/?format=openapi".format(self.normalize_url(self.base)), headers=headers,)
	if req.ok:
	return req.json()
	else:
	raise RequestError(req)
	def get_version(self):
	"""Gets the API version of Nautobot.
	Issues a GET request to the base URL to read the API version from the
	response headers.
	:Raises: RequestError if req.ok returns false.
	:Returns: Version number as a string. Empty string if version is not
	present in the headers.
	"""
	headers = {"Content-Type": "application/json;"}
	if self.api_version:
	headers["accept"] = f"application/json; version={self.api_version}"
	req = self.http_session.get(self.normalize_url(self.base), headers=headers,)
	if req.ok:
	return req.headers.get("API-Version", "")
	else:
	raise RequestError(req)
	def get_status(self):
	"""Gets the status from /api/status/ endpoint in Nautobot.
	:Returns: Dictionary as returned by Nautobot.
	:Raises: RequestError if request is not successful.
	"""
	headers = {"Content-Type": "application/json;"}
	if self.token:
	headers["authorization"] = "Token {}".format(self.token)
	if self.api_version:
	headers["accept"] = f"application/json; version={self.api_version}"
	req = self.http_session.get("{}status/".format(self.normalize_url(self.base)), headers=headers,)
	if req.ok:
	return req.json()
	else:
	raise RequestError(req)

Longer term this is not a maintainable pattern. There are so many individual calls to create Request instances where instead each of the various Api and Endpoint objects or anything that is behaving as a client to the API should inherit from a common base and call the same underlying method on every request using the same centralized code path.

This will make the code easier to maintain and easier to use across the board and assert that when we want to extend or augment existing functionality to support new API features, they can be done in a single place, and won't require scouring the entire source tree.

[jvanderaa]

In this refactor, should we also look at moving from using Requests to perhaps HTTPX that may get some async capabilities for users while maintaining the similar Requests API calls?

[jathanism]

HTTPX

Worth considering as part of the scoping at a minimum. First I've personally heard of httpx.