Skip to content

feat: raise TM1pyNetworkException for HTML/Cloudflare responses#1415

Merged
nicolasbisurgi merged 4 commits into
cubewise-code:masterfrom
jamestwilkinson:fix/cloudflare-network-exception
Jun 10, 2026
Merged

feat: raise TM1pyNetworkException for HTML/Cloudflare responses#1415
nicolasbisurgi merged 4 commits into
cubewise-code:masterfrom
jamestwilkinson:fix/cloudflare-network-exception

Conversation

@jamestwilkinson

@jamestwilkinson jamestwilkinson commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

PR: Raise TM1pyNetworkException for HTML / Cloudflare responses

Summary
This PR adds a new exception type, TM1pyNetworkException, to handle network/WAF/Cloudflare HTML error pages that are not valid TM1 REST responses, and misleading as TM1 has not received any message. These responses previously caused TM1pyRestException to be raised with confusing error messages.

Problem
When TM1 is fronted by Cloudflare, load balancers, proxies, or WAFs, upstream failures often return HTML pages, not JSON.
RestService.verify_response attempted to parse these as TM1 REST errors, resulting in:

  • misleading exception messages
  • JSON decode errors
  • exceptions being caught incorrectly by except TM1pyRestException blocks

This behaviour is tracked in Issue #1393.

Solution

  1. Added new exception: TM1pyNetworkException
  2. Extracts Cloudflare Ray ID when present
  3. Provides clearer error messages for HTML responses
  4. Is not a subclass of TM1pyRestException

Updated RestService.verify_response:

  1. Detects HTML responses via Content-Type or signature
  2. Raises TM1pyNetworkException instead of TM1pyRestException

Added unit tests:

  1. Exception properties and formatting
  2. Ray ID extraction (colon, space, Cloudflare formats)
  3. Inheritance behaviour
  4. Integration test for verify_response raising the new exception

Impact

  • No breaking changes for existing TM1 REST workflows
  • Existing except TM1pyRestException blocks continue to behave correctly
  • HTML/WAF/Cloudflare failures now produce clear, actionable errors
  • Fully backwards‑compatible for environments not using Cloudflare

Fixes
Fixes #1393

@jamestwilkinson
feat: raise TM1pyNetworkException for HTML/Cloudflare responses
9f8940c 
- Added TM1pyNetworkException to handle upstream HTML/WAF/Cloudflare error pages
- Updated RestService.verify_response to detect non-JSON HTML responses
- Added comprehensive unit tests for exception behaviour and verify_response integration
nicolasbisurgi
nicolasbisurgi reviewed Jun 8, 2026
View reviewed changes
Comment thread TM1py/Services/RestService.py Outdated
@nicolasbisurgi

nicolasbisurgi commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Hi @jamestwilkinson - thanks for working on this, I agree that is much needed as we don't have dedicated network exceptions.
I've reviewed the code a bit and it looks good, but there are two main issues to address I think:

  1. TM1pyNetworkException isn't exported from TM1py/Exceptions/__init__.py
    Since people import these with from TM1py.Exceptions import …, the exception you just built can't be caught by anyone using the library. (Small related thing: RestService.py imports it from the deep …Exceptions.Exceptions path instead of the package, which is different from how the other services do it.) Just adding it to __init__.py and tidying that import sorts it.

  2. The retry loop swallows the new exception
    This one's sneaky. In the reconnect-and-retry block, the code catches TM1pyRestException and re-raises it, but anything else falls into a generic except Exception that just shrugs and retries. Since the new exception isn't a TM1pyRestException, a Cloudflare/HTML error during a retry slips into that generic catch, the retries run out, and the user ends up seeing the original disconnect error instead of the real reason — which is exactly the confusing behavior this PR set out to kill. Easy fix: have line 510 catch both exception types.

Please let me know your thoughts.

@jamestwilkinson
fix: address PR review feedback
1c35535 
- Merge exception imports onto single line using package path in RestService.py
- Add TM1pyNetworkException to retry loop alongside TM1pyRestException
- Add headers paramater to _extract_cloudflare_ray_id to first check CF-RAY header
@jamestwilkinson

jamestwilkinson commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Hi @nicolasbisurgi, thanks for the review. I've addressed the points raised and pushed the updated code. Thanks, James.

@nicolasbisurgi @claude
fix: address review nits + export regression on TM1pyNetworkException
8c65b90 
- Export TM1pyVersionDeprecationException from TM1py.Exceptions (the
  package-path import in RestService introduced in the prior commit
  referenced a name that was never exported, breaking `import TM1py`)
- Broaden HTML detection: case-insensitive, also match bare <html>
- Type hints: Optional[str] for _extract_cloudflare_ray_id and ray_id
- Document the intentional non-subclassing of TM1pyRestException
- Add regression tests: JSON error still raises TM1pyRestException,
  isolate each HTML detection branch, OK response raises nothing
- Cleanup: trailing whitespace, missing EOF newlines, import sorting

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nicolasbisurgi

nicolasbisurgi commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks @jamestwilkinson !
I did one last check when running the tests and one small issue popped up: when switching RestService.py to import from the TM1py.Exceptions package pulled in TM1pyVersionDeprecationException, which was never exported in __init__.py. That broke import TM1py entirely on the branch (ImportError on import). I exported the missing name to fix it.

The rest is all tidy-up coming from ruff and other linter:

  • Broadened HTML detection — now case-insensitive and also matches a bare <html>, not just uppercase <!DOCTYPE
  • Optional[str] type hints on _extract_cloudflare_ray_id and ray_id
  • Added a docstring note that not subclassing TM1pyRestException is intentional
  • Added regression tests: a genuine JSON error still raises TM1pyRestException, each HTML-detection branch tested in isolation, and an OK response raises nothing
  • Whitespace / EOF newline / import-sort cleanup

All 20 tests pass and ruff check . is clean. Thanks for the quick turnaround on the earlier feedback — the CF-RAY header check was a nice touch.

Let me know if you are ok with these changes and then we can merge it.

@nicolasbisurgi nicolasbisurgi added the release:patch Triggers patch version bump (e.g.: 1.4.9 → 1.4.10) label Jun 9, 2026
@jamestwilkinson

jamestwilkinson commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @nicolasbisurgi, please go ahead by all means. Kind regards, James.

@nicolasbisurgi @claude
style: apply black formatting (line-length 120)
e718648 
Collapse multi-line exception calls and fix blank-line/comment
whitespace to satisfy the PR-validation black --check step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nicolasbisurgi nicolasbisurgi merged commit be1f210 into cubewise-code:master Jun 10, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolasbisurgi nicolasbisurgi nicolasbisurgi left review comments

@MariusWirtz MariusWirtz MariusWirtz approved these changes

Assignees

No one assigned

Labels

release:patch Triggers patch version bump (e.g.: 1.4.9 → 1.4.10)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Raise TM1pyNetworkException for HTML / Cloudflare responses instead of TM1pyRestException

3 participants

@jamestwilkinson @nicolasbisurgi @MariusWirtz