Remove references to urllib.request and replace with requests library refs #10759

johnnymck · 2025-05-12T12:14:34Z

updated testing framework head of new behaviour
replaced urllib with requests; behaviour remains same
Replaced urllib references with reuqests; behaviour is the same
replaced refs to urllib with requests library - worried about lack of test coverage
Replaced urllib refs in solr_updater, requests has builtin json parser
Replaced references to urllib with requests, removed cookielib as functionality handled by Session object
removed uncaught commented-out reference :S Sorry!
Updated missed docstring changes in test_obfi.py
Replaced refs to urllib with requests lib, removed cookielib as functionality supported inside request Session() object

This PR refactors references to urllib.request (as per this discussion point) and replaces them with up-to-date functionality from the requests library.

Also worth noting that this is my first contribution to this project, and my second-ever FOSS code contribution. Please be kind! If I've done anything silly or unhandy, please don't hesitate to mention, just note it was done out of ignorance rather than laziness or lack of skill.

Technical

PR Also removes some references to no-longer needed lib import cookielib, as this functionality is also handled by Session object in requests.

Testing

Some functionality updated is not covered by tests, for example, adapter.py. Any advice on testing this/adding coverage is appreciated!

Screenshot

Stakeholders

@RayBB @mekarpeles

… into refactor/remove-urllib2-requests

… test coverage

…ctionality handled by Session object

…ionality supported inside request Session() object

for more information, see https://pre-commit.ci

…/johnnymck/openlibrary into refactor/remove-urllib2-requests

RayBB

Hi @johnnymck welcome and thanks for making your contribution!

The tricky part of this issue is testing. Such as, making the adapter.py file.
After making these changes did you try starting your local instance, logging in, and using a few basic functions of the site? That's a good first check to do.

I left you to short questions on the code itself.

Finally, before we can review this please make sure to fix the mypy errors visible here. You should be able to reproduce these locally when you setup pre-commit as is mentioned in our readme. That way you don't have to push up and wait for the run to see if it's fixed.

Good luck with the changes and thanks for opening the PR!

RayBB · 2025-05-13T23:52:34Z

scripts/tests/test_obfi.py


        def __enter__(self):
            return self

        def __exit__(self, *args):
            pass

-    return MockRead()
+    return MockGet()


Why does this now return MockGet() instead of MockRead()

A better name for this is likely MockResponse()

scripts/obfi/hide.py

Copilot

Pull Request Overview

This PR refactors the code to replace outdated urllib references with the requests library, streamlining HTTP calls and simplifying cookie management. Key changes include:

Removing urllib and cookielib dependencies and adding requests across multiple modules.
Updating HTTP request and response handling in both production and test code.
Adjusting seed fetching in obfi modules to work with requests.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
scripts/tests/test_obfi.py	Changed mocks and monkeypatches to use requests.get instead of urllib.request.urlopen.
scripts/solr_updater.py	Updated URL fetching and JSON parsing to use requests.get and response.json().
scripts/obfi/mktable.py	Replaced urllib.request.urlopen with requests.get and adjusted exception handling.
scripts/obfi/hide.py	Updated seed retrieval to use requests.get instead of urllib.request.urlopen.
openlibrary/plugins/upstream/adapter.py	Converted HTTP request handling from urllib to requests; updated parameter encoding with requests utilities.
openlibrary/plugins/openlibrary/tests/test_ratingsapi.py	Modified tests to use a requests.Session and adjusted login data encoding.
openlibrary/plugins/openlibrary/tests/test_listapi.py	Replaced urllib and cookielib with requests and updated POST data handling.

Copilot · 2025-05-14T16:42:14Z

openlibrary/plugins/upstream/adapter.py

@@ -263,7 +263,7 @@ def before_request(self):
            q = json.loads(i['query'])
            q = convert_dict(q)
            i['query'] = json.dumps(q)
-            self.data = urllib.parse.urlencode(i)
+            self.data = requests.utils.requote_uri(i)


Replacing urllib.parse.urlencode with requests.utils.requote_uri on a dictionary 'i' may lead to improper URL encoding. Consider using urllib.parse.urlencode or another proper encoding method to ensure correct form-encoding of parameters.

Copilot · 2025-05-14T16:42:15Z

openlibrary/plugins/upstream/adapter.py

@@ -272,7 +272,7 @@ def before_request(self):
        if 'keys' in i:
            keys = [convert_key(k) for k in json.loads(i['keys'])]
            i['keys'] = json.dumps(keys)
-            self.data = urllib.parse.urlencode(i)
+            self.data = requests.utils.requote_uri(i)


Using requests.utils.requote_uri on a dictionary might not perform the intended URL encoding—unlike urllib.parse.urlencode which transforms a dict into a URL-encoded string. Revert to a proper form encoding function or utilize requests’ built-in handling for form data.

openlibrary/plugins/openlibrary/tests/test_ratingsapi.py

openlibrary/plugins/openlibrary/tests/test_listapi.py

cdrini

Awesome, thank you for tackling this one! This looks super close, please let us know if you hit any issues!

cdrini · 2025-05-14T16:44:01Z

openlibrary/plugins/openlibrary/tests/test_listapi.py

-
-        self.opener = urllib.request.build_opener()
-        self.opener.add_handler(urllib.request.HTTPCookieProcessor(self.cookiejar))
+        self.sesh = requests.Session()  # Session object handles cookies automatically


We generally call this session instead of sesh.

Suggested change

self.sesh = requests.Session() # Session object handles cookies automatically

self.session = requests.Session() # Session object handles cookies automatically

cdrini · 2025-05-14T16:50:44Z

openlibrary/plugins/upstream/adapter.py

+        prepped = req.prepare()
+        resp = s.send(prepped)


Ray looked into prepare and it seems like we shouldn't need it; it seems like it is primarily used for caching an http request that we might need to send multiple times, to avoid having to prepare it every time. But in this case, we don't save it or anything so it serves no purpose.

Suggested change

prepped = req.prepare()

resp = s.send(prepped)

resp = s.send(req)

cdrini · 2025-05-14T16:51:12Z

openlibrary/plugins/openlibrary/tests/test_listapi.py

+        prepped = req.prepare()
+        return self.sesh.send(prepped)


Ditto here.

Suggested change

prepped = req.prepare()

return self.sesh.send(prepped)

return self.sesh.send(req)

cdrini · 2025-05-14T16:51:30Z

openlibrary/plugins/openlibrary/tests/test_ratingsapi.py

+        prepped = req.prepare()
+        return self.sesh.send(prepped)


Suggested change

prepped = req.prepare()

return self.sesh.send(prepped)

return self.sesh.send(req)

cdrini · 2025-05-14T17:20:34Z

scripts/obfi/hide.py

-                content = handle.read()
+            r = requests.get(SEED_PATH)
+            r.raise_for_status()
+            content = r.text


This needs to be bytes

Suggested change

content = r.text

content = r.content

cdrini · 2025-05-14T17:21:24Z

openlibrary/plugins/upstream/adapter.py

+        self.status_code = resp.status_code
+        # a one-liner that replaces the __str__ functionality of the HTTPResponse object replaced from urllib
+        self.status_msg = "\n".join(f"{k}: {v}" for k, v in resp.headers.items())
+        self.output = resp.read()


This method doesn't exist on the requests Response; it should be

Suggested change

self.output = resp.read()

self.output = resp.content

cdrini · 2025-05-14T17:22:58Z

scripts/tests/test_obfi.py

-        def read(self):
-            return b"seed=1234"
+    class MockGet:
+        text = b"seed=1234"


Since it's bytes, this should now be content

Suggested change

text = b"seed=1234"

content = b"seed=1234"

cdrini · 2025-05-14T17:24:35Z

scripts/tests/test_obfi.py


        def __enter__(self):
            return self

        def __exit__(self, *args):
            pass

-    return MockRead()
+    return MockGet()


A better name for this is likely MockResponse()

… :S)

…bute

johnnymck · 2025-05-30T12:05:41Z

@cdrini I've since made the changes you requested. Can you have another review on this when you have time? 😄

Co-authored-by: Copilot <[email protected]>

for more information, see https://pre-commit.ci

RayBB

I think it's looking really. Here are three small changes I'll commit. Then we can get @cdrini to look

openlibrary/plugins/openlibrary/tests/test_listapi.py

openlibrary/plugins/openlibrary/tests/test_ratingsapi.py

openlibrary/plugins/upstream/adapter.py

Co-authored-by: Copilot <[email protected]>

for more information, see https://pre-commit.ci

cdrini

Awesome work @johnnymck and @RayBB ! Everything looks 100%!

cclauss · 2025-06-04T17:37:59Z

Time to modernize to
https://www.python-httpx.org ?

johnnymck · 2025-06-09T11:52:23Z

Excellent! Thanks so much guys - glad to have helped, even in some small way. I'll have to pick more tasks in future.

johnnymck added 10 commits May 8, 2025 21:07

updated testing framework head of new behaviour

1aba571

replaced urllib with requests; behaviour remains same

44e3296

Replaced urllib references with reuqests; behaviour is the same

768b2e7

Merge branch 'master' of https://github.com/internetarchive/openlibrary…

6f2b6c2

… into refactor/remove-urllib2-requests

replaced refs to urllib with requests library - worried about lack of…

a90b35a

… test coverage

Replaced urllib refs in solr_updater, requests has builtin json parser

53b7a10

Replaced references to urllib with requests, removed cookielib as fun…

fa28c06

…ctionality handled by Session object

removed uncaught commented-out reference :S Sorry!

089867d

Updated missed docstring changes in test_obfi.py

6162ed5

Replaced refs to urllib with requests lib, removed cookielib as funct…

5cc95a8

…ionality supported inside request Session() object

github-actions bot assigned RayBB May 12, 2025

github-project-automation bot added this to Ray's Project May 12, 2025

github-project-automation bot moved this to Waiting Review/Merge from Staff in Ray's Project May 12, 2025

pre-commit-ci bot and others added 3 commits May 12, 2025 12:17

[pre-commit.ci] auto fixes from pre-commit.com hooks

6e29cc6

for more information, see https://pre-commit.ci

Whoops! Missed a dangling response ref from previous code

3febded

Merge branch 'refactor/remove-urllib2-requests' of https://github.com…

d046ea4

…/johnnymck/openlibrary into refactor/remove-urllib2-requests

RayBB requested changes May 14, 2025

View reviewed changes

cdrini requested a review from Copilot May 14, 2025 16:40

Copilot AI reviewed May 14, 2025

View reviewed changes

cdrini requested changes May 14, 2025

View reviewed changes

RayBB moved this from Waiting Review/Merge from Staff to Someone else is working on it in Ray's Project May 14, 2025

johnnymck added 6 commits May 23, 2025 15:44

mktable content as bytes instead of string as intended

9dc9189

hide content as bytes - as intended, instead of text (as per my error…

7ee3b92

… :S)

Updated session references and preparations in test_listapi

0f1a29b

updated references to session and request preparation

2b34c6d

updated MockGet name to MockResponse and attribute name to content

6dc662a

removed prepared request and updated response read() to content attri…

cc068f1

…bute

github-actions bot added the Needs: Response Issues which require feedback from lead label May 31, 2025

Merge branch 'master' into refactor/remove-urllib2-requests

9ee8abf

RayBB requested a review from cdrini June 3, 2025 02:31

RayBB moved this from Someone else is working on it to Waiting Review/Merge from Staff in Ray's Project Jun 3, 2025

RayBB and others added 2 commits June 2, 2025 19:36

Update openlibrary/plugins/openlibrary/tests/test_ratingsapi.py

27599c5

Co-authored-by: Copilot <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

2342e17

for more information, see https://pre-commit.ci

RayBB approved these changes Jun 3, 2025

View reviewed changes

openlibrary/plugins/openlibrary/tests/test_listapi.py Outdated Show resolved Hide resolved

openlibrary/plugins/openlibrary/tests/test_ratingsapi.py Outdated Show resolved Hide resolved

openlibrary/plugins/upstream/adapter.py Outdated Show resolved Hide resolved

RayBB and others added 5 commits June 2, 2025 19:40

Update openlibrary/plugins/openlibrary/tests/test_listapi.py

315880f

Update openlibrary/plugins/openlibrary/tests/test_ratingsapi.py

e995f30

Update openlibrary/plugins/upstream/adapter.py

f2b84de

Apply suggestions from code review

a72de9e

Co-authored-by: Copilot <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

de92364

for more information, see https://pre-commit.ci

RayBB mentioned this pull request Jun 3, 2025

Remove unused adapter.py web app #10774

Merged

RayBB added 3 commits June 4, 2025 06:31

Delete openlibrary/plugins/upstream/adapter.py

c806e27

restore adapter.py from master

ef0890b

Merge branch 'master' into refactor/remove-urllib2-requests

dae8fcb

cdrini approved these changes Jun 4, 2025

View reviewed changes

cdrini merged commit 08ede86 into internetarchive:master Jun 4, 2025
4 checks passed

github-project-automation bot moved this from Waiting Review/Merge from Staff to Done in Ray's Project Jun 4, 2025

johnnymck deleted the refactor/remove-urllib2-requests branch June 9, 2025 11:45

	self.sesh = requests.Session() # Session object handles cookies automatically
	self.session = requests.Session() # Session object handles cookies automatically

	prepped = req.prepare()
	resp = s.send(prepped)
	resp = s.send(req)

	prepped = req.prepare()
	return self.sesh.send(prepped)
	return self.sesh.send(req)

Uh oh!

Remove references to urllib.request and replace with requests library refs #10759

Remove references to urllib.request and replace with requests library refs #10759

Uh oh!

Conversation

johnnymck commented May 12, 2025

Technical

Testing

Screenshot

Stakeholders

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cdrini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnymck commented May 30, 2025

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdrini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cclauss commented Jun 4, 2025

Uh oh!

johnnymck commented Jun 9, 2025

Uh oh!

Uh oh!