fix(requests): Properly handle optional request parameters (#1750)

* test(web): Define the behavior of the /type/{accountType} endpoint * fix(request): Properly declare optional request parameters Optional is not the right way to declare that a particular request parameter is not required. There was a similar issue in the corresponding clouddriver endpoint that was fixed by spinnaker/clouddriver#6067 --------- Co-authored-by: Daniel Zheng <[email protected]>
spinnaker · Dec 14, 2023 · ba6c51b · ba6c51b
1 parent e01e687
commit ba6c51b
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 4 deletions.
diff --git a/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/CredentialsController.groovy b/gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/CredentialsController.groovy
@@ -97,11 +97,11 @@ class CredentialsController {
     @ApiParam(value = 'Value of the "@type" key for accounts to search for.', example = 'kubernetes')
     @PathVariable String accountType,
     @ApiParam('Maximum number of entries to return in results. Used for pagination.')
-    @RequestParam OptionalInt limit,
+    @RequestParam(required = false) Integer limit,
     @ApiParam('Account name to start account definition listing from. Used for pagination.')
-    @RequestParam Optional<String> startingAccountName
+    @RequestParam(required = false) String startingAccountName
   ) {
-    clouddriverService.getAccountDefinitionsByType(accountType, limit.isPresent() ? limit.getAsInt() : null, startingAccountName.orElse(null))
+    clouddriverService.getAccountDefinitionsByType(accountType, limit, startingAccountName)
   }
 
   @PostMapping

diff --git a/...b/src/test/groovy/com/netflix/spinnaker/gate/controllers/CredentialsControllerSpec.groovy b/...b/src/test/groovy/com/netflix/spinnaker/gate/controllers/CredentialsControllerSpec.groovy
@@ -58,7 +58,11 @@ class CredentialsControllerSpec extends Specification {
     contentNegotiationManagerFactoryBean.addMediaType("json", MediaType.APPLICATION_JSON)
     contentNegotiationManagerFactoryBean.favorPathExtension = false
     mockMvc = MockMvcBuilders
-      .standaloneSetup(new CredentialsController(accountLookupService:  accountLookupService, allowedAccountsSupport: allowedAccountsSupport))
+      .standaloneSetup(new CredentialsController(
+        accountLookupService:  accountLookupService,
+        allowedAccountsSupport: allowedAccountsSupport,
+        clouddriverService: clouddriverService
+      ))
       .setContentNegotiationManager(contentNegotiationManagerFactoryBean.build())
       .build()
   }
@@ -78,4 +82,39 @@ class CredentialsControllerSpec extends Specification {
     "test"     || "test"
     "test.com" || "test.com"
   }
+
+  @Unroll
+  void "listing credentials by type should succeed when optional arguments are not provided"() {
+    when:
+    MockHttpServletResponse response = mockMvc.perform(get("/credentials/type/${accountType}")
+      .accept(MediaType.APPLICATION_JSON)).andReturn().response
+
+    then:
+    response.status == 200
+    1 * clouddriverService.getAccountDefinitionsByType(accountType, _, _)
+
+    where:
+    accountType | _
+    "type1"     | _
+    "type2"     | _
+  }
+
+  @Unroll
+  void "listing credentials by type should succeed when optional arguments are provided"() {
+    when:
+    MockHttpServletResponse response = mockMvc.perform(
+      get("/credentials/type/${accountType}")
+      .param("limit", "${limit}")
+      .param("startingAccountName", startingAccountName)
+      .accept(MediaType.APPLICATION_JSON)).andReturn().response
+
+    then:
+    response.status == 200
+    1 * clouddriverService.getAccountDefinitionsByType(accountType, limit, startingAccountName)
+
+    where:
+    accountType | limit | startingAccountName
+    "type1"     | 2     | "account1"
+    "type2"     | 500   | "account2"
+  }
 }