Skip to content

[java]: fix decoding of line endings #14539

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Oct 7, 2024
Merged

[java]: fix decoding of line endings #14539

merged 5 commits into from
Oct 7, 2024

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Sep 26, 2024 •
edited by qodo-merge-pro bot
Loading

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

Fixes #13207

Motivation and Context

It removes old code which changed the line endings from \r\n to \n because it is not needed anymore and also breaks encoding-decoding. @titusfortner said removing this shouldn't impact anything and this code is also useless because other bindings don't do it.

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

Bug fix

Description

  • Removed the code that changed line endings from \r\n to \n in the AbstractHttpResponseCodec class, as it was unnecessary and caused encoding-decoding issues.
  • Ensured that the response value is set directly without altering line endings, aligning with other bindings.

Changes walkthrough 📝

Relevant files
Bug fix
AbstractHttpResponseCodec.java
Remove unnecessary line ending normalization in response decoding

java/src/org/openqa/selenium/remote/codec/AbstractHttpResponseCodec.java

  • Removed code that normalized line endings from \r\n to \n.
  • Ensured that the response value is set without altering line endings.
    		•  +0/-6     

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

    @qodo-merge-pro qodo-merge-pro bot added P-bug fix PR addresses a known issue Review effort [1-5]: 2 labels Sep 26, 2024
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Compatibility Issue
    Removing line ending normalization might affect systems expecting normalized line endings. Ensure this change doesn't break existing functionality across different platforms.

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 26, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Add a comment to explain the removal of line ending normalization

    Consider adding a comment explaining why the line ending normalization was removed.
    This will help future maintainers understand the reasoning behind this change.

    java/src/org/openqa/selenium/remote/codec/AbstractHttpResponseCodec.java [105-107]

     if (!content.isEmpty()) {
+  // Line ending normalization removed to preserve original content
   response.setValue(content);
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a comment to explain the removal of line ending normalization can improve maintainability by providing context for future developers, though it is not crucial for functionality.

    		7

    💡 Need additional feedback ? start a PR chat

    pujagani
    pujagani approved these changes Oct 7, 2024
    View reviewed changes
    Copy link
    Contributor

    @pujagani pujagani left a comment

    Choose a reason for hiding this comment

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

    Based on the discussion done in the issue, it looks good to me. Thank you @Delta456!

    @pujagani pujagani merged commit a12b597 into SeleniumHQ:trunk Oct 7, 2024
    26 of 30 checks passed
    @Delta456 Delta456 deleted the remove_line_ending_manipulation branch November 6, 2024 10:30
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @Delta456 @harsha509
    @pujagani @M1troll
    [java]: fix decoding of line endings (SeleniumHQ#14539)
    5a82a2b 
    Co-authored-by: Sri Harsha <12621691+harsha509@users.noreply.github.com>
Co-authored-by: Puja Jagani <puja.jagani93@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @pujagani pujagani pujagani approved these changes

    Assignees
    No one assigned
    Labels
    P-bug fix PR addresses a known issue Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Return wrong value when using JavaScriptExecutor to execute decoding js function "atob"
    3 participants
    @Delta456 @pujagani @harsha509