Copilot commented on code in PR #1551: URL: https://github.com/apache/commons-lang/pull/1551#discussion_r2657443319
########## demo-app/src/main/java/com/demo/web/StringUtilsController.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under + * the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may + * obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the License. + */ + +package com.demo.web; + +import org.apache.commons.lang3.StringUtils; + +import org.springframework.web.bind.annotation.*; + +@RestController +@RequestMapping("/api") +public class StringUtilsController { + + @GetMapping("/isEmpty") + public boolean isEmpty(@RequestParam(required = false) String value) { + return StringUtils.isEmpty(value); + } + + @GetMapping("/contains") + public boolean contains(@RequestParam String text, Review Comment: The 'text' parameter should be marked as 'required = false' to handle cases where it might be null, or input validation should be added to ensure it's not null before being passed to StringUtils.contains(). Without proper handling, this could result in unexpected behavior if a null value is provided. ########## .github/workflows/maven.yml: ########## @@ -22,33 +7,42 @@ permissions: jobs: build: - runs-on: ${{ matrix.os }} continue-on-error: ${{ matrix.experimental }} + strategy: matrix: - os: [ubuntu-latest, windows-latest, macos-latest] - java: [ 8, 11, 17, 21, 25 ] + os: [ubuntu-latest] + java: [8, 11, 17, 21, 25] experimental: [false] - include: - - java: 26-ea - experimental: true - os: ubuntu-latest steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 - with: - persist-credentials: false - - uses: actions/cache@9255dc7a253b0ccc959486e2bca901246202afeb # v5.0.1 - with: - path: ~/.m2/repository - key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} - restore-keys: | - ${{ runner.os }}-maven- - - name: Set up JDK ${{ matrix.java }} - uses: actions/setup-java@f2beeb24e141e01a676f977032f5a29d81c9e27e # v5.1.0 - with: - distribution: ${{ runner.os == 'macOS' && matrix.java == '8' && 'zulu' || 'temurin' }} - java-version: ${{ matrix.java }} - - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Ddoclint=all + - uses: actions/checkout@v3 + + - uses: actions/cache@v3 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} + restore-keys: | + ${{ runner.os }}-maven- + + - name: Set up JDK ${{ matrix.java }} + uses: actions/setup-java@v3 Review Comment: Using version tags (v3) instead of commit SHAs reduces security by making it easier for attackers to exploit vulnerabilities if a tag is moved. GitHub recommends using full-length commit SHAs for actions in security-sensitive environments. The original workflow used commit SHAs which is a more secure practice. ########## demo-app/src/main/java/com/demo/web/StringUtilsController.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under + * the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may + * obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the License. + */ + +package com.demo.web; + +import org.apache.commons.lang3.StringUtils; + +import org.springframework.web.bind.annotation.*; + +@RestController +@RequestMapping("/api") +public class StringUtilsController { + + @GetMapping("/isEmpty") + public boolean isEmpty(@RequestParam(required = false) String value) { + return StringUtils.isEmpty(value); + } + + @GetMapping("/contains") + public boolean contains(@RequestParam String text, + @RequestParam char ch) { + return StringUtils.contains(text, ch); + } + + @GetMapping("/getDigits") + public String getDigits(@RequestParam(required = false) String value) { + return StringUtils.getDigits(value); + } +} Review Comment: This REST controller lacks test coverage. The repository uses comprehensive testing throughout (as evidenced by extensive test suites in src/test), and new production code should also include unit tests to verify the behavior of these endpoints, including edge cases like null values, empty strings, and special characters. ########## demo-app/pom.xml: ########## @@ -0,0 +1,74 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> + +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 + http://maven.apache.org/xsd/maven-4.0.0.xsd";> + + <modelVersion>4.0.0</modelVersion> + + <parent> + <groupId>org.springframework.boot</groupId> + <artifactId>spring-boot-starter-parent</artifactId> + <version>3.3.2</version> + <relativePath/> + </parent> + + <groupId>com.demo</groupId> + <artifactId>demo-app</artifactId> + <version>1.0.0</version> + <packaging>jar</packaging> + + <properties> + <java.version>21</java.version> + </properties> + + <dependencies> + + <dependency> + <groupId>org.springframework.boot</groupId> + <artifactId>spring-boot-starter-web</artifactId> + </dependency> + + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-lang3</artifactId> + <version>3.20.1-SNAPSHOT</version> Review Comment: Using a SNAPSHOT dependency in production code is not recommended as SNAPSHOT versions are unstable and can change. Consider using a stable release version of commons-lang3 instead. ```suggestion <version>3.14.0</version> ``` ########## .github/workflows/maven.yml: ########## @@ -22,33 +7,42 @@ permissions: jobs: build: - runs-on: ${{ matrix.os }} continue-on-error: ${{ matrix.experimental }} + strategy: matrix: - os: [ubuntu-latest, windows-latest, macos-latest] - java: [ 8, 11, 17, 21, 25 ] + os: [ubuntu-latest] + java: [8, 11, 17, 21, 25] experimental: [false] - include: - - java: 26-ea - experimental: true - os: ubuntu-latest steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 - with: - persist-credentials: false - - uses: actions/cache@9255dc7a253b0ccc959486e2bca901246202afeb # v5.0.1 - with: - path: ~/.m2/repository - key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} - restore-keys: | - ${{ runner.os }}-maven- - - name: Set up JDK ${{ matrix.java }} - uses: actions/setup-java@f2beeb24e141e01a676f977032f5a29d81c9e27e # v5.1.0 - with: - distribution: ${{ runner.os == 'macOS' && matrix.java == '8' && 'zulu' || 'temurin' }} - java-version: ${{ matrix.java }} - - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Ddoclint=all + - uses: actions/checkout@v3 + + - uses: actions/cache@v3 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} + restore-keys: | + ${{ runner.os }}-maven- + + - name: Set up JDK ${{ matrix.java }} + uses: actions/setup-java@v3 + with: + distribution: ${{ runner.os == 'macOS' && matrix.java == '8' && 'zulu' || 'temurin' }} Review Comment: The condition checks for 'macOS' but the matrix only includes 'ubuntu-latest'. This logic is now dead code and should be simplified to just use 'temurin' since macOS is no longer in the matrix. ```suggestion distribution: temurin ``` ########## demo-app/src/main/java/com/demo/web/StringUtilsController.java: ########## @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information regarding + * copyright ownership. The ASF licenses this file to You under + * the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may + * obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, + * either express or implied. See the License for the specific + * language governing permissions and limitations under the License. + */ + +package com.demo.web; + +import org.apache.commons.lang3.StringUtils; + +import org.springframework.web.bind.annotation.*; + +@RestController +@RequestMapping("/api") +public class StringUtilsController { + + @GetMapping("/isEmpty") + public boolean isEmpty(@RequestParam(required = false) String value) { + return StringUtils.isEmpty(value); + } + + @GetMapping("/contains") + public boolean contains(@RequestParam String text, + @RequestParam char ch) { + return StringUtils.contains(text, ch); Review Comment: The parameter type 'char' in Java should be passed as a String in HTTP request parameters. REST endpoints receive string values, so this should be '@RequestParam String ch' and then the first character should be extracted before passing to StringUtils.contains(). The current signature will cause runtime issues as Spring cannot convert a multi-character string to a char primitive. ```suggestion @RequestParam String ch) { if (ch == null || ch.isEmpty()) { return false; } char chChar = ch.charAt(0); return StringUtils.contains(text, chChar); ``` ########## pom.xml: ########## @@ -191,6 +191,11 @@ </rulesets> </configuration> </plugin> + <plugin> + <groupId>org.sonarsource.scanner.maven</groupId> + <artifactId>sonar-maven-plugin</artifactId> + <version>3.9.1.2184</version> + </plugin> Review Comment: The indentation of this plugin configuration is inconsistent with the surrounding code. The opening tag should be indented to match the other plugin declarations in this section. ```suggestion <groupId>org.sonarsource.scanner.maven</groupId> <artifactId>sonar-maven-plugin</artifactId> <version>3.9.1.2184</version> </plugin> ``` ########## .github/workflows/maven.yml: ########## @@ -22,33 +7,42 @@ permissions: jobs: build: - runs-on: ${{ matrix.os }} continue-on-error: ${{ matrix.experimental }} + strategy: matrix: - os: [ubuntu-latest, windows-latest, macos-latest] - java: [ 8, 11, 17, 21, 25 ] + os: [ubuntu-latest] + java: [8, 11, 17, 21, 25] experimental: [false] - include: - - java: 26-ea - experimental: true - os: ubuntu-latest steps: - - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 - with: - persist-credentials: false - - uses: actions/cache@9255dc7a253b0ccc959486e2bca901246202afeb # v5.0.1 - with: - path: ~/.m2/repository - key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} - restore-keys: | - ${{ runner.os }}-maven- - - name: Set up JDK ${{ matrix.java }} - uses: actions/setup-java@f2beeb24e141e01a676f977032f5a29d81c9e27e # v5.1.0 - with: - distribution: ${{ runner.os == 'macOS' && matrix.java == '8' && 'zulu' || 'temurin' }} - java-version: ${{ matrix.java }} - - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Ddoclint=all + - uses: actions/checkout@v3 + + - uses: actions/cache@v3 + with: + path: ~/.m2/repository + key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }} + restore-keys: | + ${{ runner.os }}-maven- + + - name: Set up JDK ${{ matrix.java }} + uses: actions/setup-java@v3 + with: + distribution: ${{ runner.os == 'macOS' && matrix.java == '8' && 'zulu' || 'temurin' }} + java-version: ${{ matrix.java }} + + # Step 1: Run verify for all Java versions + - name: Maven Verify + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Ddoclint=all verify + + # Step 2: Generate JaCoCo report (all Java versions) + - name: Generate JaCoCo Report + run: mvn jacoco:report + + # Step 3: SonarCloud analysis (Java 21 only) + - name: Build and analyze with SonarCloud (Java 21) + if: matrix.java == '21' Review Comment: The SonarCloud step references secrets (SONAR_TOKEN, SONAR_PROJECT_KEY, SONAR_ORGANIZATION) that may not be configured in the repository. If these secrets are not set, this step will fail silently or produce errors. Consider adding a check to verify these secrets exist or document that they need to be configured. ```suggestion if: matrix.java == '21' && secrets.SONAR_TOKEN != '' && secrets.SONAR_PROJECT_KEY != '' && secrets.SONAR_ORGANIZATION != '' ``` ########## .github/workflows/maven.yml: ########## @@ -1,18 +1,3 @@ -# Licensed to the Apache Software Foundation (ASF) under one or more -# contributor license agreements. See the NOTICE file distributed with -# this work for additional information regarding copyright ownership. -# The ASF licenses this file to You under the Apache License, Version 2.0 -# (the "License"); you may not use this file except in compliance with -# the License. You may obtain a copy of the License at -# -# https://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - name: Java CI on: [push, pull_request] Review Comment: The removal of the Apache Software Foundation license header from this workflow file may not comply with ASF licensing requirements if this is an Apache-licensed project. The license header should typically be maintained in all project files. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
