Skip to content

Comments

fix: Add protocol validation to KiotaRequestAdapterHook#61103

Merged
dabla merged 7 commits intoapache:mainfrom
Fury0508:fix/powerbi-protocol-issue-61081
Jan 29, 2026
Merged

fix: Add protocol validation to KiotaRequestAdapterHook#61103
dabla merged 7 commits intoapache:mainfrom
Fury0508:fix/powerbi-protocol-issue-61081

Conversation

@Fury0508
Copy link
Contributor

Description

Fixes protocol validation bug in KiotaRequestAdapterHook that caused PowerBIDatasetRefreshOperator to fail with httpx.UnsupportedProtocol: Request URL is missing an 'http://' or 'https://' protocol error.

Root Cause

The hook had two bugs introduced in PR #45006:

  1. Logic bug in get_host() method (line 238): Used or instead of tuple check
   # BROKEN - always evaluates to True for URLs with https://
   if not self.host.startswith("http://") or not self.host.startswith("https://"):
       return f"{connection.schema}://{self.host}"
   # Result: "https://https://api.powerbi.com" ❌
  1. Missing protocol validation: __init__ set self.host directly without ensuring protocol prefix

Changes

  • Added _ensure_protocol() method to validate and add protocol prefix when missing
  • Fixed logic bug in get_host() - changed or to proper tuple check with startswith(("http://", "https://"))
  • Added safety fallback (schema or "https") when connection.schema is None
  • Added comprehensive test class TestKiotaRequestAdapterHookProtocol with 10 test cases covering all protocol scenarios

Testing

closes: #61081


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude (Anthropic) following the guidelines

AI Assistance Details:

  • Used to understand root cause location by analyzing the codebase and error trace
  • Used to write comprehensive test code (TestKiotaRequestAdapterHookProtocol class with 10 test cases)
  • Used to suggest fail-safe options
  • Final implementation and code review done by human contributor

Checklist:

  • Code changes are covered with tests
  • All tests pass locally
  • Pre-commit checks pass (prek run --all-files)
  • Follows commit message guidelines

Fixes apache#61081

PowerBIDatasetRefreshOperator was failing with 'Request URL missing protocol'
error when URLs were configured without https:// prefix.

Changes:
- Add _ensure_protocol() method to validate and fix URLs
- Fix logic bug in get_host() (changed 'or' to proper tuple check)
- Add safety fallback when connection.schema is None
- Add comprehensive tests for protocol handling

This ensures URLs always have proper protocol prefix while maintaining
backward compatibility.
Copy link
Contributor

@dabla dabla left a comment

Choose a reason for hiding this comment

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

Good catch, thanks for the PR, some small changes and we're good.

- Remove redundant if host else None check in __init__
- Use _ensure_protocol in get_host to stay DRY
- Ensure _ensure_protocol returns None for empty strings

All tests passing.
@Fury0508
Copy link
Contributor Author

All feedback addressed! Thanks @henry3260 , @dabla

Changes made:

  • Removed redundant if host else None check in __init__ (DRY principle)
  • Updated get_host() to use _ensure_protocol(self.host, schema) instead of duplicating logic
  • Fixed _ensure_protocol to return None for empty strings (not "")

All 10 tests passing. Ready for review!

Fury0508 and others added 4 commits January 27, 2026 07:56
- Changed _ensure_protocol parameter to accept str | None
- Changed get_host return type to str (never returns None)
- Used cast() for type narrowing where needed
@Fury0508
Copy link
Contributor Author

Fury0508 commented Jan 27, 2026

@henry3260, @dabla Thanks for the review! I've addressed your feedback:

  • Updated _ensure_protocol parameter to accept str | None
  • Changed get_host return type to str (never returns None due to fallback logic)
  • Used cast(str, ...) for type narrowing where needed

All tests pass and mypy checks pass now. Ready for another look when you have time!

@Fury0508
Copy link
Contributor Author

Hi @dabla ,
Thanks for pointing that out! I’ve refactored get_host() to delegate protocol handling entirely to _ensure_protocol(), removing the duplicate startswith check and keeping things DRY. Changes are pushed 👍

Copy link
Contributor

@dabla dabla left a comment

Choose a reason for hiding this comment

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

Thx @Fury0508, nice work!

@dabla dabla merged commit 0e920ae into apache:main Jan 29, 2026
89 checks passed
morelgeorge pushed a commit to morelgeorge/airflow that referenced this pull request Feb 1, 2026
* fix: Add protocol validation to KiotaRequestAdapterHook

Fixes apache#61081

PowerBIDatasetRefreshOperator was failing with 'Request URL missing protocol'
error when URLs were configured without https:// prefix.

Changes:
- Add _ensure_protocol() method to validate and fix URLs
- Fix logic bug in get_host() (changed 'or' to proper tuple check)
- Add safety fallback when connection.schema is None
- Add comprehensive tests for protocol handling

This ensures URLs always have proper protocol prefix while maintaining
backward compatibility.

* refactor: Address review feedback

- Remove redundant if host else None check in __init__
- Use _ensure_protocol in get_host to stay DRY
- Ensure _ensure_protocol returns None for empty strings

All tests passing.

* fix: Update _ensure_protocol return type to include None

* fix: Update type hints to properly handle None values

* fix: Update type hints to properly handle None values

- Changed _ensure_protocol parameter to accept str | None
- Changed get_host return type to str (never returns None)
- Used cast() for type narrowing where needed

* made the code DRY
shashbha14 pushed a commit to shashbha14/airflow that referenced this pull request Feb 2, 2026
* fix: Add protocol validation to KiotaRequestAdapterHook

Fixes apache#61081

PowerBIDatasetRefreshOperator was failing with 'Request URL missing protocol'
error when URLs were configured without https:// prefix.

Changes:
- Add _ensure_protocol() method to validate and fix URLs
- Fix logic bug in get_host() (changed 'or' to proper tuple check)
- Add safety fallback when connection.schema is None
- Add comprehensive tests for protocol handling

This ensures URLs always have proper protocol prefix while maintaining
backward compatibility.

* refactor: Address review feedback

- Remove redundant if host else None check in __init__
- Use _ensure_protocol in get_host to stay DRY
- Ensure _ensure_protocol returns None for empty strings

All tests passing.

* fix: Update _ensure_protocol return type to include None

* fix: Update type hints to properly handle None values

* fix: Update type hints to properly handle None values

- Changed _ensure_protocol parameter to accept str | None
- Changed get_host return type to str (never returns None)
- Used cast() for type narrowing where needed

* made the code DRY
jason810496 pushed a commit to abhijeets25012-tech/airflow that referenced this pull request Feb 3, 2026
* fix: Add protocol validation to KiotaRequestAdapterHook

Fixes apache#61081

PowerBIDatasetRefreshOperator was failing with 'Request URL missing protocol'
error when URLs were configured without https:// prefix.

Changes:
- Add _ensure_protocol() method to validate and fix URLs
- Fix logic bug in get_host() (changed 'or' to proper tuple check)
- Add safety fallback when connection.schema is None
- Add comprehensive tests for protocol handling

This ensures URLs always have proper protocol prefix while maintaining
backward compatibility.

* refactor: Address review feedback

- Remove redundant if host else None check in __init__
- Use _ensure_protocol in get_host to stay DRY
- Ensure _ensure_protocol returns None for empty strings

All tests passing.

* fix: Update _ensure_protocol return type to include None

* fix: Update type hints to properly handle None values

* fix: Update type hints to properly handle None values

- Changed _ensure_protocol parameter to accept str | None
- Changed get_host return type to str (never returns None)
- Used cast() for type narrowing where needed

* made the code DRY
jhgoebbert pushed a commit to jhgoebbert/airflow_Owen-CH-Leung that referenced this pull request Feb 8, 2026
* fix: Add protocol validation to KiotaRequestAdapterHook

Fixes apache#61081

PowerBIDatasetRefreshOperator was failing with 'Request URL missing protocol'
error when URLs were configured without https:// prefix.

Changes:
- Add _ensure_protocol() method to validate and fix URLs
- Fix logic bug in get_host() (changed 'or' to proper tuple check)
- Add safety fallback when connection.schema is None
- Add comprehensive tests for protocol handling

This ensures URLs always have proper protocol prefix while maintaining
backward compatibility.

* refactor: Address review feedback

- Remove redundant if host else None check in __init__
- Use _ensure_protocol in get_host to stay DRY
- Ensure _ensure_protocol returns None for empty strings

All tests passing.

* fix: Update _ensure_protocol return type to include None

* fix: Update type hints to properly handle None values

* fix: Update type hints to properly handle None values

- Changed _ensure_protocol parameter to accept str | None
- Changed get_host return type to str (never returns None)
- Used cast() for type narrowing where needed

* made the code DRY
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to airflow 3.1.6 breaks PowerBIDatasetRefreshOperator

3 participants