Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Enhance URLResolverMiddleware to support host-based path resolution #1193

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

abdullai-t
Copy link
Contributor

@abdullai-t abdullai-t commented Dec 5, 2024

Summary / Highlights

This pull request introduces significant changes to the src/api/middlewares/url_resolver_middleware.py file, focusing on improving URL resolution and handling of host-specific paths. The key changes include the addition of a host map and a new method for resolving paths.

Enhancements to URL resolution:

  • Added import for host_patterns from _main_.hosts and initialized a host_map to map host regex patterns to host objects.
  • Replaced the direct call to resolve with a new method _resolve_path that constructs host-specific paths and resolves them.
  • Updated the __call__ method to use _resolve_path for resolving request paths and set request.path_info accordingly.

New method for path resolution:

  • Introduced _resolve_path method to handle host-specific path resolution and return the resolved path or None if the path cannot be resolved.

Details (Give details about what this PR accomplishes, include any screenshots etc)

Testing Steps (Provide details on how your changes can be tested)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've tested my changes manually.
  • I've added unit tests to my PR.
Transparency (Project board)
  • I've given my PR a meaningful title.
  • I linked this PR to the relevant issue.
  • I moved the linked issue from the "Sprint backlog" to "In progress" when I started this.
  • I moved the linked issue to "QA Verification" now that it is ready to merge.

@abdullai-t abdullai-t changed the title feat: Enhance URLResolverMiddleware to support host-based path resolution Fix: Enhance URLResolverMiddleware to support host-based path resolution Dec 5, 2024
@abdullai-t abdullai-t self-assigned this Dec 5, 2024
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.71%. Comparing base (181b65b) to head (e45a2e0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
+ Coverage   58.66%   58.71%   +0.05%     
==========================================
  Files         498      499       +1     
  Lines       37130    37176      +46     
==========================================
+ Hits        21781    21829      +48     
+ Misses      15349    15347       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

if request.path == '/':
resolved_path = f'/{current_host}'
else:
resolved_path = f'/{current_host}{request.path}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add tests for this ? @abdullai-t Would request.path have a / preceding it or would current_host have a trailing slash / ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Collaborator

@HeyMrQuaidoo HeyMrQuaidoo left a comment

Choose a reason for hiding this comment

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

LGTM

@abdullai-t abdullai-t merged commit e78ba46 into main Dec 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants