Fix invalid URL handling in PyJWKClient.fetch_data()

[Kontrolix]

Problem

When PyJWKClient is initialized with an invalid URL string and fetch_data() is called, urllib.request.Request raises a ValueError, which is not caught or handled gracefully.

Solution

• Isolated URL validation into a separate try-except block to explicitly catch ValueError when constructing the request
• Converts the ValueError to PyJWKClientError with a descriptive message that includes the invalid URL and the original error
• Prevents None from being cached on invalid URLs (by returning early)

Testing

• Added test_fetch_data_invalid_url() to verify that invalid URLs raise PyJWKClientError with the appropriate error message

Benefits

• Clearer error handling for invalid URL configurations
• Consistent error types across the API (all errors raised as PyJWKClientError)
• Better debugging experience with detailed error messages

[Copilot]

Pull request overview

This PR adds explicit handling for invalid URLs in PyJWKClient.fetch_data() by catching ValueError exceptions from urllib.request.Request and converting them to PyJWKClientError with descriptive error messages. A test case is added to verify this behavior.

Changes:

• Added ValueError exception handling in fetch_data() to catch invalid URL errors from urllib.request.Request
• Added test case test_fetch_data_invalide_uri() to verify invalid URL handling raises PyJWKClientError

Reviewed changes

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

File	Description
jwt/jwks_client.py	Added try-except block to catch ValueError when constructing urllib Request object and convert it to PyJWKClientError
tests/test_jwks_client.py	Added test case to verify invalid URLs raise PyJWKClientError with appropriate error message

Comments suppressed due to low confidence (1)

jwt/jwks_client.py:73

• The ValueError handling does not prevent None from being cached as claimed in the PR description. When a ValueError is raised, the finally block (lines 71-73) still executes before the exception propagates, which will cache jwk_set=None since it was initialized to None on line 54. To fix this, the finally block should only cache when jwk_set is not None, or the ValueError should be caught before entering the try-finally structure. Consider moving the ValueError handling outside the try-finally block or adding a check in the finally block like: if self.jwk_set_cache is not None and jwk_set is not None: self.jwk_set_cache.put(jwk_set).

        try:
            r = urllib.request.Request(url=self.uri, headers=self.headers)
        except ValueError as e:
            raise PyJWKClientError(f'Invalid URL "{self.uri}", err: "{e}"') from e

        try:
            with urllib.request.urlopen(
                r, timeout=self.timeout, context=self.ssl_context
            ) as response:
                jwk_set = json.load(response)
        except (URLError, TimeoutError) as e:
            raise PyJWKClientConnectionError(
                f'Fail to fetch data from the url, err: "{e}"'
            ) from e
        else:
            return jwk_set
        finally:
            if self.jwk_set_cache is not None:
                self.jwk_set_cache.put(jwk_set)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The test does not verify the caching behavior mentioned in the PR description. Since the PR claims to "prevent None from being cached on invalid URLs", the test should verify that the cache remains empty or unchanged after an invalid URL error. Consider adding an assertion like: assert jwks_client.jwk_set_cache.get() is None (if cache is enabled by default) or verify that attempting to use the client after the error doesn't return cached None values.

[Kontrolix]

I don't think that this test is needed. The fact that the cache is not updated to None is more of a (happy?) side effect than the real intent of this code.

[auvipy]

please fix the merge conflict