Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*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
*KIND, either express or implied. See the License for the
*specific language governing permissions and limitations
*under the License.
*/
Expand All @@ -30,70 +30,94 @@

@Service
public class PeopleService {
private ConcurrentMap< String, Person> persons = new ConcurrentHashMap< String, Person >();
public void setInitPeople()
{

for(int count =1;count <=10; count ++)
{
Person newPerson= new Person();
newPerson.setEmail(String.format("test-%d@wso2.com",count));
newPerson.setFirstName(String.format("testUser%d",count));
newPerson.setLastName(String.format("testLasrName%d",count));
persons.putIfAbsent(String.format("test-%d@wso2.com",count),newPerson) ;

private ConcurrentMap<String, Person> persons = new ConcurrentHashMap<>();

public void setInitPeople() {
for (int count = 1; count <= 10; count++) {
Person newPerson = new Person();
newPerson.setEmail(String.format("test-%d@wso2.com", count));
newPerson.setFirstName(String.format("testUser%d", count));
newPerson.setLastName(String.format("testLasrName%d", count));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Typo: "testLasrName%d""testLastName%d"

The misspelled last-name template will be stored in the persons map and could cause test assertions on lastName fields to fail unexpectedly.

🐛 Proposed fix
-            newPerson.setLastName(String.format("testLasrName%d", count));
+            newPerson.setLastName(String.format("testLastName%d", count));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
newPerson.setLastName(String.format("testLasrName%d", count));
newPerson.setLastName(String.format("testLastName%d", count));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/main/java/org/wso2/am/integration/services/jaxrs/peoplesample/PeopleService.java`
at line 41, In PeopleService update the last-name template string used when
setting newPerson.lastName: replace the misspelled "testLasrName%d" with the
correct "testLastName%d" in the call
newPerson.setLastName(String.format("testLasrName%d", count)) so stored entries
in the persons map use the correct lastName value.

persons.putIfAbsent(String.format("test-%d@wso2.com", count), newPerson);
}
}
public Collection< Person > getPeople( int page, int pageSize ) {
Collection< Person > person = new ArrayList< Person >( pageSize );

for( int index = 0; index < pageSize; ++index ) {
person.add( new Person( String.format( "person+%d@at.com", ( pageSize * ( page - 1 ) + index + 1 ) ) ) );
public Collection<Person> getPeople(int page, int pageSize) {

Collection<Person> person = new ArrayList<>(pageSize);

for (int index = 0; index < pageSize; ++index) {
person.add(new Person(
String.format("person+%d@at.com",
(pageSize * (page - 1) + index + 1))));
Comment on lines +50 to +53
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing validation for page and pageSize can produce malformed email addresses

With page = 0 the expression pageSize * (page - 1) + index + 1 evaluates to a negative number, producing strings like "person+-9@at.com". Callers of this integration-test endpoint can easily pass page = 0 by accident.

🛡️ Proposed guard
     public Collection<Person> getPeople(int page, int pageSize) {
+        if (page < 1) {
+            throw new IllegalArgumentException("page must be >= 1");
+        }
+        if (pageSize < 0) {
+            throw new IllegalArgumentException("pageSize must be >= 0");
+        }
         Collection<Person> person = new ArrayList<>(pageSize);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/main/java/org/wso2/am/integration/services/jaxrs/peoplesample/PeopleService.java`
around lines 50 - 53, The PeopleService endpoint currently uses page and
pageSize directly in the loop (in the for loop that constructs Person using
String.format("person+%d@at.com", (pageSize * (page - 1) + index + 1))) which
allows page=0 or pageSize<=0 to produce malformed emails; add input validation
at the start of the request handler (or the method that computes the list) to
ensure both page and pageSize are >= 1 (or return an HTTP
400/IllegalArgumentException for invalid values) and normalize or clamp values
as appropriate before running the loop so the computed index expression cannot
become negative. Ensure the validation references the page and pageSize
parameters in PeopleService and short-circuits before the person list creation.

}

setInitPeople();
return person;
}
Comment on lines +46 to 58
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

setInitPeople() is a disconnected side effect and should be called at bean initialization, not inside getPeople

The person list returned by getPeople contains "person+N@at.com" entries built inline; the setInitPeople() call on line 56 populates persons (the backing map) with entirely different "test-N@wso2.com" entries. These two data sets are never reconciled, so:

  1. The objects returned to callers are never stored in persons, making them unreachable via getByEmail / checkPersonByEmail.
  2. setInitPeople() runs on every getPeople invocation — wasteful, even if putIfAbsent keeps it idempotent.

The idiomatic Spring pattern is to annotate setInitPeople() with @PostConstruct so the backing store is populated exactly once after dependency injection completes, rather than as a side effect of a request-scoped method.

♻️ Proposed fix — move initialization to `@PostConstruct`
+import javax.annotation.PostConstruct;
 ...
+    `@PostConstruct`
     public void setInitPeople() {
         for (int count = 1; count <= 10; count++) {
             ...
         }
     }

     public Collection<Person> getPeople(int page, int pageSize) {
         Collection<Person> person = new ArrayList<>(pageSize);
         for (int index = 0; index < pageSize; ++index) {
             person.add(new Person(
                     String.format("person+%d@at.com",
                             (pageSize * (page - 1) + index + 1))));
         }
-        setInitPeople();
         return person;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/main/java/org/wso2/am/integration/services/jaxrs/peoplesample/PeopleService.java`
around lines 46 - 58, Remove the call to setInitPeople() from getPeople and
instead run setInitPeople once at bean initialization by annotating the
setInitPeople method with `@PostConstruct`; this ensures the backing map persons
is populated once (so getByEmail / checkPersonByEmail can find entries) and
avoids re-initializing on every getPeople call while leaving getPeople to only
build/return the requested page of Person objects.


public Person addPerson( String email ) {
public Person addPerson(String email) {

return new Person( email );
if (email == null || email.isEmpty()) {
throw new IllegalArgumentException("Email cannot be null or empty");
}

return new Person(email);
}
Comment on lines +60 to 67
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "PeopleService.java" | head -5

Repository: wso2/product-apim

Length of output: 222


🏁 Script executed:

cat -n ./all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/main/java/org/wso2/am/integration/services/jaxrs/peoplesample/PeopleService.java

Repository: wso2/product-apim

Length of output: 4764


🏁 Script executed:

#!/bin/bash
# Find REST resource files that might call these addPerson methods
find . -type f -name "*Resource.java" -o -name "*Controller.java" | grep -i people

Repository: wso2/product-apim

Length of output: 43


🏁 Script executed:

#!/bin/bash
# Find all Java files that reference PeopleService
rg -l "PeopleService" --type=java

Repository: wso2/product-apim

Length of output: 694


🏁 Script executed:

cat -n ./all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/main/java/org/wso2/am/integration/services/jaxrs/peoplesample/PeopleRestService.java

Repository: wso2/product-apim

Length of output: 6224


🏁 Script executed:

#!/bin/bash
# Search for all calls to addPerson to confirm usage patterns
rg -n "addPerson\s*\(" --type=java -A2 -B2

Repository: wso2/product-apim

Length of output: 6320


addPerson(String email) validates the input but never persists the new Person to the backing store

The single-argument addPerson creates and returns a Person without calling persons.putIfAbsent. This is inconsistent with the three-argument overload (lines 94–111) which does store the person. The REST endpoint at line 47–48 of PeopleRestService.java exposes this method via a @PUT /people operation, meaning any HTTP PUT request returns a person that is immediately invisible to getByEmail, checkPersonByEmail, and removePerson.

🐛 Proposed fix — persist the person (mirroring the 3-arg overload)
     public Person addPerson(String email) {
         if (email == null || email.isEmpty()) {
             throw new IllegalArgumentException("Email cannot be null or empty");
         }
-        return new Person(email);
+        final Person person = new Person(email);
+        if (persons.putIfAbsent(email, person) != null) {
+            throw new PersonAlreadyExistsException(email);
+        }
+        return person;
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/main/java/org/wso2/am/integration/services/jaxrs/peoplesample/PeopleService.java`
around lines 60 - 67, The addPerson(String email) method currently validates and
returns a new Person but does not persist it; update addPerson(String email) to
mirror the three-argument overload by inserting into the backing map using
persons.putIfAbsent(email, person) (or equivalent) and return the resident value
(the existing person if present, otherwise the newly created person) so that
getByEmail, checkPersonByEmail, and removePerson will observe the new entry.


public Person getByEmail( final String email ) {
final Person person = persons.get( email );
if( person == null ) {
throw new PersonNotFoundException( email );
public Person getByEmail(final String email) {

if (email == null || email.isEmpty()) {
throw new IllegalArgumentException("Email cannot be null or empty");
}

final Person person = persons.get(email);

if (person == null) {
throw new PersonNotFoundException(email);
}

return person;
}

public boolean checkPersonByEmail(final String email)
{
boolean personExists= true;
final Person person = persons.get( email );
if( person == null ) {
personExists=false;
public boolean checkPersonByEmail(final String email) {

if (email == null || email.isEmpty()) {
return false;
}
return personExists;

final Person person = persons.get(email);
return person != null;
}

public Person addPerson( final String email, final String firstName, final String lastName ) {
final Person person = new Person( email );
person.setFirstName( firstName );
person.setLastName( lastName );
public Person addPerson(final String email,
final String firstName,
final String lastName) {

if( persons.putIfAbsent( email, person ) != null ) {
throw new PersonAlreadyExistsException( email );
if (email == null || email.isEmpty()) {
throw new IllegalArgumentException("Email cannot be null or empty");
}

return person;
}
final Person person = new Person(email);
person.setFirstName(firstName);
person.setLastName(lastName);

public void removePerson( final String email ) {
if( persons.remove( email ) == null ) {
throw new PersonNotFoundException( email );
if (persons.putIfAbsent(email, person) != null) {
throw new PersonAlreadyExistsException(email);
}

return person;
}

public void removePerson(final String email) {

if (email == null || email.isEmpty()) {
throw new IllegalArgumentException("Email cannot be null or empty");
}

if (persons.remove(email) == null) {
throw new PersonNotFoundException(email);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package org.wso2.am.integration.services.jaxrs.peoplesample.bean;

import org.junit.Assert;
import org.junit.Test;

public class PersonTest {

@Test
public void testPersonConstructorAndGetters() {

Person person = new Person();

person.setEmail("john@example.com");
person.setFirstName("John");
person.setLastName("Doe");

Assert.assertEquals("john@example.com", person.getEmail());
Assert.assertEquals("John", person.getFirstName());
Assert.assertEquals("Doe", person.getLastName());
}

@Test
public void testPersonEquality() {

Person p1 = new Person();
p1.setEmail("test@example.com");

Person p2 = new Person();
p2.setEmail("test@example.com");

Assert.assertEquals(p1.getEmail(), p2.getEmail());
Comment on lines +23 to +31
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name testPersonEquality is misleading because it only compares email fields rather than testing object equality (using equals() method). Consider renaming to testPersonEmailEquality or testGetEmailReturnsCorrectValue to accurately reflect what is being tested.

Copilot uses AI. Check for mistakes.
}
Comment on lines +22 to +32
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

testPersonEquality does not test object equality and can never fail.

Two concrete issues:

  1. Trivially-true assertion: Assert.assertEquals(p1.getEmail(), p2.getEmail()) only compares two identical String literals — it will always pass regardless of whether Person implements equals()/hashCode() correctly. This gives false confidence.

  2. Wrong assertEquals argument order: JUnit convention is (expected, actual). Both arguments here are computed values, so a failure message would be unhelpful.

To meaningfully test equality, assert on the Person objects directly (if equals() is implemented) or at minimum fix the argument order and rename the method to reflect what is actually being tested:

🐛 Proposed fix
-    `@Test`
-    public void testPersonEquality() {
-
-        Person p1 = new Person();
-        p1.setEmail("test@example.com");
-
-        Person p2 = new Person();
-        p2.setEmail("test@example.com");
-
-        Assert.assertEquals(p1.getEmail(), p2.getEmail());
-    }
+    `@Test`
+    public void testPersonEqualityBySameEmail() {
+        Person p1 = new Person();
+        p1.setEmail("test@example.com");
+
+        Person p2 = new Person();
+        p2.setEmail("test@example.com");
+
+        // Comparing email field values (expected, actual)
+        Assert.assertEquals("test@example.com", p1.getEmail());
+        Assert.assertEquals("test@example.com", p2.getEmail());
+    }
+
+    `@Test`
+    public void testPersonEqualityByObject() {
+        // Uncomment if Person overrides equals()/hashCode()
+        // Person p1 = new Person();
+        // p1.setEmail("test@example.com");
+        // Person p2 = new Person();
+        // p2.setEmail("test@example.com");
+        // Assert.assertEquals(p1, p2);
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@all-in-one-apim/modules/integration/tests-common/backend-service/jaxrs-app/src/test/java/org/wso2/am/integration/services/jaxrs/peoplesample/bean/PersonTest.java`
around lines 22 - 32, The test testPersonEquality currently compares only the
email strings and uses two computed values in the wrong argument order, so
update it to assert object equality: create two Person instances with identical
fields and use Assert.assertEquals(expectedPerson, actualPerson) (or
Assert.assertTrue(p1.equals(p2))) to verify Person.equals()/hashCode() behavior,
or if you only intend to test the email accessor keep the method name and use
Assert.assertEquals("test@example.com", p1.getEmail()) with the expected-first
argument order; reference Person, testPersonEquality, equals(), and
Assert.assertEquals when making the change.

}
Loading