Skip to content

Commit f3f1080

Browse files
committed
fix: a wiki file finds its session without a URL
- _ensure_session falls back to the registered credential, so an upload (which has no URL yet) can use the lazy session at all - from_other keeps a session the source carries instead of writing None - sessions are cached per credential, not per host, and a replaced credential replaces the session - LocalFileController without a file fails at construction, as the controller it replaces did - pin opensemantic.core to the branch carrying the file controllers
1 parent 7e63571 commit f3f1080

5 files changed

Lines changed: 257 additions & 22 deletions

File tree

‎pyproject.toml‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,12 @@ classifiers = [
2121
"Topic :: Software Development :: Libraries :: Python Modules",
2222
]
2323
dependencies = [
24-
"oold>=0.15.0",
24+
# registered_credentials(), used to find the wiki of a file without a URL
25+
"oold>=1.1.0",
2526
"opensemantic",
26-
"opensemantic.core>=0.57.4",
27+
# TEMPORARY: the file controllers this module wraps are on the core branch
28+
# below. Replace with opensemantic.core>=0.59.0 once released.
29+
"opensemantic.core @ git+https://github.com/OpenSemanticWorld-Packages/opensemantic.core-python@feat/regen-core-v0.59.0",
2730
"opensemantic.base>=0.42.7",
2831
"pydantic[email]>=2.12.0",
2932
"datamodel-code-generator==0.51.0",

‎src/osw/controller/file/local.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,15 @@ def __init__(self, **kwargs):
2525
if path is not None and not kwargs.get("local_path"):
2626
kwargs["local_path"] = str(path)
2727
super().__init__(**kwargs)
28-
if self.local_path:
29-
self._set_metadata()
28+
if not self.local_path:
29+
# The controller this replaces declared path as a required field,
30+
# so an instance without one never existed. Raising here keeps the
31+
# error at the call site that is wrong, instead of surfacing it on
32+
# the first read.
33+
raise ValueError(
34+
f"{type(self).__name__} needs a file: pass path= or local_path="
35+
)
36+
self._set_metadata()
3037

3138
@classmethod
3239
def from_other(cls, other, path=None, **kwargs) -> "LocalFileController":

‎src/osw/controller/file/wiki.py‎

Lines changed: 67 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import functools
22
import logging
33
import os
4-
from typing import IO, Any, Dict, List, Optional
4+
from typing import IO, Any, Dict, List, Optional, Tuple
55
from urllib.parse import urlparse
66

77
import mwclient.errors
88
from oold.backend.auth import get_credential as _get_registered_credential
9+
from oold.backend.auth import registered_credentials as _registered_credentials
910

1011
from osw.auth import CredentialManager
1112
from osw.controller.file.base import FileController
@@ -82,31 +83,49 @@ def store_params_for_upload(
8283
return {**se_params, "overwrite": "replace remote"}
8384

8485

85-
_sessions: Dict[str, OSW] = {}
86-
"""Sessions by host, so several files of one wiki share a connection."""
86+
_sessions: Dict[str, Tuple[Any, OSW]] = {}
87+
"""Sessions by credential IRI, with the credential they were built from.
8788
89+
Keyed by the credential rather than by the host, because two credentials
90+
scoped to different paths of one wiki are two different logins. The credential
91+
is kept so a later ``set_credential`` replaces the session instead of being
92+
ignored for the lifetime of the process.
93+
"""
8894

89-
def session_for(url: str) -> OSW:
90-
"""An OSW session for the host of ``url``, from the registered credential.
9195

92-
The file entity may be stored somewhere other than its bytes, so the
93-
lookup key is the URL the content lives at, not the entity's own IRI.
94-
"""
96+
def clear_sessions() -> None:
97+
"""Drop the cached sessions, so the next access logs in again."""
98+
_sessions.clear()
99+
100+
101+
def credential_for(url: str) -> Any:
102+
"""The credential registered for ``url``, or a readable error."""
95103
host = urlparse(url).netloc or url
96-
session = _sessions.get(host)
97-
if session is not None:
98-
return session
99104
try:
100-
credential = _get_registered_credential(url)
105+
return _get_registered_credential(url)
101106
except ValueError as error:
102107
raise ValueError(
103108
f"No credential registered for {host}. Pass osw= explicitly or "
104109
"register one with oold.backend.auth.set_credential."
105110
) from error
111+
112+
113+
def session_for(url: str, credential: Any = None) -> OSW:
114+
"""An OSW session for the host of ``url``, from the registered credential.
115+
116+
The file entity may be stored somewhere other than its bytes, so the
117+
lookup key is the URL the content lives at, not the entity's own IRI.
118+
"""
119+
host = urlparse(url).netloc or url
120+
if credential is None:
121+
credential = credential_for(url)
122+
cached = _sessions.get(credential.iri)
123+
if cached is not None and cached[0] == credential:
124+
return cached[1]
106125
cred_mngr = CredentialManager()
107126
cred_mngr.add_credential(credential)
108127
session = OSW(site=WtSite(WtSite.WtSiteConfig(iri=host, cred_mngr=cred_mngr)))
109-
_sessions[host] = session
128+
_sessions[credential.iri] = (credential, session)
110129
return session
111130

112131

@@ -135,7 +154,12 @@ def from_other(
135154
osw: Optional[OSW] = None,
136155
**kwargs: Dict[str, Any],
137156
) -> "WikiFileController":
138-
return super().from_other(other, osw=osw, **kwargs)
157+
# Only pass a session that was given: writing None over the one the
158+
# source already carries would defer the failure to put time, with a
159+
# message about a missing credential rather than a dropped session.
160+
if osw is not None:
161+
kwargs["osw"] = osw
162+
return super().from_other(other, **kwargs)
139163

140164
def get(self) -> IO:
141165
self._init()
@@ -288,11 +312,38 @@ def url(self):
288312
)
289313

290314
def _ensure_session(self) -> OSW:
291-
"""The session, connecting on first use when none was passed in."""
315+
"""The session, connecting on first use when none was passed in.
316+
317+
A file that is about to be uploaded has no URL yet - that is what the
318+
upload produces - so the URL is only one of the ways to find the wiki.
319+
Without one the registered credentials decide, and they are only
320+
unambiguous when there is exactly one.
321+
"""
292322
if self.osw is None:
293-
self.osw = session_for(self._url())
323+
self.osw = session_for(self._session_url())
294324
return self.osw
295325

326+
def _session_url(self) -> str:
327+
"""The wiki to connect to: the file's own URL, else the credential's."""
328+
try:
329+
return self._url()
330+
except ValueError:
331+
pass
332+
registered = _registered_credentials()
333+
if len(registered) == 1:
334+
return next(iter(registered))
335+
if not registered:
336+
raise ValueError(
337+
f"{type(self).__name__} has no download_url or access_url and "
338+
"no credential is registered. Pass osw= explicitly or register "
339+
"one with oold.backend.auth.set_credential."
340+
)
341+
raise ValueError(
342+
f"{type(self).__name__} has no download_url or access_url, and "
343+
f"credentials are registered for {', '.join(sorted(registered))}. "
344+
"Pass osw= explicitly to say which wiki this file belongs to."
345+
)
346+
296347
def _init(self, name=None, suffix=None):
297348
self._ensure_session()
298349
# set the name attribute to the actual file name, e.g., "image.png"

‎tests/test_in_memory_upload.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@
2020
def test_controller_accepts_a_caller_supplied_stream():
2121
stream = BytesIO(b"payload")
2222
controller = InMemoryController(stream=stream)
23-
assert controller.get() is stream
23+
assert controller.stream is stream
24+
# get() hands out a reader over the buffer, not the buffer itself, so a
25+
# caller closing what it read does not empty the controller
26+
assert controller.get().read() == b"payload"
2427
assert controller.get().read() == b"payload"
2528

2629

@@ -68,7 +71,7 @@ def _fake_from_other(other, osw, **data):
6871

6972
controller = seen["source_file_controller"]
7073
assert isinstance(controller, InMemoryController)
71-
assert controller.get() is stream
74+
assert controller.stream is stream
7275

7376

7477
def test_upload_rejects_a_source_that_is_not_file_like():

‎tests/test_wiki_file_session.py‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
"""Unit tests for how a wiki file controller finds its session.
2+
3+
The session is optional: a controller built from a file entity connects on
4+
first use. A file that is about to be uploaded has no URL yet - that is what
5+
the upload produces - so the URL cannot be the only way to find the wiki.
6+
7+
Offline: the session is never built here, only the inputs that decide which
8+
one would be.
9+
"""
10+
11+
from io import BytesIO
12+
13+
import pytest
14+
from oold.backend.auth import UserPwdCredential, set_credential
15+
16+
import osw.controller.file.wiki as wiki_module
17+
from osw.controller.file.local import LocalFileController
18+
from osw.controller.file.wiki import OSW, WikiFileController, WtSite
19+
20+
21+
@pytest.fixture(autouse=True)
22+
def _clean_credentials(monkeypatch):
23+
"""Keep the process-wide credential store and session cache per test."""
24+
import oold.backend.auth as auth
25+
26+
saved = auth._credentials.get()
27+
auth._credentials.set({})
28+
wiki_module.clear_sessions()
29+
yield
30+
auth._credentials.set(saved)
31+
wiki_module.clear_sessions()
32+
33+
34+
def _credential(iri, username="bot", password="secret"):
35+
return UserPwdCredential(iri=iri, username=username, password=password)
36+
37+
38+
def _a_session() -> OSW:
39+
"""A stand-in session: only which one travels is under test.
40+
41+
pydantic v1 copies a model on assignment, so the controller does not hold
42+
the very object that was passed in; the sessions are told apart by the
43+
site they carry.
44+
"""
45+
return OSW.construct(site=WtSite.__new__(WtSite))
46+
47+
48+
def _controller(**kwargs):
49+
return WikiFileController(**kwargs)
50+
51+
52+
# -- choosing the wiki --------------------------------------------------------
53+
54+
55+
def test_a_url_decides_the_wiki():
56+
controller = _controller(download_url="https://wiki.example.org/wiki/File:A.bin")
57+
assert controller._session_url() == "https://wiki.example.org/wiki/File:A.bin"
58+
59+
60+
def test_without_a_url_the_single_registered_credential_decides():
61+
"""A fresh upload has no URL, so the credential is the only pointer."""
62+
set_credential(_credential("wiki.example.org"))
63+
assert _controller()._session_url() == "wiki.example.org"
64+
65+
66+
def test_without_a_url_and_without_a_credential_the_error_says_so():
67+
with pytest.raises(ValueError, match="no credential is registered"):
68+
_controller()._session_url()
69+
70+
71+
def test_several_credentials_without_a_url_are_ambiguous():
72+
set_credential(_credential("wiki.example.org"))
73+
set_credential(_credential("other.example.org"))
74+
with pytest.raises(ValueError, match=r"other\.example\.org"):
75+
_controller()._session_url()
76+
77+
78+
def test_a_session_that_was_passed_in_is_kept():
79+
sentinel = _a_session()
80+
controller = _controller(osw=sentinel)
81+
assert controller._ensure_session().site is sentinel.site
82+
83+
84+
# -- the session cache --------------------------------------------------------
85+
86+
87+
def test_sessions_are_cached_per_credential(monkeypatch):
88+
"""Two path-scoped credentials of one host are two logins."""
89+
built = []
90+
monkeypatch.setattr(
91+
wiki_module, "OSW", lambda site: built.append(site) or f"session{len(built)}"
92+
)
93+
monkeypatch.setattr(wiki_module, "WtSite", lambda config: config)
94+
monkeypatch.setattr(wiki_module.WtSite, "WtSiteConfig", dict, raising=False)
95+
set_credential(_credential("wiki.example.org/a", username="a"))
96+
set_credential(_credential("wiki.example.org/b", username="b"))
97+
98+
first = wiki_module.session_for("https://wiki.example.org/a/File:X")
99+
second = wiki_module.session_for("https://wiki.example.org/b/File:X")
100+
again = wiki_module.session_for("https://wiki.example.org/a/File:Y")
101+
102+
assert first != second
103+
assert again == first
104+
105+
106+
def test_a_replaced_credential_replaces_the_session(monkeypatch):
107+
"""A later set_credential must not be ignored for the process lifetime."""
108+
built = []
109+
monkeypatch.setattr(
110+
wiki_module, "OSW", lambda site: built.append(site) or f"session{len(built)}"
111+
)
112+
monkeypatch.setattr(wiki_module, "WtSite", lambda config: config)
113+
monkeypatch.setattr(wiki_module.WtSite, "WtSiteConfig", dict, raising=False)
114+
set_credential(_credential("wiki.example.org", password="old"))
115+
first = wiki_module.session_for("https://wiki.example.org/File:X")
116+
117+
set_credential(_credential("wiki.example.org", password="new"))
118+
second = wiki_module.session_for("https://wiki.example.org/File:X")
119+
120+
assert second != first
121+
122+
123+
def test_a_missing_credential_is_reported_with_the_host():
124+
with pytest.raises(ValueError, match=r"wiki\.example\.org"):
125+
wiki_module.session_for("https://wiki.example.org/File:X")
126+
127+
128+
# -- from_other ---------------------------------------------------------------
129+
130+
131+
def test_from_other_keeps_a_session_the_source_carries():
132+
sentinel = _a_session()
133+
source = WikiFileController(osw=sentinel, download_url="https://w.example/x")
134+
135+
target = WikiFileController.from_other(source)
136+
137+
assert target.osw.site is sentinel.site
138+
139+
140+
def test_from_other_takes_the_session_it_is_given():
141+
sentinel = _a_session()
142+
other = _a_session()
143+
source = WikiFileController(osw=other, download_url="https://w.example/x")
144+
145+
target = WikiFileController.from_other(source, osw=sentinel)
146+
147+
assert target.osw.site is sentinel.site
148+
assert target.osw.site is not other.site
149+
150+
151+
# -- the local controller -----------------------------------------------------
152+
153+
154+
def test_local_controller_needs_a_file():
155+
with pytest.raises(ValueError, match="needs a file"):
156+
LocalFileController()
157+
158+
159+
def test_local_controller_accepts_path_or_local_path(tmp_path):
160+
target = tmp_path / "a.bin"
161+
target.write_bytes(b"x")
162+
163+
assert LocalFileController(path=target).path == target
164+
assert LocalFileController(local_path=str(target)).path == target
165+
166+
167+
def test_local_controller_round_trips_a_file(tmp_path):
168+
controller = LocalFileController(path=tmp_path / "a.bin")
169+
controller.put(BytesIO(b"payload"))
170+
with controller.get() as handle:
171+
assert handle.read() == b"payload"

0 commit comments

Comments
 (0)