Skip to content

fix(client): Python correctness sweep - logging, timeouts, file handle, None guards #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vidiecan merged 2 commits into from
Aug 17, 2026
Merged

fix(client): Python correctness sweep - logging, timeouts, file handle, None guards #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
feeb399
fix(client): correctness sweep - logging, timeouts, file handle, None…
Aug 17, 2026
e9eef06
fix: address Copilot review (handler guard, header copy, docstrings)
Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 53 additions & 35 deletions dspace_rest_client/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,13 @@

__all__ = ['DSpaceClient']

logging.basicConfig(format='%(asctime)s - %(message)s', level=logging.INFO)
_logger = logging.getLogger("dspace.client")
# A library must not configure the root logger - that is the consuming
# application's job. Attach a NullHandler (once) so records are dropped unless
# the application opts in to logging - guarded so reloads/re-imports don't
# accumulate duplicate handlers.
if not any(isinstance(h, logging.NullHandler) for h in _logger.handlers):
_logger.addHandler(logging.NullHandler())


def parse_json(response):
Expand Down Expand Up @@ -79,6 +84,9 @@ class DSpaceClient:
USER_AGENT = os.environ['USER_AGENT']
verbose = False
ITER_PAGE_SIZE = 20
# Default per-request timeout in seconds so a stalled server cannot hang the
# client forever; override via the `timeout` constructor argument.
DEFAULT_TIMEOUT = 60
PROXY_DICT = dict(http=os.environ["PROXY_URL"],https=os.environ["PROXY_URL"]) if "PROXY_URL" in os.environ else dict()

# Simple enum for patch operation types
Expand All @@ -89,7 +97,7 @@ class PatchOperation:
MOVE = 'move'

def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWORD, solr_endpoint=SOLR_ENDPOINT,
solr_auth=SOLR_AUTH, fake_user_agent=False, proxies=PROXY_DICT):
solr_auth=SOLR_AUTH, fake_user_agent=False, proxies=PROXY_DICT, timeout=None):
"""
Accept optional API endpoint, username, password arguments using the OS environment variables as defaults
:param api_endpoint: base path to DSpace REST API, eg. http://localhost:8080/server/api
Expand All @@ -105,6 +113,7 @@ def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWO
self.proxies = proxies
self.solr = None
self._last_err = None
self.timeout = timeout if timeout is not None else self.DEFAULT_TIMEOUT
try:
import pysolr
self.solr = pysolr.Solr(url=solr_endpoint, always_commit=True, timeout=300, auth=solr_auth)
Expand Down Expand Up @@ -137,7 +146,7 @@ def authenticate(self, retry=False):
# Get and update CSRF token
r = self.session.post(self.LOGIN_URL, data={'user': self.USERNAME, 'password': self.PASSWORD},
headers=self.auth_request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand All @@ -164,7 +173,7 @@ def authenticate(self, retry=False):

# Get and check authentication status
r = self.session.get(f'{self.API_ENDPOINT}/authn/status', headers=self.request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
if r.status_code == 200:
r_json = parse_json(r)
if 'authenticated' in r_json and r_json['authenticated'] is True:
Expand Down Expand Up @@ -214,7 +223,7 @@ def api_get(self, url, params=None, data=None, headers=None):
if headers is None:
headers = self.request_headers
r = self.session.get(url, params=params, data=data, headers=headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)
return r

Expand All @@ -230,7 +239,7 @@ def api_post(self, url, params, json, retry=False, timeout=None):
"""
self._last_err = None
r = self.session.post(url, json=json, params=params, headers=self.request_headers,
proxies=self.proxies, timeout=timeout)
proxies=self.proxies, timeout=timeout if timeout is not None else self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand All @@ -252,10 +261,10 @@ def api_post(self, url, params, json, retry=False, timeout=None):
r_json = parse_json(r)
if 'message' in (r_json or {}) and 'Authentication is required' in r_json['message']:
if retry:
logging.error(
_logger.error(
'API Post: Already retried... something must be wrong')
else:
logging.debug("API Post: Retrying request with updated CSRF token")
_logger.debug("API Post: Retrying request with updated CSRF token")
# try to authenticate
self.authenticate()
# Try to authenticate and repeat the request 3 times -
Expand All @@ -275,7 +284,7 @@ def api_post_uri(self, url, params, uri_list, retry=False):
"""
self._last_err = None
r = self.session.post(url, data=uri_list, params=params, headers=self.list_request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand Down Expand Up @@ -305,7 +314,7 @@ def api_put(self, url, params, json, retry=False):
"""
self._last_err = None
r = self.session.put(url, params=params, json=json, headers=self.request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand Down Expand Up @@ -337,7 +346,7 @@ def api_put_uri(self, url, params, uri_list, retry=False):
"""
self._last_err = None
r = self.session.put(url, params=params, data=uri_list, headers=self.list_request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand Down Expand Up @@ -368,7 +377,7 @@ def api_delete(self, url, params, retry=False):
"""
self._last_err = None
r = self.session.delete(url, params=params, headers=self.request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand Down Expand Up @@ -401,15 +410,15 @@ def api_patch(self, url, operation, path, value, params=None, retry=False):
"""
self._last_err = None
if url is None:
logging.error('Missing required URL argument')
_logger.error('Missing required URL argument')
return None
if path is None:
logging.error('Need valid path eg. /withdrawn or /metadata/dc.title/0/language')
_logger.error('Need valid path eg. /withdrawn or /metadata/dc.title/0/language')
return None
if (operation == self.PatchOperation.ADD or operation == self.PatchOperation.REPLACE
or operation == self.PatchOperation.MOVE) and value is None:
# missing value required for add/replace/move operations
logging.error('Missing required "value" argument for add/replace/move operations')
_logger.error('Missing required "value" argument for add/replace/move operations')
return None

# compile patch data
Expand All @@ -426,7 +435,7 @@ def api_patch(self, url, operation, path, value, params=None, retry=False):
# set headers
# perform patch request
r = self.session.patch(url, json=[data], params=params, headers=self.request_headers,
proxies=self.proxies)
proxies=self.proxies, timeout=self.timeout)
self.update_token(r)

if r.status_code == 403:
Expand Down Expand Up @@ -635,7 +644,7 @@ def update_dso(self, dso, params=None):
return None
dso_type = type(dso)
if not isinstance(dso, SimpleDSpaceObject):
logging.error('Only SimpleDSpaceObject types (eg Item, Collection, Community) '
_logger.error('Only SimpleDSpaceObject types (eg Item, Collection, Community) '
'are supported by generic update_dso PUT.')
return dso
try:
Expand Down Expand Up @@ -682,11 +691,11 @@ def delete_dso(self, dso=None, url=None, params=None):
"""
if dso is None:
if url is None:
logging.error('Need a DSO or a URL to delete')
_logger.error('Need a DSO or a URL to delete')
return None
else:
if not isinstance(dso, SimpleDSpaceObject):
logging.error('Only SimpleDSpaceObject types (eg Item, Collection, Community, EPerson) '
_logger.error('Only SimpleDSpaceObject types (eg Item, Collection, Community, EPerson) '
'are supported by generic update_dso PUT.')
return dso
# Get self URI from HAL links
Expand Down Expand Up @@ -844,15 +853,19 @@ def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadat
if metadata is None:
metadata = {}
url = f'{self.API_ENDPOINT}/core/bundles/{bundle.uuid}/bitstreams'
file = (name, open(path, 'rb'), mime)
files = {'file': file}
properties = {'name': name, 'metadata': metadata, 'bundleName': bundle.name}
payload = {'properties': json.dumps(properties) + ';application/json'}
h = self.session.headers
h.update({'Content-Encoding': 'gzip', 'User-Agent': self.USER_AGENT})
req = Request('POST', url, data=payload, headers=h, files=files)
prepared_req = self.session.prepare_request(req)
r = self.session.send(prepared_req, proxies=self.proxies)
# open the file in a context manager so the handle is always closed,
# even if prepare/send raises (it was previously leaked to the GC).
with open(path, 'rb') as fh:
files = {'file': (name, fh, mime)}
properties = {'name': name, 'metadata': metadata, 'bundleName': bundle.name}
payload = {'properties': json.dumps(properties) + ';application/json'}
# copy the session headers so this request's Content-Encoding does
# not leak onto every subsequent request (and across threads)
h = dict(self.session.headers)
h.update({'Content-Encoding': 'gzip', 'User-Agent': self.USER_AGENT})
req = Request('POST', url, data=payload, headers=h, files=files)
prepared_req = self.session.prepare_request(req)
r = self.session.send(prepared_req, proxies=self.proxies, timeout=self.timeout)
if 'DSPACE-XSRF-TOKEN' in r.headers:
t = r.headers['DSPACE-XSRF-TOKEN']
_logger.debug('Updating token to ' + t)
Expand Down Expand Up @@ -1200,7 +1213,7 @@ def create_user(self, user, token=None):

def delete_user(self, user):
if not isinstance(user, User):
logging.error('Must be a valid user')
_logger.error('Must be a valid user')
return None
return self.delete_dso(user)

Expand Down Expand Up @@ -1430,16 +1443,21 @@ def get_clarinlruallowances_by_bitstream_and_user(self, bitstream_uuid, user_uui
return None


def create_clarinlruallowances(self, bitstream_uuid):
def create_clarinlruallowances(self, bitstream_uuid, metadata_payload=None):
"""
Create clarinlruallowances for a bitstream for logged user
by managing user metadata of bitstream.
Create clarinlruallowances for a bitstream for the logged-in user by
managing the bitstream's user metadata.
@param bitstream_uuid: target bitstream UUID
@param metadata_payload: list of {"metadataKey", "metadataValue"} dicts.
Required - there is no meaningful default (the
previous hardcoded "Test" value was leftover
debug data, not usable for real callers).
"""
if not metadata_payload:
_logger.error('create_clarinlruallowances requires a metadata_payload')
return False
url = f'{self.API_ENDPOINT}/core/clarinusermetadata/manage'
params = {'bitstreamUUID': bitstream_uuid}
metadata_payload = [
{"metadataKey": "NAME", "metadataValue": "Test"}
]
try:
response = self.api_post(url, json=metadata_payload, params=params)
if response.status_code == 200:
Expand Down
Loading