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

[py] refactor HtmlOnlyHandler in webserver.py to support JSON content #14705

Merged

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Nov 2, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

The current HtmlOnlyHandler does not takes into account various other content types like JSON and results into errors.

This PR add a new ExtendedHandlerthat can help with these types and also enable CORS for GET, POST methods.

Motivation

I was trying out FedCM implementation for python and cannot get the POST methods to work with the SimpleWebServer() due to CORS origin errors and JSON type not supported in the earlier HtmlOnlyHandler. The added ExtendedHandler will help with running the FedCM tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement


Description

  • Replaced the HtmlOnlyHandler with ExtendedHandler to handle various content types including JSON.
  • Implemented CORS support by adding appropriate headers for GET, POST, and OPTIONS methods.
  • Enhanced the do_GET method to dynamically serve HTML pages and support different file types based on extensions.
  • Rewrote the do_POST method for better clarity and efficiency, including improved handling of file uploads.

Changes walkthrough 📝

Relevant files
Enhancement
webserver.py
Enhance HTTP handler to support JSON and CORS                       

py/test/selenium/webdriver/common/webserver.py

  • Replaced HtmlOnlyHandler with ExtendedHandler to support more content
    types.
  • Added CORS support for GET, POST, and OPTIONS methods.
  • Enhanced do_GET method to serve JSON and other file types.
  • Rewrote do_POST method for improved clarity and efficiency.
  • +50/-43 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    to support various content types and CORS origin. also re-wrote the `do_POST` method for efficiency
    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 2, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Potential file exposure:
    The new implementation in the do_GET method allows serving any file type from the server's file system. This could potentially expose sensitive files if proper access controls are not in place. The code should be reviewed to ensure that only intended files and directories are accessible.

    ⚡ Recommended focus areas for review

    Error Handling
    The new implementation might not handle all error cases properly. For example, the file reading operation is not wrapped in a try-except block, which could lead to unhandled exceptions.

    Security Concern
    The new implementation allows serving any file type, which could potentially lead to serving sensitive files if not properly restricted.

    Performance Issue
    The new implementation reads the entire file content into memory before sending it. For large files, this could lead to high memory usage and potential out-of-memory errors.

    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 2, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check for the existence of required headers to prevent potential exceptions

    Consider adding a check for the existence of the 'content-length' header before
    attempting to access it. This can prevent potential KeyError exceptions if the
    header is missing.

    py/test/selenium/webdriver/common/webserver.py [112]

    -remaining_bytes = int(self.headers["content-length"])
    +content_length = self.headers.get("content-length")
    +if content_length is None:
    +    self.send_error(400, "Missing Content-Length header")
    +    return
    +remaining_bytes = int(content_length)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion to check for the existence of the 'content-length' header is valuable as it prevents potential KeyError exceptions, improving the robustness and reliability of the code when handling POST requests.

    8
    Enhancement
    Use a dictionary to map file extensions to content types for improved maintainability and scalability

    Consider using a dictionary to map file extensions to content types instead of using
    multiple if-else statements. This approach is more scalable and easier to maintain.

    py/test/selenium/webdriver/common/webserver.py [93-98]

    -if file_extension == ".json":
    -    content_type = "application/json"
    -elif file_extension == ".html":
    -    content_type = "text/html"
    -else:
    -    content_type = "application/octet-stream"
    +content_types = {
    +    ".json": "application/json",
    +    ".html": "text/html"
    +}
    +content_type = content_types.get(file_extension, "application/octet-stream")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use a dictionary for mapping file extensions to content types enhances code readability and maintainability. It simplifies the addition of new content types in the future, making the code more scalable.

    7

    💡 Need additional feedback ? start a PR chat

    @navin772 navin772 changed the title [py] move from HtmlOnlyHandler to ExtendedHandler [py] add a new ExtendedHandler in webserver.py Nov 2, 2024
    @AutomatedTester
    Copy link
    Member

    There seems to be quite a bit of code duplication between the new code and the existing code. Let's refactor it

    @navin772
    Copy link
    Contributor Author

    navin772 commented Nov 4, 2024

    @AutomatedTester Initially, I tried to modify the HtmlOnlyHandler so that it can work for other use cases, but some tests were failing (refer to the previous commit).

    What do you suggest? Having 2 handlers or just one?

    @navin772
    Copy link
    Contributor Author

    navin772 commented Nov 4, 2024

    Refactored the code into one class to handle JSON type

    @AutomatedTester AutomatedTester merged commit f9d3096 into SeleniumHQ:trunk Nov 4, 2024
    14 checks passed
    @navin772 navin772 deleted the webserver-extended-handler branch November 4, 2024 11:12
    @navin772 navin772 changed the title [py] add a new ExtendedHandler in webserver.py [py] refactor HtmlOnlyHandler in webserver.py to support JSON content Nov 4, 2024
    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.

    2 participants