Copilot commented on code in PR #12241:
URL: https://github.com/apache/cloudstack/pull/12241#discussion_r2610608709
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java:
##########
@@ -424,7 +447,15 @@ protected EnforcementPointListResult
getEnforcementPoints(String siteId) {
public TransportZoneListResult getTransportZones() {
try {
com.vmware.nsx.TransportZones transportZones =
(com.vmware.nsx.TransportZones)
nsxService.apply(com.vmware.nsx.TransportZones.class);
- return transportZones.list(null, null, true, null, null, null,
null, null, TransportType.OVERLAY.name(), null);
+ return PagedFetcher.<TransportZoneListResult,
TransportZone>withPageFetcher(
+ cursor -> transportZones.list(cursor, null, true, null,
null, null, null, null, TransportType.OVERLAY.name(), cursor)
Review Comment:
The cursor parameter is passed twice to the list method - once as the first
parameter and again as the last parameter. The first parameter should be the
cursor for pagination, but the last parameter appears to be a duplicate. Check
the NSX SDK API documentation to ensure the correct parameters are being passed.
```suggestion
cursor -> transportZones.list(cursor, null, true, null,
null, null, null, null, TransportType.OVERLAY.name())
```
##########
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/PagedFetcherTest.java:
##########
@@ -0,0 +1,140 @@
+package org.apache.cloudstack.service;
+
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+
+public class PagedFetcherTest {
+
+ private static class Page {
+ private String cursor;
+ private List<String> items;
+
+ Page(String cursor, List<String> items) {
+ this.cursor = cursor;
+ this.items = items;
+ }
+
+ String getCursor() {
+ return cursor;
+ }
+
+ List<String> getItems() {
+ return items;
+ }
+
+ void setItems(List<String> items) {
+ this.items = items;
+ }
+ }
+
+ @Test
+ public void testFetchAllWhenThereIsNoPagination() {
+ // given
+ Page firstPage = new Page(null, new ArrayList<>(List.of("a", "b")));
+ AtomicBoolean itemsSetterCalled = new AtomicBoolean(false);
+ PagedFetcher<Page, String> fetcher = PagedFetcher.<Page,
String>withPageFetcher(
+ cursor -> {
+ assertNull(cursor);
+ return firstPage;
+ })
+ .cursorExtractor(Page::getCursor)
+ .itemsExtractor(Page::getItems)
+ .itemsSetter((page, items) -> itemsSetterCalled.set(true));
+
+ // when
+ Page result = fetcher.fetchAll();
+
+ // then
+ assertSame(firstPage, result);
+ assertEquals(List.of("a", "b"), result.getItems());
+ assertFalse("itemsSetter must not be called when there is no next
page", itemsSetterCalled.get());
+ }
+
+ @Test
+ public void testFetchAllWhenThereIsNoPaginationAndEmptyCursor() {
+ // given
+ Page firstPage = new Page("", new ArrayList<>(List.of("x")));
+
+ AtomicBoolean itemsSetterCalled = new AtomicBoolean(false);
+
+ PagedFetcher<Page, String> fetcher = PagedFetcher
+ .<Page, String>withPageFetcher(cursor -> {
+ assertNull(cursor);
+ return firstPage;
+ })
+ .cursorExtractor(Page::getCursor)
+ .itemsExtractor(Page::getItems)
+ .itemsSetter((page, items) -> itemsSetterCalled.set(true));
+
+ // when
+ Page result = fetcher.fetchAll();
+
+ // then
+ assertSame(firstPage, result);
+ assertEquals(List.of("x"), result.getItems());
+ assertFalse("itemsSetter must not be called when there is no next
page", itemsSetterCalled.get());
+ }
+
+ @Test
+ public void fetchAllWhenMultiPages() {
+ // given
+ Page page1 = new Page("c1", new ArrayList<>(List.of("p1a", "p1b")));
+ Page page2 = new Page("c2", new ArrayList<>(List.of("p2a")));
+ Page page3 = new Page(null, new ArrayList<>(List.of("p3a", "p3b")));
+
+ Map<String, Page> pagesByCursor = new HashMap<>();
+ pagesByCursor.put(null, page1);
+ pagesByCursor.put("c1", page2);
+ pagesByCursor.put("c2", page3);
+
+ PagedFetcher<Page, String> fetcher = PagedFetcher
+ .<Page, String>withPageFetcher(pagesByCursor::get)
+ .cursorExtractor(Page::getCursor)
+ .itemsExtractor(Page::getItems)
+ .itemsSetter((page, items) -> {
+ assertSame(page1, page);
+ page.setItems(items);
+ });
+
+ // when
+ Page result = fetcher.fetchAll();
+
+ // then
+ assertSame("Result must be the first page object", page1, result);
+ assertEquals(List.of("p1a", "p1b", "p2a", "p3a", "p3b"),
result.getItems());
+ }
+
+ @Test
+ public void fetchAllFirstPageItemsNullSecondWithItems() {
Review Comment:
The test method name does not follow Java naming conventions. It should be
`testFetchAllFirstPageItemsNullSecondWithItems` to be consistent with other
test methods in the class that start with `test`.
```suggestion
public void testFetchAllFirstPageItemsNullSecondWithItems() {
```
##########
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java:
##########
@@ -582,8 +623,7 @@ public void deleteNatRule(Network.Service service, String
privatePort, String pr
natService.delete(tier1GatewayName, NatId.USER.name(),
ruleName);
}
} catch (Error error) {
- String msg = String.format("Cannot find NAT rule with name %s: %s,
skipping deletion", ruleName, error.getMessage());
- logger.debug(msg);
+ logger.debug("Cannot find NAT rule with name %s: %s, skipping
deletion", ruleName, error.getMessage());
Review Comment:
The debug message is using String.format style placeholders (%s) but is
being called with SLF4J's parameterized logging. Change the %s placeholders to
{} to match the SLF4J logging pattern used consistently throughout the rest of
the file.
```suggestion
logger.debug("Cannot find NAT rule with name {}: {}, skipping
deletion", ruleName, error.getMessage());
```
##########
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/PagedFetcherTest.java:
##########
@@ -0,0 +1,140 @@
+package org.apache.cloudstack.service;
+
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+
+public class PagedFetcherTest {
+
+ private static class Page {
+ private String cursor;
+ private List<String> items;
+
+ Page(String cursor, List<String> items) {
+ this.cursor = cursor;
+ this.items = items;
+ }
+
+ String getCursor() {
+ return cursor;
+ }
+
+ List<String> getItems() {
+ return items;
+ }
+
+ void setItems(List<String> items) {
+ this.items = items;
+ }
+ }
+
+ @Test
+ public void testFetchAllWhenThereIsNoPagination() {
+ // given
+ Page firstPage = new Page(null, new ArrayList<>(List.of("a", "b")));
+ AtomicBoolean itemsSetterCalled = new AtomicBoolean(false);
+ PagedFetcher<Page, String> fetcher = PagedFetcher.<Page,
String>withPageFetcher(
+ cursor -> {
+ assertNull(cursor);
+ return firstPage;
+ })
+ .cursorExtractor(Page::getCursor)
+ .itemsExtractor(Page::getItems)
+ .itemsSetter((page, items) -> itemsSetterCalled.set(true));
+
+ // when
+ Page result = fetcher.fetchAll();
+
+ // then
+ assertSame(firstPage, result);
+ assertEquals(List.of("a", "b"), result.getItems());
+ assertFalse("itemsSetter must not be called when there is no next
page", itemsSetterCalled.get());
+ }
+
+ @Test
+ public void testFetchAllWhenThereIsNoPaginationAndEmptyCursor() {
+ // given
+ Page firstPage = new Page("", new ArrayList<>(List.of("x")));
+
+ AtomicBoolean itemsSetterCalled = new AtomicBoolean(false);
+
+ PagedFetcher<Page, String> fetcher = PagedFetcher
+ .<Page, String>withPageFetcher(cursor -> {
+ assertNull(cursor);
+ return firstPage;
+ })
+ .cursorExtractor(Page::getCursor)
+ .itemsExtractor(Page::getItems)
+ .itemsSetter((page, items) -> itemsSetterCalled.set(true));
+
+ // when
+ Page result = fetcher.fetchAll();
+
+ // then
+ assertSame(firstPage, result);
+ assertEquals(List.of("x"), result.getItems());
+ assertFalse("itemsSetter must not be called when there is no next
page", itemsSetterCalled.get());
+ }
+
+ @Test
+ public void fetchAllWhenMultiPages() {
Review Comment:
The test method name does not follow Java naming conventions. It should be
`testFetchAllWhenMultiPages` to be consistent with other test methods in the
class (e.g., `testFetchAllWhenThereIsNoPagination` on line 41).
```suggestion
public void testFetchAllWhenMultiPages() {
```
--
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]
