Skip to content

[java] Add nullness for Proxy and print #15094

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

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Jan 15, 2025

User description

Description

In this PR I'm adding nullness annotations for classes:

  • org.openqa.selenium.Proxy
  • org.openqa.selenium.print.PageMargin
  • org.openqa.selenium.print.PageSize
  • org.openqa.selenium.print.PrintOptions

NullAway analysis: #14421

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291

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

  • Added JSpecify Nullness annotations to improve null safety.

  • Annotated fields and methods in Proxy class with @Nullable.

  • Marked PageMargin, PageSize, and PrintOptions classes with @NullMarked.

  • Enhanced IDE and static analysis support for nullability.


Changes walkthrough 📝

Relevant files
Enhancement
Proxy.java
Add nullness annotations to `Proxy` class                               

java/src/org/openqa/selenium/Proxy.java

  • Added @NullMarked annotation to the class.
  • Annotated fields and methods with @Nullable for null safety.
  • Updated equals method to handle nullable objects.
  • Improved null handling in extractFrom method.
  • +24/-21 
    PageMargin.java
    Mark `PageMargin` class with `@NullMarked`                             

    java/src/org/openqa/selenium/print/PageMargin.java

    • Added @NullMarked annotation to the class.
    +2/-0     
    PageSize.java
    Mark `PageSize` class with `@NullMarked`                                 

    java/src/org/openqa/selenium/print/PageSize.java

    • Added @NullMarked annotation to the class.
    +2/-0     
    PrintOptions.java
    Add nullness annotations to `PrintOptions` class                 

    java/src/org/openqa/selenium/print/PrintOptions.java

  • Added @NullMarked annotation to the class.
  • Annotated pageRanges field and related methods with @Nullable.
  • +5/-2     

    💡 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:

    🎫 Ticket compliance analysis ✅

    14291 - PR Code Verified

    Compliant requirements:

    • Add JSpecify Nullness annotations to Selenium framework code
    • Annotations specify which parameters and return values can be null
    • Annotations are transparent to IDEs and static code analyzers

    Requires further human verification:

    • Verify improved interoperability with Kotlin
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Null Check

    The setters.containsKey(key) check was added after the null check, which could lead to a NullPointerException if the key exists but is not in the setters map

    if (key != null && value != null && setters.containsKey(key)) {

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Maintain consistent null safety by adding @nullable annotations to setter method parameters

    Add @nullable annotation to the parameter of setFtpProxy() and other setter methods
    to maintain consistent null safety across the codebase.

    java/src/org/openqa/selenium/Proxy.java [242-244]

    -public Proxy setFtpProxy(String ftpProxy) {
    +public Proxy setFtpProxy(@Nullable String ftpProxy) {
       this.ftpProxy = ftpProxy;
       return this;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion aligns well with the PR's focus on null safety improvements. Adding @nullable to setter parameters would complete the null safety annotations consistently across the codebase.

    7
    Possible issue
    Add null check before calling accept() on potentially null Consumer to prevent NullPointerException

    Add null check for setters.get(key) before calling accept() to avoid potential
    NullPointerException if key exists in map but value is null.

    java/src/org/openqa/selenium/Proxy.java [126-128]

     if (key != null && value != null && setters.containsKey(key)) {
    -  setters.get(key).accept(value);
    +  Consumer<Object> setter = setters.get(key);
    +  if (setter != null) {
    +    setter.accept(value);
    +  }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 2

    Why: While technically correct, this suggestion is overly cautious. The setters.get(key) cannot return null since we already check setters.containsKey(key), making the additional null check redundant.

    2

    @mk868
    Copy link
    Contributor Author

    mk868 commented Jan 15, 2025

    Fixed NullAway errors:

    java/src/org/openqa/selenium/print/PrintOptions.java:49: error: [NullAway] @NonNull field pageRanges not initialized
      private String[] pageRanges;
                       ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/Proxy.java:90: error: [NullAway] initializer method does not guarantee @NonNull fields ftpProxy (line 80), httpProxy (line 81), noProxy (line 82), sslProxy (line 83), socksProxy (line 84), socksVersion (line 85), socksUsername (line 86), socksPassword (line 87), proxyAutoconfigUrl (line 88) are initialized along all control-flow paths (remember to check for exceptions or early returns).
      public Proxy() {
             ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/Proxy.java:94: error: [NullAway] initializer method does not guarantee @NonNull fields ftpProxy (line 80), httpProxy (line 81), noProxy (line 82), sslProxy (line 83), socksProxy (line 84), socksVersion (line 85), socksUsername (line 86), socksPassword (line 87), proxyAutoconfigUrl (line 88) are initialized along all control-flow paths (remember to check for exceptions or early returns).
      public Proxy(Map<String, ?> raw) {
             ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/Proxy.java:124: error: [NullAway] dereferenced expression setters.get(key) is @Nullable
                setters.get(key).accept(value);
                                ^
        (see http://t.uber.com/nullaway )
    java/src/org/openqa/selenium/Proxy.java:441: error: [NullAway] returning @Nullable expression from method with @NonNull return type
        return proxy;
        ^
        (see http://t.uber.com/nullaway )
    

    @diemol diemol merged commit 11e1309 into SeleniumHQ:trunk Jan 16, 2025
    @mk868 mk868 deleted the jspecify-print-and-proxy branch January 16, 2025 09:44
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    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