diff options
author | Martin Polden <mpolden@mpolden.no> | 2019-07-04 10:48:07 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-07-04 10:48:07 +0200 |
commit | 8c2a1b931b3b54cd076665c4a5aeb986bac2d5e7 (patch) | |
tree | 04ccd75a109507911ef0303fb1ea289cd7405bb4 | |
parent | 7f00f2354a3a3c390f86ea56e8f70cce86ff959f (diff) | |
parent | b8aed836516a9867474b15fbe2961c99171606ef (diff) |
Merge pull request #9950 from vespa-engine/freva/return-response
Return better JSON responses
-rw-r--r-- | controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java | 26 | ||||
-rw-r--r-- | controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/EmptyResponse.java (renamed from controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/EmptyJsonResponse.java) | 7 | ||||
-rw-r--r-- | controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java | 8 | ||||
-rw-r--r-- | controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiHandler.java | 7 | ||||
-rw-r--r-- | controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java | 32 | ||||
-rw-r--r-- | controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java | 2 |
6 files changed, 33 insertions, 49 deletions
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index dce7d2e68ac..9c320df2f6c 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -29,9 +29,7 @@ import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.NotExistsException; import com.yahoo.vespa.hosted.controller.api.ActivateResult; -import com.yahoo.vespa.hosted.controller.api.application.v4.ApplicationResource; import com.yahoo.vespa.hosted.controller.api.application.v4.EnvironmentResource; -import com.yahoo.vespa.hosted.controller.api.application.v4.TenantResource; import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions; import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus; import com.yahoo.vespa.hosted.controller.api.application.v4.model.configserverbindings.RefeedAction; @@ -68,7 +66,6 @@ import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse; import com.yahoo.vespa.hosted.controller.restapi.MessageResponse; import com.yahoo.vespa.hosted.controller.restapi.ResourceResponse; import com.yahoo.vespa.hosted.controller.restapi.SlimeJsonResponse; -import com.yahoo.vespa.hosted.controller.restapi.StringResponse; import com.yahoo.vespa.hosted.controller.security.AccessControlRequests; import com.yahoo.vespa.hosted.controller.security.Credentials; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; @@ -267,7 +264,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { private HttpResponse handleOPTIONS() { // We implement this to avoid redirect loops on OPTIONS requests from browsers, but do not really bother // spelling out the methods supported at each path, which we should - EmptyJsonResponse response = new EmptyJsonResponse(); + EmptyResponse response = new EmptyResponse(); response.headers().put("Allow", "GET,PUT,POST,PATCH,DELETE,OPTIONS"); return response; } @@ -945,12 +942,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { Optional<Hostname> hostname = Optional.ofNullable(request.getProperty("hostname")).map(Hostname::new); controller.applications().restart(deploymentId, hostname); - // TODO: Change to return JSON - return new StringResponse("Requested restart of " + path(TenantResource.API_PATH, tenantName, - ApplicationResource.API_PATH, applicationName, - EnvironmentResource.API_PATH, environment, - "region", region, - "instance", instanceName)); + return new MessageResponse("Requested restart of " + deploymentId); } private HttpResponse jobDeploy(ApplicationId id, JobType type, HttpRequest request) { @@ -1097,21 +1089,17 @@ public class ApplicationApiHandler extends LoggingRequestHandler { ? Optional.empty() : Optional.of(accessControlRequests.credentials(id.tenant(), toSlime(request.getData()).get(), request.getJDiscRequest())); controller.applications().deleteApplication(id, credentials); - return new EmptyJsonResponse(); // TODO: Replicates current behavior but should return a message response instead + return new MessageResponse("Deleted application " + id); } private HttpResponse deactivate(String tenantName, String applicationName, String instanceName, String environment, String region, HttpRequest request) { Application application = controller.applications().require(ApplicationId.from(tenantName, applicationName, instanceName)); // Attempt to deactivate application even if the deployment is not known by the controller - controller.applications().deactivate(application.id(), ZoneId.from(environment, region)); - - // TODO: Change to return JSON - return new StringResponse("Deactivated " + path(TenantResource.API_PATH, tenantName, - ApplicationResource.API_PATH, applicationName, - "instance", instanceName, - EnvironmentResource.API_PATH, environment, - "region", region)); + DeploymentId deploymentId = new DeploymentId(application.id(), ZoneId.from(environment, region)); + controller.applications().deactivate(deploymentId.applicationId(), deploymentId.zoneId()); + + return new MessageResponse("Deactivated " + deploymentId); } private HttpResponse notifyJobCompletion(String tenant, String application, HttpRequest request) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/EmptyJsonResponse.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/EmptyResponse.java index be3222cc1a8..e343615f066 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/EmptyJsonResponse.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/EmptyResponse.java @@ -8,16 +8,13 @@ import java.io.OutputStream; /** * @author bratseth */ -public class EmptyJsonResponse extends HttpResponse { +public class EmptyResponse extends HttpResponse { - public EmptyJsonResponse() { + public EmptyResponse() { super(200); } @Override public void render(OutputStream stream) {} - @Override - public String getContentType() { return "application/json"; } - } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java index 978b7e4397d..44b67a186b8 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java @@ -8,18 +8,18 @@ import com.yahoo.config.provision.HostName; import com.yahoo.container.jdisc.HttpRequest; import com.yahoo.container.jdisc.HttpResponse; import com.yahoo.container.jdisc.LoggingRequestHandler; +import com.yahoo.restapi.Path; import com.yahoo.slime.Cursor; import com.yahoo.slime.Slime; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.application.JobList; import com.yahoo.vespa.hosted.controller.application.JobStatus; -import com.yahoo.vespa.hosted.controller.versions.VespaVersion; import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse; import com.yahoo.vespa.hosted.controller.restapi.SlimeJsonResponse; import com.yahoo.vespa.hosted.controller.restapi.Uri; -import com.yahoo.vespa.hosted.controller.restapi.application.EmptyJsonResponse; -import com.yahoo.restapi.Path; +import com.yahoo.vespa.hosted.controller.restapi.application.EmptyResponse; +import com.yahoo.vespa.hosted.controller.versions.VespaVersion; import com.yahoo.yolean.Exceptions; import java.util.Optional; @@ -71,7 +71,7 @@ public class DeploymentApiHandler extends LoggingRequestHandler { private HttpResponse handleOPTIONS() { // We implement this to avoid redirect loops on OPTIONS requests from browsers, but do not really bother // spelling out the methods supported at each path, which we should - EmptyJsonResponse response = new EmptyJsonResponse(); + EmptyResponse response = new EmptyResponse(); response.headers().put("Allow", "GET,OPTIONS"); return response; } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiHandler.java index 5ef997b6d55..7a76f13392d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiHandler.java @@ -13,20 +13,19 @@ import com.yahoo.slime.Cursor; import com.yahoo.slime.Inspector; import com.yahoo.slime.Slime; import com.yahoo.vespa.config.SlimeUtils; +import com.yahoo.vespa.hosted.controller.api.integration.user.Roles; import com.yahoo.vespa.hosted.controller.api.integration.user.UserId; import com.yahoo.vespa.hosted.controller.api.integration.user.UserManagement; -import com.yahoo.vespa.hosted.controller.api.integration.user.Roles; import com.yahoo.vespa.hosted.controller.api.role.Role; import com.yahoo.vespa.hosted.controller.api.role.RoleDefinition; import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse; import com.yahoo.vespa.hosted.controller.restapi.MessageResponse; import com.yahoo.vespa.hosted.controller.restapi.SlimeJsonResponse; -import com.yahoo.vespa.hosted.controller.restapi.application.EmptyJsonResponse; +import com.yahoo.vespa.hosted.controller.restapi.application.EmptyResponse; import com.yahoo.yolean.Exceptions; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -99,7 +98,7 @@ public class UserApiHandler extends LoggingRequestHandler { } private HttpResponse handleOPTIONS() { - EmptyJsonResponse response = new EmptyJsonResponse(); + EmptyResponse response = new EmptyResponse(); response.headers().put("Allow", "GET,PUT,POST,PATCH,DELETE,OPTIONS"); return response; } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index a0acbc625f7..29931a1f626 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -241,7 +241,7 @@ public class ApplicationApiTest extends ControllerContainerTest { new File("deploy-result.json")); tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/test/region/us-east-1/instance/instance1", DELETE) .screwdriverIdentity(SCREWDRIVER_ID), - "Deactivated tenant/tenant1/application/application1/instance/instance1/environment/test/region/us-east-1"); + "{\"message\":\"Deactivated tenant1.application1.instance1 in test.us-east-1\"}"); controllerTester.jobCompletion(JobType.systemTest) .application(id) @@ -255,7 +255,7 @@ public class ApplicationApiTest extends ControllerContainerTest { new File("deploy-result.json")); tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/staging/region/us-east-3/instance/instance1", DELETE) .screwdriverIdentity(SCREWDRIVER_ID), - "Deactivated tenant/tenant1/application/application1/instance/instance1/environment/staging/region/us-east-3"); + "{\"message\":\"Deactivated tenant1.application1.instance1 in staging.us-east-3\"}"); controllerTester.jobCompletion(JobType.stagingTest) .application(id) .projectId(screwdriverProjectId) @@ -367,7 +367,7 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant2/application/application2", DELETE) .userIdentity(USER_ID) .oktaAccessToken(OKTA_AT), - ""); + "{\"message\":\"Deleted application tenant2.application2\"}"); // Set version 6.1 to broken to change compile version for. controllerTester.upgrader().overrideConfidence(Version.fromString("6.1"), VespaVersion.Confidence.broken); @@ -480,27 +480,27 @@ public class ApplicationApiTest extends ControllerContainerTest { // POST a 'restart application' command tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1/restart", POST) .userIdentity(USER_ID), - "Requested restart of tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1"); + "{\"message\":\"Requested restart of tenant1.application1.instance1 in prod.us-central-1\"}"); // POST a 'restart application' command tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1/restart", POST) .screwdriverIdentity(SCREWDRIVER_ID), - "Requested restart of tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1"); + "{\"message\":\"Requested restart of tenant1.application1.instance1 in prod.us-central-1\"}"); // POST a 'restart application' in staging environment command tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/staging/region/us-central-1/instance/instance1/restart", POST) .screwdriverIdentity(SCREWDRIVER_ID), - "Requested restart of tenant/tenant1/application/application1/environment/staging/region/us-central-1/instance/instance1"); + "{\"message\":\"Requested restart of tenant1.application1.instance1 in staging.us-central-1\"}"); // POST a 'restart application' in staging test command tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/test/region/us-central-1/instance/instance1/restart", POST) .screwdriverIdentity(SCREWDRIVER_ID), - "Requested restart of tenant/tenant1/application/application1/environment/test/region/us-central-1/instance/instance1"); + "{\"message\":\"Requested restart of tenant1.application1.instance1 in test.us-central-1\"}"); // POST a 'restart application' in staging dev command tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/dev/region/us-central-1/instance/instance1/restart", POST) .userIdentity(USER_ID), - "Requested restart of tenant/tenant1/application/application1/environment/dev/region/us-central-1/instance/instance1"); + "{\"message\":\"Requested restart of tenant1.application1.instance1 in dev.us-central-1\"}"); // POST a 'restart application' command with a host filter (other filters not supported yet) tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1/restart?hostname=host1", POST) @@ -530,18 +530,18 @@ public class ApplicationApiTest extends ControllerContainerTest { // DELETE (deactivate) a deployment - dev tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/dev/region/us-west-1/instance/instance1", DELETE) .userIdentity(USER_ID), - "Deactivated tenant/tenant1/application/application1/instance/instance1/environment/dev/region/us-west-1"); + "{\"message\":\"Deactivated tenant1.application1.instance1 in dev.us-west-1\"}"); // DELETE (deactivate) a deployment - prod tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1", DELETE) .screwdriverIdentity(SCREWDRIVER_ID), - "Deactivated tenant/tenant1/application/application1/instance/instance1/environment/prod/region/us-central-1"); + "{\"message\":\"Deactivated tenant1.application1.instance1 in prod.us-central-1\"}"); // DELETE (deactivate) a deployment is idempotent tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-central-1/instance/instance1", DELETE) .screwdriverIdentity(SCREWDRIVER_ID), - "Deactivated tenant/tenant1/application/application1/instance/instance1/environment/prod/region/us-central-1"); + "{\"message\":\"Deactivated tenant1.application1.instance1 in prod.us-central-1\"}"); // POST an application package to start a deployment to dev tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1/deploy/dev-us-east-1", POST) @@ -661,7 +661,7 @@ public class ApplicationApiTest extends ControllerContainerTest { // DELETE an application tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1", DELETE).userIdentity(USER_ID) .oktaAccessToken(OKTA_AT), - ""); + "{\"message\":\"Deleted application tenant1.application1.instance1\"}"); // DELETE a tenant tester.assertResponse(request("/application/v4/tenant/tenant1", DELETE).userIdentity(USER_ID) .oktaAccessToken(OKTA_AT), @@ -990,7 +990,7 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1", DELETE) .userIdentity(USER_ID) .oktaAccessToken(OKTA_AT), - ""); + "{\"message\":\"Deleted application tenant1.application1.instance1\"}"); // DELETE application again - should produce 404 tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/instance1", DELETE) .oktaAccessToken(OKTA_AT) @@ -1089,7 +1089,7 @@ public class ApplicationApiTest extends ControllerContainerTest { tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", DELETE) .userIdentity(authorizedUser) .oktaAccessToken(OKTA_AT), - "", + "{\"message\":\"Deleted application tenant1.application1\"}", 200); // Updating a tenant for an Athens domain the user is not admin for is disallowed @@ -1528,7 +1528,7 @@ public class ApplicationApiTest extends ControllerContainerTest { new File("deploy-result.json")); tester.assertResponse(request(testPath, DELETE) .screwdriverIdentity(SCREWDRIVER_ID), - "Deactivated " + testPath.replaceFirst("/application/v4/", "")); + "{\"message\":\"Deactivated " + application + " in test.us-east-1\"}"); controllerTester.jobCompletion(JobType.systemTest) .application(application) .projectId(projectId) @@ -1543,7 +1543,7 @@ public class ApplicationApiTest extends ControllerContainerTest { new File("deploy-result.json")); tester.assertResponse(request(stagingPath, DELETE) .screwdriverIdentity(SCREWDRIVER_ID), - "Deactivated " + stagingPath.replaceFirst("/application/v4/", "")); + "{\"message\":\"Deactivated " + application + " in staging.us-east-3\"}"); controllerTester.jobCompletion(JobType.stagingTest) .application(application) .projectId(projectId) diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java index 59f63f0472a..d0e9ae77965 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java @@ -137,7 +137,7 @@ public class UserApiTest extends ControllerContainerCloudTest { // DELETE an application is available to application admins. tester.assertResponse(request("/application/v4/tenant/my-tenant/application/my-app", DELETE) .roles(Set.of(Role.applicationAdmin(id.tenant(), id.application()))), - ""); + "{\"message\":\"Deleted application my-tenant.my-app\"}"); // DELETE a tenant role is available to tenant admins. tester.assertResponse(request("/user/v1/tenant/my-tenant", DELETE) |