Skip to content

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

Merged
diemol merged 3 commits intoSeleniumHQ:trunkfrom
sbabcoc:pr/14035
May 28, 2024
Merged

14035: Add 'toJson' method to produce expected serialization#14038
diemol merged 3 commits intoSeleniumHQ:trunkfrom
sbabcoc:pr/14035

Conversation

@sbabcoc
Copy link
Contributor

@sbabcoc sbabcoc commented May 25, 2024 •
edited by qodo-code-review bot
Loading

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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 25, 2024

    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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 25, 2024

    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

    pujagani
    pujagani reviewed May 27, 2024
    View reviewed changes
    @pujagani pujagani added the C-java Java Bindings label May 27, 2024
    @diemol diemol merged commit 0d6ce4f into SeleniumHQ:trunk May 28, 2024
    @seedysoft seedysoft mentioned this pull request Jun 21, 2024
    Merged
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Jul 12, 2024
    Merged
    @aravindb26 aravindb26 mentioned this pull request Jul 16, 2024
    Open
    @x-way x-way mentioned this pull request Jul 24, 2024
    Closed
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @sbabcoc @pujagani
    @diemol @sandeepsuryaprasad
    14035: Add 'toJson' method to produce expected serialization (Seleniu…
    aab9aa5 
    …mHQ#14038)

Co-authored-by: Puja Jagani <puja.jagani93@gmail.com>
Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @pujagani pujagani pujagani left review comments

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    C-java Java Bindings P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: Selenium Java API: Serialization of 'Rectangle' objects by the Json API includes derived properties

    3 participants

    @sbabcoc @diemol @pujagani