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

14035: Add 'toJson' method to produce expected serialization #14038

Merged
merged 3 commits into from
May 28, 2024

Conversation

sbabcoc
Copy link
Contributor

@sbabcoc sbabcoc commented May 25, 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

The standard Rectangle class (org.openqa.selenium.Rectangle) lacks a toJson() method, which causes the serialization performed by the Json API to fall back to scanning for methods that look like JavaBean accessors to extract object properties. In addition to the methods that provide access to inherent properties (width, height, x, and y), the Rectangle class provides two methods that produce derived properties (dimension and point). Consequently, the JSON current returned for Rectangle objects includes these derived properties as well.

Motivation and Context

Fixes #14035

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 a toJson method to the Rectangle class to produce expected JSON serialization.
  • The toJson method includes the width, height, x, and y properties in the JSON output.
  • Imported the Map class to facilitate JSON serialization.

Changes walkthrough 📝

Relevant files
Enhancement
Rectangle.java
Add `toJson` method for JSON serialization in `Rectangle` class.

java/src/org/openqa/selenium/Rectangle.java

  • Added toJson method to serialize Rectangle objects.
  • Included width, height, x, and y properties in the JSON output.
  • Imported Map class for JSON serialization.
  • +6/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are localized to a single class and involve adding a straightforward serialization method. The logic is simple and the impact is limited to the serialization behavior of the Rectangle class.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Missing Access Modifier: The toJson method should be declared as public if it is intended to be used outside the class. If it's for internal use, consider making it protected or package-private with a comment explaining its intended use.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add null checks for fields in the toJson method to prevent NullPointerException

    Add null checks for the width, height, x, and y fields in the toJson method to avoid
    potential NullPointerException if any of these fields are null.

    java/src/org/openqa/selenium/Rectangle.java [69-70]

     return Map.of("class", getClass().getName(),
    -  "width", width, "height", height, "x", x, "y", y);
    +  "width", Objects.requireNonNullElse(width, 0),
    +  "height", Objects.requireNonNullElse(height, 0),
    +  "x", Objects.requireNonNullElse(x, 0),
    +  "y", Objects.requireNonNullElse(y, 0));
     
    Suggestion importance[1-10]: 7

    Why: Adding null checks is a good practice to avoid NullPointerException, enhancing the robustness of the toJson method.

    7
    Best practice
    Rename the toJson method to toMap for better clarity of its functionality

    Rename the toJson method to toMap to better reflect its functionality of converting the
    object to a map representation, which can then be serialized to JSON.

    java/src/org/openqa/selenium/Rectangle.java [68]

    -private Map<String, Object> toJson() {
    +private Map<String, Object> toMap() {
     
    Suggestion importance[1-10]: 6

    Why: Renaming toJson to toMap could improve method naming clarity, reflecting that the method returns a map rather than a JSON string.

    6
    Enhancement
    Change the visibility of the toJson method to public for better accessibility

    Consider making the toJson method public to allow external classes to access the JSON
    representation of the Rectangle object. This will make the method more useful for
    serialization purposes.

    java/src/org/openqa/selenium/Rectangle.java [68]

    -private Map<String, Object> toJson() {
    +public Map<String, Object> toJson() {
     
    Suggestion importance[1-10]: 5

    Why: Making the toJson method public could enhance its utility for external use, but the necessity of this change depends on the design intent and potential security considerations.

    5
    Use HashMap instead of Map.of for creating the JSON representation to allow future modifications

    Use a HashMap instead of Map.of to create the JSON representation. HashMap allows for more
    flexibility, such as modifying the map after creation if needed.

    java/src/org/openqa/selenium/Rectangle.java [69-70]

    -return Map.of("class", getClass().getName(),
    -  "width", width, "height", height, "x", x, "y", y);
    +Map<String, Object> jsonMap = new HashMap<>();
    +jsonMap.put("class", getClass().getName());
    +jsonMap.put("width", width);
    +jsonMap.put("height", height);
    +jsonMap.put("x", x);
    +jsonMap.put("y", y);
    +return jsonMap;
     
    Suggestion importance[1-10]: 4

    Why: While using HashMap provides flexibility, the suggestion lacks context on whether such flexibility is required, making it a potentially unnecessary change.

    4

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    3 participants