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

[java] Fix v*Network.java conditions #14585

Merged
merged 3 commits into from
Oct 14, 2024
Merged

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Oct 10, 2024

User description

Description

As described in this comment, SpotBugs found some additional bugs in the code.
In this PR I fix part of the found problems.

SpotBugs error:

H D UC: Useless condition: it's known that code <= 399 (0x18f) at this point  At V85Network.java:[line 140]

In this PR I change the condition in the v*Network classes - the warning should appear only if there was a problem reading the body in a request other than a redirect.

Motivation and Context

Fixing the actual problems is necessary before enabling full SpotBugs analysis, in order to not break the build.

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

  • Fixed a logical error in the condition checking HTTP status codes in multiple v*Network classes.
  • Changed the condition from using 'and' to 'or' to correctly identify non-redirect HTTP responses.
  • This change addresses SpotBugs warnings and improves the reliability of the code.

Changes walkthrough 📝

Relevant files
Bug fix
v127Network.java
Correct HTTP status code condition in v127Network               

java/src/org/openqa/selenium/devtools/v127/v127Network.java

  • Fixed condition to correctly handle HTTP status codes.
  • Changed logical operator from 'and' to 'or' in condition.
  • +1/-1     
    v128Network.java
    Correct HTTP status code condition in v128Network               

    java/src/org/openqa/selenium/devtools/v128/v128Network.java

  • Fixed condition to correctly handle HTTP status codes.
  • Changed logical operator from 'and' to 'or' in condition.
  • +1/-1     
    v129Network.java
    Correct HTTP status code condition in v129Network               

    java/src/org/openqa/selenium/devtools/v129/v129Network.java

  • Fixed condition to correctly handle HTTP status codes.
  • Changed logical operator from 'and' to 'or' in condition.
  • +1/-1     
    V85Network.java
    Correct HTTP status code condition in V85Network                 

    java/src/org/openqa/selenium/devtools/v85/V85Network.java

  • Fixed condition to correctly handle HTTP status codes.
  • Changed logical operator from 'and' to 'or' in condition.
  • +1/-1     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Change
    The condition for logging warnings about unable to get body for request has been changed. Verify if this new condition correctly handles all scenarios, especially for HTTP status codes 300-399.

    Consistency
    Ensure that the changes made in this file are consistent with the changes in other similar files (v127, v129, v85) to maintain uniformity across different versions.

    Error Handling
    The new condition might affect error handling for certain HTTP status codes. Verify that this change doesn't introduce any unintended side effects in error reporting or handling.

    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 10, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Log exception details with the warning message for better debugging context

    Consider logging the exception details along with the warning message to provide
    more context for debugging.

    java/src/org/openqa/selenium/devtools/v127/v127Network.java [131]

    -LOG.warning("Unable to get body for request id " + pausedReq.getRequestId());
    +LOG.warning("Unable to get body for request id " + pausedReq.getRequestId(), e);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Logging exception details provides valuable context for debugging, making it easier to diagnose issues. This suggestion is practical and enhances the logging mechanism significantly.

    8
    Best practice
    Use constants for HTTP status code ranges to improve code readability and maintainability

    Consider using a constant for the HTTP status code range instead of hardcoding the
    values 300 and 399. This would improve readability and maintainability.

    java/src/org/openqa/selenium/devtools/v127/v127Network.java [130-132]

    -if (code < 300 || code > 399) {
    +if (code < HttpStatus.MULTIPLE_CHOICES || code > HttpStatus.PERMANENT_REDIRECT) {
       LOG.warning("Unable to get body for request id " + pausedReq.getRequestId());
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using constants for HTTP status codes enhances readability and maintainability by avoiding magic numbers. This suggestion is relevant and improves code quality without altering functionality.

    7

    💡 Need additional feedback ? start a PR chat

    @mk868 mk868 requested a review from joerg1985 October 11, 2024 10:52
    @pujagani pujagani merged commit 274f43f into SeleniumHQ:trunk Oct 14, 2024
    3 of 4 checks passed
    @mk868 mk868 deleted the fix-network-bugs branch October 14, 2024 15:38
    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.

    3 participants