Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [unreleased] - 2025-12-11
### Changed
- Reuse the same request session on the same Data API instance

### Added
- Added Data API configuration to request signatures
- Added Data API Demo to standalone demos
- Added a close() public method to close the session

### Fixed
- Fixed mypy errors failing lint check on ci build
Expand Down
18 changes: 17 additions & 1 deletion learnosity_sdk/request/dataapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@

class DataApi(object):

def __init__(self) -> None:
self._session = requests.Session()
Comment thread
pablo-estevez-lrn marked this conversation as resolved.
Comment on lines +15 to +16

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗️ Pressing: Adding __init__ here makes super().__init__() mandatory for subclasses. DataApi previously inherited object.__init__, and since we could have customers that don't have a version pinned and did the above we would actually break their integration. We might need to make it Optional

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, if _session is not an attr when we try to do the request, we can create the request. Fixed on f2b5117


def __del__(self) -> None:
self.close()

def _extract_consumer(self, security_packet: Dict[str, str]) -> str:
"""
Extract the consumer key from the security packet.
Expand Down Expand Up @@ -55,6 +61,12 @@ def _derive_action(self, endpoint: str, action: str) -> str:

return f"{action}_{path}"

def close(self) -> None:
Comment thread
Copilot marked this conversation as resolved.
"""Close pooled connections; later requests on this client remain supported."""
session = getattr(self, '_session', None)
if session is not None:
session.close()
Comment on lines +66 to +68

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Question: Why allow later requests? Would we not just fully drop it and have an actual blank slate?

Suggested change
session = getattr(self, '_session', None)
if session is not None:
session.close()
if getattr(self, '_session', None) is not None:
self._session.close()
self._session = None

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 reasons:

  1. To avoid an invalid state after close(), when we raise an error None does not have an attribute post. (after the last commit, will be creating a new session instead)
  2. To use the same known semantic that requests, where close is closing the transport, but not converting to a blank slate.
    If you want to destroy the object, you can always use del()

it will be functionally equivalent, so happy to change if you want


def request(self, endpoint: str, security_packet: Dict[str, str],
secret: str, request_packet:Dict[str, Any] = {}, action: str = 'get') -> Response:
"""
Expand Down Expand Up @@ -91,8 +103,12 @@ def request(self, endpoint: str, security_packet: Dict[str, str],
'X-Learnosity-Action': derived_action,
'X-Learnosity-SDK': f'Python:{sdk_version}'
}
# Lazily create the session so subclasses that override __init__
# without calling super().__init__() still benefit from connection reuse.
if getattr(self, '_session', None) is None:
self._session = requests.Session()

return requests.post(endpoint, data=init.generate(), headers=headers)
return self._session.post(endpoint, data=init.generate(), headers=headers)

def results_iter(self, endpoint: str, security_packet: Dict[str, str],
secret: str, request_packet: Dict[str, Any] = {},
Expand Down
23 changes: 23 additions & 0 deletions tests/integration/test_dataapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,29 @@

assert keys == {'py-sdk-test-2019-1', 'py-sdk-test-2019-2'}

def test_request_after_close(self) -> None:
"""Verify that a client can still make requests after close() is called

Two requests are made to warm the pooled session, close() releases it,
and a third request on the same client must still succeed because the
session lazily rebuilds its connection pool.
"""
client = DataApi()
endpoint = self.__build_base_url() + items_endpoint

references: List[str] = cast(List[str], items_request['references'])

first = client.request(endpoint, security, consumer_secret, items_request, action)
assert first.json()['data'][0]['reference'] in references

Check warning on line 133 in tests/integration/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/integration/test_dataapi.py#L133

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.

second = client.request(endpoint, security, consumer_secret, items_request, action)
assert second.json()['data'][0]['reference'] in references

Check warning on line 136 in tests/integration/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/integration/test_dataapi.py#L136

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.

client.close()

third = client.request(endpoint, security, consumer_secret, items_request, action)
assert third.json()['data'][0]['reference'] in references

Check warning on line 141 in tests/integration/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/integration/test_dataapi.py#L141

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.

def test_question_paging(self) -> None:
"""Verify that paging works"""
client = DataApi()
Expand Down
68 changes: 68 additions & 0 deletions tests/unit/test_dataapi.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from typing import Any, Dict, cast
import unittest
from unittest import mock
import requests
import responses
from learnosity_sdk.request import DataApi
from learnosity_sdk.exceptions import DataApiException
Expand Down Expand Up @@ -221,6 +223,72 @@
assert action == 'get_/itembank/questions'


@responses.activate
def test_session_reused_across_requests(self) -> None:
"""Verify that the same requests.Session is reused across requests on one instance"""
for dummy in self.dummy_responses:
responses.add(responses.POST, self.endpoint, json=dummy)
client = DataApi()

# The instance holds a single Session...
assert isinstance(client._session, requests.Session)

Check warning on line 234 in tests/unit/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/unit/test_dataapi.py#L234

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
session = client._session

# ...and spying on that session's post shows every paginated request
# goes through the same object rather than a fresh connection each time.
original_post = session.post
call_count = 0

def counting_post(*args: Any, **kwargs: Any) -> Any:
nonlocal call_count
call_count += 1
return original_post(*args, **kwargs)

session.post = counting_post # type: ignore[method-assign]

list(client.request_iter(self.endpoint, self.security, self.consumer_secret,
self.request, self.action))

assert call_count == 2

Check warning on line 252 in tests/unit/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/unit/test_dataapi.py#L252

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
assert client._session is session

Check warning on line 253 in tests/unit/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/unit/test_dataapi.py#L253

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.

def test_close_closes_session(self) -> None:
"""Verify that close() closes the underlying session"""
client = DataApi()
with mock.patch.object(client._session, 'close') as mock_close:
client.close()
mock_close.assert_called_once_with()

def test_close_without_session_is_safe(self) -> None:
"""Verify that close() does not raise if the session was never created"""
client = DataApi.__new__(DataApi)
# No __init__ ran, so _session does not exist; close() must not raise.
client.close()

@responses.activate
def test_request_lazily_creates_session_for_subclass_skipping_super(self) -> None:
"""Verify a subclass that overrides __init__ without super() still works

Before session reuse, DataApi had no __init__, so such a subclass was
valid. request() must lazily create the session so these subclasses keep
working and still benefit from connection reuse.
"""
class LegacySubclass(DataApi):
def __init__(self) -> None:
# Intentionally does not call super().__init__()
pass

client = LegacySubclass()
assert getattr(client, '_session', None) is None

Check warning on line 282 in tests/unit/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/unit/test_dataapi.py#L282

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.

responses.add(responses.POST, self.endpoint, json=self.dummy_responses[0])
response = client.request(self.endpoint, self.security, self.consumer_secret, self.request, self.action)

assert response.json() == self.dummy_responses[0]

Check warning on line 287 in tests/unit/test_dataapi.py

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tests/unit/test_dataapi.py#L287

Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
# The session was created on demand, so subsequent calls reuse it.
assert isinstance(client._session, requests.Session)


@responses.activate
def test_metadata_headers_in_paginated_requests(self) -> None:
"""Verify that metadata headers are sent in all paginated requests"""
Expand Down
Loading