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

grid/TomlConfig: migrate TOML library to tomlj/tomlj #14470

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

Delta456
Copy link
Contributor

@Delta456 Delta456 commented Sep 4, 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 PR uses a new TOML parser as stated in #13538

Motivation and Context

As this TOML library is more maintained than the one used so lesser chances of problems and vulnerabilities.

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, dependencies, tests


Description

  • Migrated the TOML parsing library from jtoml to tomlj in the TomlConfig class to ensure better maintenance and reduce vulnerabilities.
  • Implemented error handling for TOML parsing errors using the tomlj library.
  • Updated tests to validate the new error handling and ensure compatibility with the tomlj library.
  • Adjusted Bazel and Maven configurations to replace jtoml with tomlj.

Changes walkthrough 📝

Relevant files
Enhancement
TomlConfig.java
Migrate TOML parsing from `jtoml` to `tomlj`                         

java/src/org/openqa/selenium/grid/config/TomlConfig.java

  • Migrated from jtoml to tomlj for TOML parsing.
  • Added error handling for TOML parsing errors.
  • Updated methods to use tomlj API.
  • +29/-13 
    Tests
    TomlConfigTest.java
    Update tests for new TOML library `tomlj`                               

    java/test/org/openqa/selenium/grid/config/TomlConfigTest.java

  • Added test for error handling in TOML parsing.
  • Updated existing tests to accommodate new TOML library.
  • +11/-4   
    Dependencies
    MODULE.bazel
    Update Bazel dependencies for TOML library migration         

    MODULE.bazel

    • Removed jtoml dependency.
    • Added tomlj dependency.
    +1/-1     
    maven_install.json
    Update Maven artifacts for TOML library migration               

    java/maven_install.json

    • Removed jtoml artifact.
    • Added tomlj artifact.
    +37/-15 
    BUILD.bazel
    Update Bazel build file for TOML library change                   

    java/src/org/openqa/selenium/grid/config/BUILD.bazel

    • Replaced jtoml with tomlj in dependencies.
    +1/-1     
    BUILD.bazel
    Update test dependencies for TOML library migration           

    java/test/org/openqa/selenium/grid/config/BUILD.bazel

    • Removed jtoml from test dependencies.
    +0/-1     

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

    @Delta456
    Copy link
    Contributor Author

    Delta456 commented Sep 4, 2024

    As this TOML library uses a different approach compared to the old one. I would require help from the core maintainers to pass the tests and may have to change the tests and add more accordingly.

    @pujagani pujagani marked this pull request as ready for review September 12, 2024 07:08
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The new implementation throws a ConfigException with all parsing errors concatenated. Consider handling each error separately or providing more context.

    Possible Performance Issue
    The getAll method now performs multiple checks and conversions. Consider optimizing for frequently accessed configurations.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use try-with-resources to automatically close the reader

    Consider using a try-with-resources statement to ensure that the reader is properly
    closed after use.

    java/src/org/openqa/selenium/grid/config/TomlConfig.java [42-44]

     public TomlConfig(Reader reader) {
    -  try {
    -    toml = Toml.parse(reader);
    +  try (Reader r = reader) {
    +    toml = Toml.parse(r);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using try-with-resources is a best practice for managing resources like readers, ensuring they are closed properly, which can prevent resource leaks.

    8
    Use a null-safe method for converting values to strings

    Consider using a null-safe method like Objects.toString() instead of
    String.valueOf() to handle potential null values.

    java/src/org/openqa/selenium/grid/config/TomlConfig.java [125]

    -return Optional.of(List.of(String.valueOf(value)));
    +return Optional.of(List.of(Objects.toString(value)));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using Objects.toString() is a null-safe way to convert objects to strings, which can prevent potential NullPointerExceptions.

    6
    Enhancement
    Use a more specific exception type for TOML parsing errors

    Consider using a more specific exception type for TOML parsing errors instead of the
    generic ConfigException.

    java/src/org/openqa/selenium/grid/config/TomlConfig.java [46-51]

     if (toml.hasErrors()) {
       String error =
           toml.errors().stream().map(TomlParseError::toString).collect(Collectors.joining("\n"));
     
    -  throw new ConfigException(error);
    +  throw new TomlParseException(error);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more specific exception type like TomlParseException can improve error handling by providing more detailed information about the nature of the error.

    7
    Add more specific assertions for error handling tests

    Consider adding more specific assertions to check the exact error message or
    exception details in the error handling test.

    java/test/org/openqa/selenium/grid/config/TomlConfigTest.java [42-43]

     assertThatThrownBy(() -> new TomlConfig(new StringReader(raw)))
    -    .isInstanceOf(ConfigException.class);
    +    .isInstanceOf(ConfigException.class)
    +    .hasMessageContaining("bare keys cannot contain");
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding specific assertions for error messages can improve test robustness by ensuring that the correct exceptions are thrown with expected messages.

    7

    @pujagani
    Copy link
    Contributor

    pujagani commented Sep 12, 2024

    Updating the TOML parser will lead to a breaking change for users.

    Example:
    With the current TOML parser:

    // This throws errors with the new parser tomlj
    String raw = "[cheeses]\nselected=brie";
    Config config = new TomlConfig(new StringReader(raw))
    

    After the new TOML parser:

    String raw = "[cheeses]\nselected=\"brie\"";
     Config config = new TomlConfig(new StringReader(raw));
    

    The new parser requires quotes for the string values. Since our maven Grid package provides access to creating TomlConfig objects, it will be a breaking change for users.

    We can add a warning with the current code base about the upcoming change, and maybe merge this PR after 2 versions so the end users get a chance to update the TOML.

    @diemol What do you think?

    @Delta456
    Copy link
    Contributor Author

    @pujagani I believe we should go with giving a warning for this as it is a breaking change. As said let's merge and introduce this change after next 2 versions 🚀

    @pujagani
    Copy link
    Contributor

    @Delta456 Would like to create a separate PR displaying the warning?

    @Delta456
    Copy link
    Contributor Author

    @Delta456 Would like to create a separate PR displaying the warning?

    Sure

    @Delta456
    Copy link
    Contributor Author

    #14491 has been merged. Thanks @pujagani!

    @pujagani
    Copy link
    Contributor

    Please resolve the conflicts when time permits. Thank you!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    dependencies Pull requests that update a dependency file enhancement Review effort [1-5]: 3 tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants