diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 0e7e3d2..fd7fb4c 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -7,4 +7,11 @@ updates: interval: "monthly" commit-message: # Prefix all commit messages with "pip" - prefix: "[pip]" \ No newline at end of file + prefix: "[pip]" + # Keep GitHub Actions refs current (workflow uses: pins) + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "monthly" + commit-message: + prefix: "[actions]" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..f651bea --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,111 @@ +# CI quality gates (INT-3877 steps 6-10). +# Offline checks (lint, audit, secret scan, tests) run on every PR. +# The live-API suite runs only on push to master: Actions secrets are not +# exposed to fork PRs, and the suite performs real writes. +name: CI + +# No paths-ignore here: these checks become required via branch +# protection (INT-3877 step 12), and a required check that never +# triggers leaves doc-only PRs stuck in the "Expected" state. +on: + push: + branches: [master] + pull_request: + branches: [master] + +permissions: + contents: read + +# Cancel superseded runs on PR churn only. Push runs to master include +# the live-API suite, which mutates real data; a mid-test cancellation +# would leave test resources behind. +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +jobs: + lint: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Install ruff + run: pip install ruff==0.16.8 + - name: Lint + run: ruff check . + + audit: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Install pip-audit + run: pip install pip-audit==2.10.1 + # pip-audit has no severity filter; it fails on any known + # vulnerability, which is stricter than the "high severity" gate + # the ticket asks for. + - name: Audit dependencies + run: pip-audit -r requirements.txt + + secret-scan: + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + # gitleaks-action requires a license for organization repos; + # the CLI performs the same full-history scan without one. + - name: Run gitleaks (full history) + run: | + curl -sSfLO https://github.com/gitleaks/gitleaks/releases/download/v8.30.1/gitleaks_8.30.1_linux_x64.tar.gz + echo "551f6fc83ea457d62a0d98237cbad105af8d557003051f41f3e7ca7b3f2470eb gitleaks_8.30.1_linux_x64.tar.gz" | sha256sum -c - + tar -xzf gitleaks_8.30.1_linux_x64.tar.gz gitleaks + ./gitleaks git --redact --verbose . + + test-offline: + runs-on: ubuntu-latest + timeout-minutes: 10 + strategy: + matrix: + python-version: ['3.10', '3.11', '3.12'] + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: ${{ matrix.python-version }} + - name: Install dependencies + run: | + pip install -r requirements.txt + pip install build==1.6.1 + - name: Build package + run: python -m build + # Install the wheel the build step produced, not the source tree, + # so the smoke test exercises the artifact pypi-publish.yml ships. + # Import from outside the repo root: in the root, the onfleet/ + # source directory shadows the installed wheel. + - name: Import smoke test + run: | + pip install dist/*.whl + cd "$RUNNER_TEMP" + python -c "import onfleet; print(onfleet.Onfleet)" + # Without ONFLEET_API_KEY the live-API tests skip; the API-free + # unit tests in test_request_offline.py still run and assert. + - name: Run test suite (offline) + run: python -m unittest discover -s test -v + + # The job stays disabled until the ONFLEET_API_KEY secret has a value: + # set the LIVE_SUITE_ENABLED repository variable to 'true' when the + # secret lands. Secrets are not readable in job-level `if` expressions; + # variables are. This avoids a red master while the secret is pending. + test-live: + if: github.event_name == 'push' && vars.LIVE_SUITE_ENABLED == 'true' + needs: [lint, test-offline] + uses: ./.github/workflows/live-suite.yml + secrets: inherit diff --git a/.github/workflows/live-suite.yml b/.github/workflows/live-suite.yml new file mode 100644 index 0000000..5d7c611 --- /dev/null +++ b/.github/workflows/live-suite.yml @@ -0,0 +1,44 @@ +# Reusable live-API test suite, called from ci.yml (push to master) +# and pypi-publish.yml (release gate). Single Python version: the suite +# mutates real data in the test organization, so concurrent matrix jobs +# would race each other. Per-version coverage runs offline in ci.yml. +name: Live suite + +on: + workflow_call: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + timeout-minutes: 10 + # Both callers share this group, so a master push and a release + # never mutate the test organization at the same time. Never cancel + # a running suite: a mid-test cancellation leaves test resources + # behind. + concurrency: + group: live-api-suite + cancel-in-progress: false + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.12' + - name: Install dependencies + run: pip install -r requirements.txt + # Without the secret the suite skips every test and the job would + # pass green with zero assertions. Fail loudly instead. + - name: Require ONFLEET_API_KEY + env: + ONFLEET_API_KEY: ${{ secrets.ONFLEET_API_KEY }} + run: | + if [ -z "$ONFLEET_API_KEY" ]; then + echo "::error::The live-API suite cannot run without the ONFLEET_API_KEY secret." + exit 1 + fi + - name: Run test suite (live API) + env: + ONFLEET_API_KEY: ${{ secrets.ONFLEET_API_KEY }} + run: python -m unittest discover -s test -v diff --git a/.github/workflows/pypi-publish.yml b/.github/workflows/pypi-publish.yml index e963a9f..54f7569 100644 --- a/.github/workflows/pypi-publish.yml +++ b/.github/workflows/pypi-publish.yml @@ -7,25 +7,34 @@ on: release: types: [published] +# The token needs no write access: PyPI uses PYPI_API_TOKEN and Slack +# uses a webhook secret. +permissions: + contents: read + jobs: -# TODO: Run tests -# build: -# runs-on: ubuntu-latest + # Release gate: the shared live-API suite. Unlike ci.yml, this call is + # not gated on LIVE_SUITE_ENABLED — a release must never skip tests, + # so a missing secret fails the run loudly inside the suite. + test: + uses: ./.github/workflows/live-suite.yml + secrets: inherit publish: - #needs: build + needs: test runs-on: ubuntu-latest + timeout-minutes: 10 steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v2 + uses: actions/setup-python@v5 with: - python-version: '3.x' + python-version: '3.12' - name: Install dependencies run: | python -m pip install --upgrade pip - pip install build + pip install build==1.6.1 - name: Build package run: python -m build - name: Publish package @@ -37,7 +46,10 @@ jobs: notify: needs: publish runs-on: ubuntu-latest + timeout-minutes: 10 steps: + # TODO: bump slack-github-action (v2+ changed the webhook inputs; + # the payload and SLACK_WEBHOOK_TYPE need a review with the bump). - uses: slackapi/slack-github-action@v1.18.0 with: payload: | diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index ed36238..0000000 --- a/.travis.yml +++ /dev/null @@ -1,8 +0,0 @@ -language: python -python: - - "3.7" -cache: pip -install: - - pip install -r requirements.txt -script: - # - python test/test_onfleet.py \ No newline at end of file diff --git a/onfleet/__init__.py b/onfleet/__init__.py index 003c842..e86e82e 100644 --- a/onfleet/__init__.py +++ b/onfleet/__init__.py @@ -1,6 +1,9 @@ -from onfleet.onfleet import Onfleet from onfleet.endpoint import Endpoint +from onfleet.error import HttpError, PermissionError, RateLimitError, ServiceError, ValidationError +from onfleet.onfleet import Onfleet from onfleet.request import Request -from onfleet.error import ValidationError, PermissionError, HttpError, RateLimitError, ServiceError -__all__ = ["Onfleet"] \ No newline at end of file +# Only Onfleet is exported on star imports. The error classes stay +# importable by name, but out of `__all__`: onfleet.PermissionError is +# the API exception, and a star import must not shadow the builtin. +__all__ = ["Onfleet"] diff --git a/onfleet/error.py b/onfleet/error.py index 5756f10..e05a569 100644 --- a/onfleet/error.py +++ b/onfleet/error.py @@ -1,6 +1,10 @@ class _OnfleetError(Exception): ''' Base exception class for all errors raised by the Onfleet API. + + `status` holds the Onfleet error code (e.g. 1500) when the API + returns a structured error payload, and the HTTP status code + (e.g. 502) when the payload is malformed. ''' def __init__(self, message, status=None, request=None, cause=None): self.message = message diff --git a/onfleet/request.py b/onfleet/request.py index 4cdb02d..7527b8c 100644 --- a/onfleet/request.py +++ b/onfleet/request.py @@ -3,7 +3,7 @@ import time from urllib import parse -from backoff import on_exception, expo +from backoff import expo, on_exception from ratelimit import limits from onfleet._meta import __version__ @@ -27,7 +27,8 @@ def __init__(self, http_method, path, session): @on_exception(expo, RateLimitError, max_tries=8) @limits(calls=RATE_LIMIT, period=1) - def __call__(self, id=None, body=None, workerId=None, hubId=None, googleApiKey=None, queryParams=None, **extra_data): + def __call__(self, id=None, body=None, workerId=None, hubId=None, googleApiKey=None, queryParams=None, + **extra_data): obj_id = id # TODO(julian): `id` is a reserved name, let's rename it to 'obj_id' query_params = queryParams # TODO(julian): Let's rename `queryParams` to `query_params` @@ -60,16 +61,30 @@ def __call__(self, id=None, body=None, workerId=None, hubId=None, googleApiKey=N time.sleep(1 / rate_remaining) return response.status_code if (self.http_method == 'DELETE' or 'complete' in url) else response.json() - error = json.loads(response.text) - if type(error['message']) is not dict: - raise HttpError(error.get('message', 'Error'), response.status_code) - - error_message = error['message']['message'] - error_code = error['message']['error'] - error_request = error['message']['request'] - error_cause = error['message'].get('cause') + # The expected error shape is {'message': {...}}. Malformed + # payloads (non-JSON body, missing or non-dict 'message') must + # still raise the intended exception, not KeyError (issue #30). + try: + error = json.loads(response.text) + except ValueError: + error = None + error_body = error.get('message') if isinstance(error, dict) else None + + if not isinstance(error_body, dict): + raise HttpError(error_body or response.text or 'Error', response.status_code) + + error_message = error_body.get('message', 'Error') + error_code = error_body.get('error', 0) + error_request = error_body.get('request') + error_cause = error_body.get('cause') exception_args = (error_message, error_code, error_request, error_cause) + # A null or non-numeric code cannot be classified; the range + # checks below would raise TypeError on it. Floats stay + # classifiable: the range checks compare them fine. + if not isinstance(error_code, (int, float)): + raise HttpError(*exception_args) + # https://github.com/addyinc/web/blob/master/yerba/config/errors.json if 1000 <= error_code <= 1012: # InvalidContentError diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 0000000..95154e8 --- /dev/null +++ b/ruff.toml @@ -0,0 +1,14 @@ +# Minimal ruff configuration (INT-3877 step 8). +target-version = "py310" +line-length = 120 + +[lint] +# Pinned rule set: pycodestyle errors (incl. E501 line length), +# pyflakes, import sorting. +# Pinning keeps CI deterministic across ruff versions. +select = ["E4", "E5", "E7", "E9", "F", "I"] + +[lint.per-file-ignores] +# Re-export module: the imports exist for `onfleet.` access and +# are deliberately absent from `__all__`. +"onfleet/__init__.py" = ["F401"] diff --git a/setup.py b/setup.py index 7cbc9fd..6176454 100644 --- a/setup.py +++ b/setup.py @@ -1,5 +1,6 @@ import ast import pathlib + import setuptools PROJECT_ROOT = pathlib.Path(__file__).parent diff --git a/test/test_onfleet.py b/test/test_onfleet.py index 713cd0c..97ad404 100644 --- a/test/test_onfleet.py +++ b/test/test_onfleet.py @@ -1,12 +1,9 @@ -import sys -sys.path.append("../onfleet") -import onfleet - import os import unittest -import re from datetime import datetime, timedelta +import onfleet + # TODO(julian): We need to deprecate this! class TestOnfleet(unittest.TestCase): @@ -21,10 +18,15 @@ def setUp(self): self.api = onfleet.Onfleet(api_key=self.api_key) def test_authenticate(self): + # A bad key raises PermissionError on either call, so reaching the + # asserts proves authentication. The old cross-check against the + # auth message no longer works: the message does not embed the + # organization ID anymore, and the ID format is not guaranteed. auth_result = self.api.auth_test() org_result = self.api.organization.get() - match = re.search(r"[a-zA-Z\d]{24}", auth_result["message"]) - self.assertTrue(match.group(0) == org_result["id"]) + self.assertIn("message", auth_result) + self.assertIsInstance(org_result["id"], str) + self.assertTrue(org_result["id"]) def test_datatype(self): admins_list = self.api.administrators.get() @@ -47,7 +49,8 @@ def test_integrated_functions(self): new_admin_list_length = len(self.api.administrators.get()) # Length of admin list should increase by 1 self.assertTrue(old_admin_list_length + 1 == new_admin_list_length) - updated_result = self.api.administrators.update(id=new_admin_id, body={"name": "Onfleet Testing - Please delete"}) + updated_result = self.api.administrators.update( + id=new_admin_id, body={"name": "Onfleet Testing - Please delete"}) # Admin name should be updated self.assertTrue(updated_result["name"] == "Onfleet Testing - Please delete") deletion_result = self.api.administrators.deleteOne(id=new_admin_id) @@ -104,12 +107,18 @@ def test_find_and_query_parameters(self): search_result = (self.api.recipients.get(search=data)) # Create the recipient if not exist if ("id" not in search_result): - new_recipient_data = {"name":"Boris Foster","phone":"+16505551133","notes":"Always orders our GSC special", "skipPhoneNumberValidation":"true"} + new_recipient_data = { + "name": "Boris Foster", + "phone": "+16505551133", + "notes": "Always orders our GSC special", + "skipPhoneNumberValidation": "true" + } new_recipient = self.api.recipients.create(body=new_recipient_data) recipient_id = new_recipient["id"] else: recipient_id = search_result["id"] - search_result_with_query = self.api.recipients.get(search=data, queryParams={"skipPhoneNumberValidation":"true"}) + search_result_with_query = self.api.recipients.get( + search=data, queryParams={"skipPhoneNumberValidation": "true"}) self.assertTrue(search_result_with_query["id"] == recipient_id) diff --git a/test/test_request_offline.py b/test/test_request_offline.py new file mode 100644 index 0000000..58e863b --- /dev/null +++ b/test/test_request_offline.py @@ -0,0 +1,141 @@ +import json +import unittest +from unittest.mock import Mock + +from onfleet.error import HttpError, PermissionError, ServiceError, ValidationError +from onfleet.request import Request + + +def _response(payload, status_code=400, ok=False): + response = Mock() + response.ok = ok + response.text = json.dumps(payload) + response.status_code = status_code + response.headers = {'X-RateLimit-Remaining': '100'} + response.json.return_value = payload + return response + + +def _request(http_method, payload, status_code=400, ok=False): + session = Mock() + session.request.return_value = _response(payload, status_code, ok) + return Request(http_method, '/tasks', session) + + +class TestRequestUrlHelpers(unittest.TestCase): + + def test_path_selector_single_path(self): + self.assertEqual(Request._path_selector('/tasks', None, {}), '/tasks') + + def test_path_selector_get_all(self): + paths = ('/workers', '/workers/:workerId') + self.assertEqual(Request._path_selector(paths, None, {}), '/workers') + + def test_path_selector_get_by_id(self): + paths = ('/workers', '/workers/:workerId') + self.assertEqual(Request._path_selector(paths, '123', {}), '/workers/:workerId') + + def test_url_id_setter(self): + url = Request._url_id_setter('https://onfleet.com/api/v2/admins/:adminId', 'abc123') + self.assertEqual(url, 'https://onfleet.com/api/v2/admins/abc123') + + def test_url_extra_data_setter_entity(self): + url = Request._url_extra_data_setter( + 'https://onfleet.com/api/v2/containers/:entityType/:entityId', {'workers': 'w123'}) + self.assertEqual(url, 'https://onfleet.com/api/v2/containers/workers/w123') + + def test_url_extra_data_setter_lookup(self): + url = Request._url_extra_data_setter( + 'https://onfleet.com/api/v2/tasks/:taskId', {'shortId': '44a5'}) + self.assertEqual(url, 'https://onfleet.com/api/v2/tasks/shortId/44a5') + + def test_clean_params_string(self): + self.assertEqual(Request._clean_params('from=1,to=2'), {'from': '1', 'to': '2'}) + + def test_clean_params_dict_passthrough(self): + params = {'from': '1'} + self.assertIs(Request._clean_params(params), params) + + def test_clean_params_none(self): + self.assertIsNone(Request._clean_params(None)) + + +class TestRequestResponses(unittest.TestCase): + + def test_get_returns_json_body(self): + payload = {'id': 'abc123'} + self.assertEqual(_request('GET', payload, status_code=200, ok=True)(), payload) + + def test_delete_returns_status_code(self): + self.assertEqual(_request('DELETE', {}, status_code=200, ok=True)(), 200) + + +class TestRequestErrorClassification(unittest.TestCase): + + def _error_payload(self, code): + return {'message': {'message': 'Boom', 'error': code, 'request': 'req-id'}} + + def test_non_dict_message_raises_http_error(self): + with self.assertRaises(HttpError): + _request('GET', {'message': 'plain text error'})() + + def test_validation_error(self): + with self.assertRaises(ValidationError): + _request('GET', self._error_payload(1000))() + + def test_permission_error(self): + with self.assertRaises(PermissionError): + _request('GET', self._error_payload(1100))() + + def test_service_error(self): + with self.assertRaises(ServiceError): + _request('GET', self._error_payload(2500), status_code=500)() + + def test_unclassified_code_raises_http_error(self): + with self.assertRaises(HttpError): + _request('GET', self._error_payload(1500))() + + # Regression tests for the fix re-applied from PR #54 (issue #30): + # payloads with missing optional fields raise the intended exception, + # not KeyError. + def test_missing_optional_fields_raise_http_error(self): + with self.assertRaises(HttpError): + _request('GET', {'message': {'message': 'Boom'}})() + + def test_empty_message_dict_raises_http_error(self): + with self.assertRaises(HttpError): + _request('GET', {'message': {}})() + + def test_missing_code_keeps_error_details(self): + with self.assertRaises(HttpError) as context: + _request('GET', {'message': {'message': 'Boom'}})() + self.assertIn('Boom', context.exception.args) + + def test_null_error_code_raises_http_error(self): + with self.assertRaises(HttpError): + _request('GET', {'message': {'message': 'Boom', 'error': None}})() + + def test_string_error_code_raises_http_error(self): + with self.assertRaises(HttpError): + _request('GET', {'message': {'message': 'Boom', 'error': '1000'}})() + + def test_float_error_code_still_classifies(self): + with self.assertRaises(ValidationError): + _request('GET', self._error_payload(1000.0))() + + def test_missing_top_level_message_raises_http_error(self): + with self.assertRaises(HttpError): + _request('GET', {})() + + def test_non_json_body_raises_http_error(self): + response = _response({}) + response.text = '502 Bad Gateway' + session = Mock() + session.request.return_value = response + with self.assertRaises(HttpError) as context: + Request('GET', '/tasks', session)() + self.assertIn('502 Bad Gateway', context.exception.args) + + +if (__name__ == '__main__'): + unittest.main()