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] setter for flag JsonInput.readPerformed #14921

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

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Dec 19, 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

This pull request includes several changes to the JsonInput class in the java/src/org/openqa/selenium/json/JsonInput.java file. The changes focus on fixing a previously unused flag, adding a new method to mark when reading is performed, and ensuring this method is called appropriately.

Key changes:

  • Fixed the readPerformed flag to be mutable and initialized properly.
  • Added a new method markReadPerformed to set the readPerformed flag to true.
  • Ensured the markReadPerformed method is called within the read method to accurately reflect when reading has occurred.
  • Added a blank line for better readability in the constructor.

Motivation and Context

i just noticed tag FIXME and decided to provide possible solution

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, Enhancement


Description

  • Fixed a bug where the readPerformed flag in JsonInput class was never set (removed FIXME comment)
  • Changed readPerformed from final to mutable boolean field
  • Added new private method markReadPerformed() to properly set the flag
  • Integrated read tracking by calling markReadPerformed() in the read() method
  • Improved code formatting by adding a blank line in the constructor

Changes walkthrough 📝

Relevant files
Bug fix
JsonInput.java
Fix and implement JsonInput read tracking functionality   

java/src/org/openqa/selenium/json/JsonInput.java

  • Fixed unused readPerformed flag by making it mutable and properly
    initialized
  • Added new markReadPerformed() method to set the flag
  • Integrated flag setting within the read() method
  • Minor formatting improvements in constructor
  • +7/-2     

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

    revert for comments
    
    applied formatting
    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

    Thread Safety
    The readPerformed flag is modified without synchronization which could cause race conditions if JsonInput is used across multiple threads

    Code Design
    The readPerformed flag is only set but never checked/used, which may indicate incomplete implementation or dead code

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure thread safety for a boolean flag that may be accessed concurrently

    Consider marking the readPerformed flag as volatile since it may be accessed from
    different threads.

    java/src/org/openqa/selenium/json/JsonInput.java [42]

    -private boolean readPerformed = false;
    +private volatile boolean readPerformed = false;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding volatile is a valid concern for thread safety, especially since JsonInput might be used in concurrent contexts. This could prevent potential race conditions and ensure visibility across threads.

    7
    General
    Add a public accessor method to expose the state of an internal flag

    Add a public getter method to check if reading has been performed, since the flag is
    private but may be useful for external validation.

    java/src/org/openqa/selenium/json/JsonInput.java [42-407]

     private boolean readPerformed = false;
     private void markReadPerformed() {
       readPerformed = true;
     }
    +public boolean isReadPerformed() {
    +  return readPerformed;
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While the suggestion provides better encapsulation by adding a getter method, the readPerformed flag appears to be primarily for internal use, and exposing it might not be necessary for the class's core functionality.

    4

    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