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

[vidiecan]

Real-correctness / Python-hygiene fixes from a code sweep (the mutable
class-attribute defaults were intentionally left out of scope).

• Root-logger hijack — removed logging.basicConfig() at import (a library
  must not configure the root logger). Added a NullHandler and routed the
  stray root logging.* calls through the module _logger.
• No request timeouts — added a configurable per-request timeout
  (DEFAULT_TIMEOUT = 60, timeout= constructor arg) on every session call,
  so a stalled DSpace can't hang the client forever.
• File-handle leak — create_bitstream now opens the upload file in a
  with block; it was previously leaked to the GC.
• Bitstream(None) crash — guarded so the __init__ membership checks
  don't raise TypeError on None.
• Hardcoded debug payload — create_clarinlruallowances now takes a
  metadata_payload argument; the leftover hardcoded {"metadataValue":"Test"}
  is gone (it refuses with no payload).
• super(Cls, self) → super() across models.py.

68 tests (5 new), no network. Happy-path behaviour is unchanged.

[Copilot]

Pull request overview

This PR applies a Python-correctness / hygiene sweep to the DSpace REST client library, focusing on safer logging behavior, bounded HTTP requests, safer file uploads, and guarding edge cases that previously caused runtime errors.

Changes:

• Stops configuring the root logger at import time; routes logging through a module logger and adds a NullHandler.
• Introduces a per-request timeout (DEFAULT_TIMEOUT, constructor timeout=) and applies it across HTTP requests.
• Fixes correctness edge cases: closes upload file handles reliably, guards Bitstream(None), and removes the hardcoded debug payload by requiring a caller-supplied metadata payload.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
dspace_rest_client/client.py	Removes root logger configuration, adds request timeouts everywhere, closes upload files via with, and updates CLARIN allowances API to require an explicit payload.
dspace_rest_client/models.py	Replaces legacy super(Cls, self) patterns with super() and hardens Bitstream initialization against None.
tests/test_client_auth.py	Adds tests asserting default/overridden timeout and verifying no root logger hijack (via NullHandler).
tests/test_client_write.py	Adds tests ensuring CLARIN allowances refuse missing payload and POST the supplied payload correctly.
tests/test_models.py	Adds regression test ensuring Bitstream(None) does not crash.

Suppressed comments (1)

dspace_rest_client/models.py:492

• Avoid shadowing the built-in name dict in InProgressSubmission.as_dict(). Shadowing built-ins makes debugging harder and can break local tooling (e.g., type checkers or debuggers).

        parent_dict = super().as_dict()
        dict = {
            'lastModified': self.lastModified,

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_client_auth.py:40

• This test/comment claims to verify that importing the client does not call logging.basicConfig(), but it only asserts that a NullHandler exists on the module logger. Either add an explicit assertion around basicConfig usage, or (at minimum) rename/trim the test so it matches what it actually checks.

    def test_library_does_not_hijack_root_logger(self):
        # importing the client must not call logging.basicConfig; the module
        # logger carries a NullHandler so records drop unless the app opts in.
        lg = logging.getLogger("dspace.client")
        self.assertTrue(
            any(isinstance(h, logging.NullHandler) for h in lg.handlers))