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 SpotBugs bugs in manager #14608

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Oct 16, 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 errors:

M D SF: Switch statement found in org.openqa.selenium.manager.SeleniumManagerOutput$Log.fromJson(JsonInput) where default case is missing  At SeleniumManagerOutput.java:[lines 76-99]
M D NP: Possible null pointer dereference in org.openqa.selenium.manager.SeleniumManager.getBinary() due to return value of called method  Dereferenced at SeleniumManager.java:[line 212]
M B RV: Exceptional return value of java.io.File.mkdirs() ignored in org.openqa.selenium.manager.SeleniumManager.getBinary()  At SeleniumManager.java:[line 212]

Solutions:

  • Add the missing default branch to the switch to skip reading JSON value, this practice is common elsewhere: BiDiSessionStatus#fromJson(JsonInput), ConsoleLogEntry#fromJson(JsonInput), ...
  • Add exclusion for NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE to ignore potential null value from the binary.getParent() call - SpotBugs exclusion for getParent call is common, for example for org.openqa.selenium.firefox.FirefoxBinary
  • Replace path.toFile().mkdirs() with Files.createDirectories(path) - use of Files.createDirectories is common in the code base, so it is consistent

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

  • Improved error handling in SeleniumManager by replacing mkdirs() with Files.createDirectories().
  • Added a default case to the switch statement in SeleniumManagerOutput.fromJson to handle unexpected JSON values.
  • Updated SpotBugs exclusions to ignore potential null pointer dereference warnings in SeleniumManager.

Changes walkthrough 📝

Relevant files
Bug fix
SeleniumManager.java
Improve directory creation error handling in SeleniumManager

java/src/org/openqa/selenium/manager/SeleniumManager.java

  • Replaced mkdirs() with Files.createDirectories() for better error
    handling.
  • Ensured directories are created before copying files.
  • +1/-1     
    SeleniumManagerOutput.java
    Add default case to switch statement in fromJson                 

    java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java

  • Added a default case to the switch statement in fromJson.
  • Ensured JSON values are skipped if not explicitly handled.
  • +4/-0     
    Configuration changes
    spotbugs-excludes.xml
    Update SpotBugs exclusions for SeleniumManager                     

    java/spotbugs-excludes.xml

  • Added exclusion for NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE.
  • Updated SpotBugs filter to ignore specific null pointer warnings.
  • +5/-0     

    💡 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

    Error Handling
    Verify that the error handling for Files.createDirectories() is sufficient. Consider adding specific exception handling or logging for directory creation failures.

    Switch Statement
    Ensure that the new default case in the switch statement doesn't hide any potential issues. Verify if any important cases are missing or if the default behavior is appropriate for all unhandled cases.

    SpotBugs Exclusion
    Validate if excluding NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE for SeleniumManager is the best approach. Consider if there's a way to handle the potential null value without suppressing the warning.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a null check for the input stream to prevent potential NullPointerException

    Consider adding a null check for inputStream before attempting to copy it to avoid
    potential NullPointerException.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [211-214]

     try (InputStream inputStream = this.getClass().getResourceAsStream(binaryPathInJar)) {
    +  if (inputStream == null) {
    +    throw new IOException("Resource not found: " + binaryPathInJar);
    +  }
       Files.createDirectories(binary.getParent());
       Files.copy(inputStream, binary);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical issue by adding a null check for the input stream, which prevents a potential NullPointerException. This enhances the robustness and reliability of the code, especially when dealing with external resources.

    9
    Enhancement
    Use an enum for switch cases to improve type safety and maintainability

    Consider using an enum for the switch cases instead of string literals to improve
    type safety and maintainability.

    java/src/org/openqa/selenium/manager/SeleniumManagerOutput.java [95-105]

    -switch (name) {
    -  case "level":
    +enum LogField {
    +  LEVEL, TIMESTAMP, MESSAGE
    +}
    +
    +switch (LogField.valueOf(name.toUpperCase())) {
    +  case LEVEL:
         level = input.nextString();
         break;
     
    -  case "timestamp":
    +  case TIMESTAMP:
         timestamp = input.nextNumber().longValue();
         break;
     
    -  case "message":
    +  case MESSAGE:
         message = input.nextString();
         break;
     
       default:
         input.skipValue();
         break;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using an enum for switch cases improves type safety and maintainability by reducing the risk of errors from string literals. This suggestion enhances the code's structure and readability, making it easier to manage and extend.

    7

    💡 Need additional feedback ? start a PR chat

    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.

    1 participant