From 970dbe45c0241ca86c442ff153e56011cc578539 Mon Sep 17 00:00:00 2001 From: Teryk Bellahsene Date: Fri, 30 Oct 2015 11:04:55 +0100 Subject: [PATCH] SONAR-6916 Enforce some mandatory web service metadata --- .../org/sonar/api/server/ws/WebService.java | 45 +- .../sonar/api/server/ws/WebServiceTest.java | 440 +++++++++++------- 2 files changed, 294 insertions(+), 191 deletions(-) diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/WebService.java b/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/WebService.java index 045d0db25b5..6141bed8d8f 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/WebService.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/server/ws/WebService.java @@ -43,8 +43,13 @@ import org.apache.commons.io.IOUtils; import org.apache.commons.lang.StringUtils; import org.sonar.api.ExtensionPoint; import org.sonar.api.server.ServerSide; +import org.sonar.api.utils.log.Logger; +import org.sonar.api.utils.log.Loggers; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; +import static com.google.common.base.Strings.isNullOrEmpty; +import static java.lang.String.format; /** * Defines a web service. Note that contrary to the deprecated {@link org.sonar.api.web.Webservice} @@ -131,7 +136,7 @@ public interface WebService extends Definable { private void register(NewController newController) { if (controllers.containsKey(newController.path)) { throw new IllegalStateException( - String.format("The web service '%s' is defined multiple times", newController.path)); + format("The web service '%s' is defined multiple times", newController.path)); } controllers.put(newController.path, new Controller(newController)); } @@ -191,7 +196,7 @@ public interface WebService extends Definable { public NewAction createAction(String actionKey) { if (actions.containsKey(actionKey)) { throw new IllegalStateException( - String.format("The action '%s' is defined multiple times in the web service '%s'", actionKey, path)); + format("The action '%s' is defined multiple times in the web service '%s'", actionKey, path)); } NewAction action = new NewAction(actionKey); actions.put(actionKey, action); @@ -207,10 +212,7 @@ public interface WebService extends Definable { private final Map actions; private Controller(NewController newController) { - if (newController.actions.isEmpty()) { - throw new IllegalStateException( - String.format("At least one action must be declared in the web service '%s'", newController.path)); - } + checkState(!newController.actions.isEmpty(), format("At least one action must be declared in the web service '%s'", newController.path)); this.path = newController.path; this.description = newController.description; this.since = newController.since; @@ -327,10 +329,8 @@ public interface WebService extends Definable { } public NewParam createParam(String paramKey) { - if (newParams.containsKey(paramKey)) { - throw new IllegalStateException( - String.format("The parameter '%s' is defined multiple times in the action '%s'", paramKey, key)); - } + checkState(!newParams.containsKey(paramKey), + format("The parameter '%s' is defined multiple times in the action '%s'", paramKey, key)); NewParam newParam = new NewParam(paramKey); newParams.put(paramKey, newParam); return newParam; @@ -381,7 +381,7 @@ public interface WebService extends Definable { * The fields must be in the plural form (ex: "names", "keys") */ public NewAction addSearchQuery(String exampleValue, String... pluralFields) { - String actionDescription = String.format("Limit search to %s that contain the supplied string.", Joiner.on(" or ").join(pluralFields)); + String actionDescription = format("Limit search to %s that contain the supplied string.", Joiner.on(" or ").join(pluralFields)); createParam(Param.TEXT_QUERY) .setDescription(actionDescription) .setExampleValue(exampleValue); @@ -420,6 +420,8 @@ public interface WebService extends Definable { @Immutable class Action { + private static final Logger LOGGER = Loggers.get(Action.class); + private final String key; private final String deprecatedKey; private final String path; @@ -435,19 +437,20 @@ public interface WebService extends Definable { private Action(Controller controller, NewAction newAction) { this.key = newAction.key; this.deprecatedKey = newAction.deprecatedKey; - this.path = String.format("%s/%s", controller.path(), key); + this.path = format("%s/%s", controller.path(), key); this.description = newAction.description; - this.since = StringUtils.defaultIfBlank(newAction.since, controller.since); + this.since = newAction.since; this.deprecatedSince = newAction.deprecatedSince; this.post = newAction.post; this.isInternal = newAction.isInternal; this.responseExample = newAction.responseExample; - - if (newAction.handler == null) { - throw new IllegalArgumentException("RequestHandler is not set on action " + path); - } this.handler = newAction.handler; + checkState(this.handler != null, "RequestHandler is not set on action " + path); + logWarningIf(isNullOrEmpty(this.description), "Description is not set on action " + path); + logWarningIf(isNullOrEmpty(this.since), "Since is not set on action " + path); + logWarningIf(!this.post && this.responseExample == null, "The response example is not set on action " + path); + ImmutableMap.Builder paramsBuilder = ImmutableMap.builder(); for (NewParam newParam : newAction.newParams.values()) { paramsBuilder.put(newParam.key, new Param(this, newParam)); @@ -455,6 +458,12 @@ public interface WebService extends Definable { this.params = paramsBuilder.build(); } + private static void logWarningIf(boolean condition, String message) { + if (condition) { + LOGGER.warn(message); + } + } + public String key() { return key; } @@ -713,7 +722,7 @@ public interface WebService extends Definable { this.required = newParam.required; this.possibleValues = newParam.possibleValues; if (required && defaultValue != null) { - throw new IllegalArgumentException(String.format("Default value must not be set on parameter '%s?%s' as it's marked as required", action, key)); + throw new IllegalArgumentException(format("Default value must not be set on parameter '%s?%s' as it's marked as required", action, key)); } } diff --git a/sonar-plugin-api/src/test/java/org/sonar/api/server/ws/WebServiceTest.java b/sonar-plugin-api/src/test/java/org/sonar/api/server/ws/WebServiceTest.java index bdca29a4702..7e22bdbb037 100644 --- a/sonar-plugin-api/src/test/java/org/sonar/api/server/ws/WebServiceTest.java +++ b/sonar-plugin-api/src/test/java/org/sonar/api/server/ws/WebServiceTest.java @@ -24,8 +24,14 @@ import java.net.URL; import java.util.Arrays; import java.util.Collections; import org.apache.commons.lang.StringUtils; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.sonar.api.rule.RuleStatus; +import org.sonar.api.server.ws.WebService.NewAction; +import org.sonar.api.server.ws.WebService.NewController; +import org.sonar.api.utils.log.LogTester; +import org.sonar.api.utils.log.LoggerLevel; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; @@ -33,50 +39,10 @@ import static org.mockito.Mockito.mock; public class WebServiceTest { - static class MetricWs implements WebService { - boolean showCalled = false; - boolean createCalled = false; - - @Override - public void define(Context context) { - NewController newController = context.createController("api/metric") - .setDescription("Metrics") - .setSince("3.2"); - - newController.createAction("show") - .setDescription("Show metric") - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - show(request, response); - } - }); - - newController.createAction("create") - .setDescription("Create metric") - .setSince("4.1") - .setDeprecatedSince("5.3") - .setPost(true) - .setInternal(true) - .setResponseExample(getClass().getResource("/org/sonar/api/server/ws/WebServiceTest/response-example.txt")) - .setHandler(new RequestHandler() { - @Override - public void handle(Request request, Response response) { - create(request, response); - } - }); - - newController.done(); - } - - void show(Request request, Response response) { - showCalled = true; - } - - void create(Request request, Response response) { - createCalled = true; - } - } + @Rule + public ExpectedException expectedException = ExpectedException.none(); + @Rule + public LogTester logTester = new LogTester(); WebService.Context context = new WebService.Context(); @@ -104,12 +70,12 @@ public class WebServiceTest { assertThat(showAction.key()).isEqualTo("show"); assertThat(showAction.description()).isEqualTo("Show metric"); assertThat(showAction.handler()).isNotNull(); - assertThat(showAction.responseExample()).isNull(); - assertThat(showAction.responseExampleFormat()).isNull(); - assertThat(showAction.responseExampleAsString()).isNull(); + assertThat(showAction.responseExample()).isNotNull(); + assertThat(showAction.responseExampleFormat()).isNotEmpty(); + assertThat(showAction.responseExampleAsString()).isNotEmpty(); assertThat(showAction.deprecatedSince()).isNull(); // same as controller - assertThat(showAction.since()).isEqualTo("3.2"); + assertThat(showAction.since()).isEqualTo("4.2"); assertThat(showAction.isPost()).isFalse(); assertThat(showAction.isInternal()).isFalse(); assertThat(showAction.path()).isEqualTo("api/metric/show"); @@ -126,117 +92,104 @@ public class WebServiceTest { @Test public void fail_if_duplicated_ws_keys() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("The web service 'api/metric' is defined multiple times"); + MetricWs metricWs = new MetricWs(); metricWs.define(context); - try { - new WebService() { - @Override - public void define(Context context) { - NewController newController = context.createController("api/metric"); - newController.createAction("delete"); - newController.done(); - } - }.define(context); - fail(); - } catch (IllegalStateException e) { - assertThat(e).hasMessage("The web service 'api/metric' is defined multiple times"); - } + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/metric"); + newDefaultAction(newController, "delete"); + newController.done(); + } + }.define(context); } @Test public void fail_if_no_action_handler() { - try { - new WebService() { - @Override - public void define(Context context) { - NewController controller = context.createController("rule"); - controller.createAction("show"); - controller.done(); - } - }.define(context); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("RequestHandler is not set on action rule/show"); - } + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("RequestHandler is not set on action rule/show"); + + new WebService() { + @Override + public void define(Context context) { + NewController controller = context.createController("rule"); + newDefaultAction(controller, "show") + .setHandler(null); + controller.done(); + } + }.define(context); } @Test public void fail_if_duplicated_action_keys() { - try { - new WebService() { - @Override - public void define(Context context) { - NewController newController = context.createController("rule"); - newController.createAction("create"); - newController.createAction("delete"); - newController.createAction("delete"); - newController.done(); - } - }.define(context); - fail(); - } catch (IllegalStateException e) { - assertThat(e).hasMessage("The action 'delete' is defined multiple times in the web service 'rule'"); - } + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("The action 'delete' is defined multiple times in the web service 'rule'"); + + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("rule"); + newDefaultAction(newController, "create"); + newDefaultAction(newController, "delete"); + newDefaultAction(newController, "delete"); + newController.done(); + } + }.define(context); } @Test public void fail_if_no_actions() { - try { - new WebService() { - @Override - public void define(Context context) { - context.createController("rule").done(); - } - }.define(context); - fail(); - } catch (IllegalStateException e) { - assertThat(e).hasMessage("At least one action must be declared in the web service 'rule'"); - } + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("At least one action must be declared in the web service 'rule'"); + + new WebService() { + @Override + public void define(Context context) { + context.createController("rule").done(); + } + }.define(context); } @Test public void fail_if_no_controller_path() { - try { - new WebService() { - @Override - public void define(Context context) { - context.createController(null).done(); - } - }.define(context); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("WS controller path must not be empty"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("WS controller path must not be empty"); + + new WebService() { + @Override + public void define(Context context) { + context.createController(null).done(); + } + }.define(context); } @Test public void controller_path_must_not_start_with_slash() { - try { - new WebService() { - @Override - public void define(Context context) { - context.createController("/hello").done(); - } - }.define(context); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("WS controller path must not start or end with slash: /hello"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("WS controller path must not start or end with slash: /hello"); + + new WebService() { + @Override + public void define(Context context) { + context.createController("/hello").done(); + } + }.define(context); } @Test public void controller_path_must_not_end_with_slash() { - try { - new WebService() { - @Override - public void define(Context context) { - context.createController("hello/").done(); - } - }.define(context); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("WS controller path must not start or end with slash: hello/"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("WS controller path must not start or end with slash: hello/"); + + new WebService() { + @Override + public void define(Context context) { + context.createController("hello/").done(); + } + }.define(context); } @Test @@ -259,7 +212,7 @@ public class WebServiceTest { @Override public void define(Context context) { NewController newController = context.createController("api/rule"); - NewAction newAction = newController.createAction("create").setHandler(mock(RequestHandler.class)); + NewAction newAction = newDefaultAction(newController, "create"); newAction .createParam("key") .setDescription("Key of the new rule"); @@ -311,7 +264,7 @@ public class WebServiceTest { @Override public void define(Context context) { NewController newController = context.createController("api/rule"); - NewAction create = newController.createAction("create").setHandler(mock(RequestHandler.class)); + NewAction create = newDefaultAction(newController, "create"); create.createParam("status") .setDefaultValue(RuleStatus.BETA) .setPossibleValues(RuleStatus.BETA, RuleStatus.READY) @@ -339,7 +292,7 @@ public class WebServiceTest { @Override public void define(Context context) { NewController newController = context.createController("api/rule"); - NewAction create = newController.createAction("create").setHandler(mock(RequestHandler.class)); + NewAction create = newDefaultAction(newController, "create"); create.createParam("status") .setDefaultValue(null) .setPossibleValues(Collections.emptyList()) @@ -363,7 +316,7 @@ public class WebServiceTest { @Override public void define(Context context) { NewController newController = context.createController("api/rule"); - NewAction create = newController.createAction("create").setHandler(mock(RequestHandler.class)); + NewAction create = newDefaultAction(newController, "create"); create.createParam("status") .setPossibleValues(Collections.emptyList()); newController.done(); @@ -377,39 +330,34 @@ public class WebServiceTest { @Test public void fail_if_required_param_has_default_value() { - try { - new WebService() { - @Override - public void define(Context context) { - NewController controller = context.createController("api/rule"); - NewAction action = controller.createAction("create").setHandler(mock(RequestHandler.class)); - action.createParam("key").setRequired(true).setDefaultValue("abc"); - controller.done(); - } - }.define(context); - fail(); - } catch (IllegalArgumentException e) { - assertThat(e).hasMessage("Default value must not be set on parameter 'api/rule/create?key' as it's marked as required"); - } + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Default value must not be set on parameter 'api/rule/create?key' as it's marked as required"); + new WebService() { + @Override + public void define(Context context) { + NewController controller = context.createController("api/rule"); + NewAction action = newDefaultAction(controller, "create"); + action.createParam("key").setRequired(true).setDefaultValue("abc"); + controller.done(); + } + }.define(context); } @Test public void fail_if_duplicated_action_parameters() { - try { - new WebService() { - @Override - public void define(Context context) { - NewController controller = context.createController("api/rule"); - NewAction action = controller.createAction("create").setHandler(mock(RequestHandler.class)); - action.createParam("key"); - action.createParam("key"); - controller.done(); - } - }.define(context); - fail(); - } catch (IllegalStateException e) { - assertThat(e).hasMessage("The parameter 'key' is defined multiple times in the action 'create'"); - } + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("The parameter 'key' is defined multiple times in the action 'create'"); + + new WebService() { + @Override + public void define(Context context) { + NewController controller = context.createController("api/rule"); + NewAction action = newDefaultAction(controller, "create"); + action.createParam("key"); + action.createParam("key"); + controller.done(); + } + }.define(context); } @Test @@ -418,8 +366,8 @@ public class WebServiceTest { @Override public void define(Context context) { NewController newController = context.createController("api/rule"); - newController.createAction("create").setInternal(true).setHandler(mock(RequestHandler.class)); - newController.createAction("update").setInternal(true).setHandler(mock(RequestHandler.class)); + newDefaultAction(newController, "create").setInternal(true); + newDefaultAction(newController, "update").setInternal(true); newController.done(); } }.define(context); @@ -445,12 +393,8 @@ public class WebServiceTest { public void define(Context context) { try { NewController controller = context.createController("foo"); - controller - .createAction("bar") - .setHandler(mock(RequestHandler.class)) - .setResponseExample(new URL("file:/does/not/exist")); + newDefaultAction(controller, "bar").setResponseExample(new URL("file:/does/not/exist")); controller.done(); - } catch (MalformedURLException e) { e.printStackTrace(); } @@ -466,4 +410,154 @@ public class WebServiceTest { assertThat(e).hasMessage("Fail to load file:/does/not/exist"); } } + + @Test + public void post_action_without_response_example() { + WebService ws = new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/rule"); + newDefaultAction(newController, "list").setPost(true).setResponseExample(null); + newController.done(); + } + }; + ws.define(context); + + assertThat(logTester.logs(LoggerLevel.WARN)) + .doesNotContain("The response example is not set on action api/rule/list"); + } + + @Test + public void fail_if_get_and_no_response_example() { + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/rule"); + newDefaultAction(newController, "list").setResponseExample(null); + newController.done(); + } + }.define(context); + + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("The response example is not set on action api/rule/list"); + } + + @Test + public void log_if_since_on_an_action_is_empty() { + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/rule"); + newDefaultAction(newController, "list") + .setSince(""); + newController.done(); + } + }.define(context); + + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Since is not set on action api/rule/list"); + } + + @Test + public void log_if_since_on_an_action_is_null() { + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/rule"); + newDefaultAction(newController, "list") + .setSince(null); + newController.done(); + } + }.define(context); + + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Since is not set on action api/rule/list"); + } + + @Test + public void log_if_action_description_is_empty() { + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/rule"); + newDefaultAction(newController, "list") + .setDescription(""); + newController.done(); + } + }.define(context); + + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Description is not set on action api/rule/list"); + } + + @Test + public void log_if_action_description_is_null() { + new WebService() { + @Override + public void define(Context context) { + NewController newController = context.createController("api/rule"); + newDefaultAction(newController, "list") + .setDescription(null); + newController.done(); + } + }.define(context); + + assertThat(logTester.logs(LoggerLevel.WARN)) + .contains("Description is not set on action api/rule/list"); + } + + static class MetricWs implements WebService { + boolean showCalled = false; + boolean createCalled = false; + + @Override + public void define(Context context) { + NewController newController = context.createController("api/metric") + .setDescription("Metrics") + .setSince("3.2"); + + newController.createAction("show") + .setDescription("Show metric") + .setSince("4.2") + .setResponseExample(getClass().getResource("WebServiceTest/response-example.txt")) + .setHandler(new RequestHandler() { + @Override + public void handle(Request request, Response response) { + show(request, response); + } + }); + + newController.createAction("create") + .setDescription("Create metric") + .setSince("4.1") + .setDeprecatedSince("5.3") + .setPost(true) + .setInternal(true) + .setResponseExample(getClass().getResource("WebServiceTest/response-example.txt")) + .setHandler(new RequestHandler() { + @Override + public void handle(Request request, Response response) { + create(request, response); + } + }); + + newController.done(); + } + + void show(Request request, Response response) { + showCalled = true; + } + + void create(Request request, Response response) { + createCalled = true; + } + } + + private NewAction newDefaultAction(NewController controller, String actionKey) { + return controller.createAction(actionKey) + .setDescription("default description") + .setSince("5.3") + .setResponseExample(getClass().getResource("WebServiceTest/response-example.txt")) + .setHandler(mock(RequestHandler.class)); + } } -- 2.39.5