From 09a4882a4c196017e7982d2f6f5988d9d1c065bd Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 19 Oct 2016 16:58:17 +0200 Subject: [PATCH] Remove Notification Accounts abstraction (elastic/elasticsearch#3811) This change is a first step towards a real abstraction on top of all the notification services. There are a bunch of followup changes coming for this that will remove most of the classes in here but this is a first small step to actually have a notification service interface. Original commit: elastic/x-pack-elasticsearch@e14abf8a8b9dfe996cb13b5d8d92bad59b31dbe6 --- .../notification/NotificationService.java | 82 +++++++++++++++++++ .../xpack/notification/email/Accounts.java | 64 --------------- .../notification/email/EmailService.java | 24 +++--- .../notification/hipchat/HipChatAccounts.java | 69 ---------------- .../notification/hipchat/HipChatService.java | 38 ++++----- .../pagerduty/PagerDutyAccounts.java | 63 -------------- .../pagerduty/PagerDutyService.java | 22 ++--- .../notification/slack/SlackAccounts.java | 63 -------------- .../notification/slack/SlackService.java | 29 ++----- .../hipchat/ExecutableHipChatAction.java | 5 +- .../pagerduty/ExecutablePagerDutyAction.java | 5 +- .../actions/slack/ExecutableSlackAction.java | 4 +- .../xpack/watcher/condition/Condition.java | 2 +- .../notification/email/AccountsTests.java | 66 ++++++++------- .../notification/email/EmailServiceTests.java | 15 ++-- .../notification/email/ProfileTests.java | 12 ++- .../hipchat/HipChatAccountsTests.java | 78 ++++++++++-------- .../hipchat/HipChatServiceTests.java | 10 +-- .../pagerduty/PagerDutyAccountsTests.java | 77 +++++++++-------- .../slack/SlackAccountsTests.java | 67 ++++++++------- 20 files changed, 303 insertions(+), 492 deletions(-) create mode 100644 elasticsearch/src/main/java/org/elasticsearch/xpack/notification/NotificationService.java delete mode 100644 elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/Accounts.java delete mode 100644 elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccounts.java delete mode 100644 elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccounts.java delete mode 100644 elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackAccounts.java diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/NotificationService.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/NotificationService.java new file mode 100644 index 00000000000..89a4894a4e0 --- /dev/null +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/NotificationService.java @@ -0,0 +1,82 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.notification; + +import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.component.AbstractComponent; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.function.BiFunction; + +/** + * Basic notification service + */ +public abstract class NotificationService extends AbstractComponent { + + // both are guarded by this + private Map accounts; + protected Account defaultAccount; + + public NotificationService(Settings settings) { + super(settings); + } + + protected synchronized void setAccountSetting(Settings settings) { + Tuple, Account> accounts = buildAccounts(settings, this::createAccount); + this.accounts = Collections.unmodifiableMap(accounts.v1()); + this.defaultAccount = accounts.v2(); + } + + protected abstract Account createAccount(String name, Settings accountSettings); + + public Account getAccount(String name) { + // note this is not final since we mock it in tests and that causes + // trouble since final methods can't be mocked... + final Map accounts; + final Account defaultAccount; + synchronized (this) { // must read under sync block otherwise it might be inconsistent + accounts = this.accounts; + defaultAccount = this.defaultAccount; + } + Account theAccount = accounts.getOrDefault(name, defaultAccount); + if (theAccount == null) { + throw new IllegalArgumentException("no account found for name: [" + name + "]"); + } + return theAccount; + } + + private Tuple, A> buildAccounts(Settings settings, BiFunction accountFactory) { + Settings accountsSettings = settings.getAsSettings("account"); + Map accounts = new HashMap<>(); + for (String name : accountsSettings.names()) { + Settings accountSettings = accountsSettings.getAsSettings(name); + A account = accountFactory.apply(name, accountSettings); + accounts.put(name, account); + } + + final String defaultAccountName = settings.get("default_account"); + A defaultAccount; + if (defaultAccountName == null) { + if (accounts.isEmpty()) { + defaultAccount = null; + } else { + A account = accounts.values().iterator().next(); + defaultAccount = account; + + } + } else { + defaultAccount = accounts.get(defaultAccountName); + if (defaultAccount == null) { + throw new SettingsException("could not find default account [" + defaultAccountName + "]"); + } + } + return new Tuple<>(accounts, defaultAccount); + } +} diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/Accounts.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/Accounts.java deleted file mode 100644 index 7bdc2baf549..00000000000 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/Accounts.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.notification.email; - -import org.apache.logging.log4j.Logger; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsException; -import org.elasticsearch.xpack.security.crypto.CryptoService; - -import java.util.HashMap; -import java.util.Map; - -public class Accounts { - - private final String defaultAccountName; - private final Map accounts; - - public Accounts(Settings settings, CryptoService cryptoService, Logger logger) { - Settings accountsSettings = settings.getAsSettings("account"); - accounts = new HashMap<>(); - for (String name : accountsSettings.names()) { - Account.Config config = new Account.Config(name, accountsSettings.getAsSettings(name)); - Account account = new Account(config, cryptoService, logger); - accounts.put(name, account); - } - - String defaultAccountName = settings.get("default_account"); - if (defaultAccountName == null) { - if (accounts.isEmpty()) { - this.defaultAccountName = null; - } else { - Account account = accounts.values().iterator().next(); - logger.info("default account set to [{}]", account.name()); - this.defaultAccountName = account.name(); - } - } else if (!accounts.containsKey(defaultAccountName)) { - throw new SettingsException("could not find default email account [" + defaultAccountName + "]"); - } else { - this.defaultAccountName = defaultAccountName; - } - } - - /** - * Returns the account associated with the given name. If there is not such account, {@code null} is returned. - * If the given name is {@code null}, the default account will be returned. - * - * @param name The name of the requested account - * @return The account associated with the given name. - * @throws IllegalStateException if the name is null and the default account is null. - */ - public Account account(String name) throws IllegalStateException { - if (name == null) { - if (defaultAccountName == null) { - throw new IllegalStateException("cannot find default email account as no accounts have been configured"); - } - name = defaultAccountName; - } - return accounts.get(name); - } - -} diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/EmailService.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/EmailService.java index 0a4246d9719..179757d9285 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/EmailService.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/email/EmailService.java @@ -5,12 +5,11 @@ */ package org.elasticsearch.xpack.notification.email; -import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.xpack.notification.NotificationService; import org.elasticsearch.xpack.security.crypto.CryptoService; import javax.mail.MessagingException; @@ -18,31 +17,32 @@ import javax.mail.MessagingException; /** * A component to store email credentials and handle sending email notifications. */ -public class EmailService extends AbstractComponent { +public class EmailService extends NotificationService { private final CryptoService cryptoService; public static final Setting EMAIL_ACCOUNT_SETTING = Setting.groupSetting("xpack.notification.email.", Setting.Property.Dynamic, Setting.Property.NodeScope); - private volatile Accounts accounts; - public EmailService(Settings settings, @Nullable CryptoService cryptoService, ClusterSettings clusterSettings) { super(settings); this.cryptoService = cryptoService; - clusterSettings.addSettingsUpdateConsumer(EMAIL_ACCOUNT_SETTING, this::setEmailAccountSettings); - setEmailAccountSettings(EMAIL_ACCOUNT_SETTING.get(settings)); + clusterSettings.addSettingsUpdateConsumer(EMAIL_ACCOUNT_SETTING, this::setAccountSetting); + setAccountSetting(EMAIL_ACCOUNT_SETTING.get(settings)); } - private void setEmailAccountSettings(Settings settings) { - this.accounts = createAccounts(settings, logger); + @Override + protected Account createAccount(String name, Settings accountSettings) { + Account.Config config = new Account.Config(name, accountSettings); + return new Account(config, cryptoService, logger); } + public EmailSent send(Email email, Authentication auth, Profile profile) throws MessagingException { return send(email, auth, profile, (String) null); } public EmailSent send(Email email, Authentication auth, Profile profile, String accountName) throws MessagingException { - Account account = accounts.account(accountName); + Account account = getAccount(accountName); if (account == null) { throw new IllegalArgumentException("failed to send email with subject [" + email.subject() + "] via account [" + accountName + "]. account does not exist"); @@ -61,10 +61,6 @@ public class EmailService extends AbstractComponent { return new EmailSent(account.name(), email); } - protected Accounts createAccounts(Settings settings, Logger logger) { - return new Accounts(settings, cryptoService, logger); - } - public static class EmailSent { private final String account; diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccounts.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccounts.java deleted file mode 100644 index b7ccad4f817..00000000000 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccounts.java +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.notification.hipchat; - -import org.apache.logging.log4j.Logger; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsException; -import org.elasticsearch.xpack.common.http.HttpClient; -import org.elasticsearch.xpack.notification.hipchat.HipChatAccount.Profile; - -import java.util.HashMap; -import java.util.Map; - -public class HipChatAccounts { - - private final Map accounts; - private final String defaultAccountName; - - public HipChatAccounts(Settings settings, HttpClient httpClient, Logger logger) { - HipChatServer defaultServer = new HipChatServer(settings); - Settings accountsSettings = settings.getAsSettings("account"); - accounts = new HashMap<>(); - for (String name : accountsSettings.names()) { - Settings accountSettings = accountsSettings.getAsSettings(name); - Profile profile = Profile.resolve(accountSettings, "profile", null); - if (profile == null) { - throw new SettingsException("missing [profile] setting for hipchat account [" + name + "]"); - } - HipChatAccount account = profile.createAccount(name, accountSettings, defaultServer, httpClient, logger); - accounts.put(name, account); - } - - String defaultAccountName = settings.get("default_account"); - if (defaultAccountName == null) { - if (accounts.isEmpty()) { - this.defaultAccountName = null; - } else { - HipChatAccount account = accounts.values().iterator().next(); - logger.info("default hipchat account set to [{}]", account.name); - this.defaultAccountName = account.name; - } - } else if (!accounts.containsKey(defaultAccountName)) { - throw new SettingsException("could not find default hipchat account [" + defaultAccountName + "]"); - } else { - this.defaultAccountName = defaultAccountName; - } - } - - /** - * Returns the account associated with the given name. If there is not such account, {@code null} is returned. - * If the given name is {@code null}, the default account will be returned. - * - * @param name The name of the requested account - * @return The account associated with the given name, or {@code null} when requested an unkonwn account. - * @throws IllegalStateException if the name is null and the default account is null. - */ - public HipChatAccount account(String name) throws IllegalStateException { - if (name == null) { - if (defaultAccountName == null) { - throw new IllegalStateException("cannot find default hipchat account as no accounts have been configured"); - } - name = defaultAccountName; - } - return accounts.get(name); - } -} diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatService.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatService.java index c809ee9b889..1e5a004922b 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatService.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/hipchat/HipChatService.java @@ -5,46 +5,42 @@ */ package org.elasticsearch.xpack.notification.hipchat; -import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.xpack.common.http.HttpClient; +import org.elasticsearch.xpack.notification.NotificationService; /** * A component to store hipchat credentials. */ -public class HipChatService extends AbstractComponent { +public class HipChatService extends NotificationService { private final HttpClient httpClient; - private volatile HipChatAccounts accounts; public static final Setting HIPCHAT_ACCOUNT_SETTING = Setting.groupSetting("xpack.notification.hipchat.", Setting.Property.Dynamic, Setting.Property.NodeScope); + private HipChatServer defaultServer; public HipChatService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { super(settings); this.httpClient = httpClient; - clusterSettings.addSettingsUpdateConsumer(HIPCHAT_ACCOUNT_SETTING, this::setHipchatAccountSetting); - setHipchatAccountSetting(HIPCHAT_ACCOUNT_SETTING.get(settings)); + clusterSettings.addSettingsUpdateConsumer(HIPCHAT_ACCOUNT_SETTING, this::setAccountSetting); + setAccountSetting(HIPCHAT_ACCOUNT_SETTING.get(settings)); } - private void setHipchatAccountSetting(Settings setting) { - accounts = new HipChatAccounts(setting, httpClient, logger); + @Override + protected synchronized void setAccountSetting(Settings settings) { + defaultServer = new HipChatServer(settings); + super.setAccountSetting(settings); } - /** - * @return The default hipchat account. - */ - public HipChatAccount getDefaultAccount() { - return accounts.account(null); + @Override + protected HipChatAccount createAccount(String name, Settings accountSettings) { + HipChatAccount.Profile profile = HipChatAccount.Profile.resolve(accountSettings, "profile", null); + if (profile == null) { + throw new SettingsException("missing [profile] setting for hipchat account [" + name + "]"); + } + return profile.createAccount(name, accountSettings, defaultServer, httpClient, logger); } - - /** - * @return The account identified by the given name. If the given name is {@code null} the default - * account will be returned. - */ - public HipChatAccount getAccount(String name) { - return accounts.account(name); - } - } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccounts.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccounts.java deleted file mode 100644 index 49bde3a146e..00000000000 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccounts.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.notification.pagerduty; - -import org.apache.logging.log4j.Logger; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsException; -import org.elasticsearch.xpack.common.http.HttpClient; - -import java.util.HashMap; -import java.util.Map; - -public class PagerDutyAccounts { - - private final Map accounts; - private final String defaultAccountName; - - public PagerDutyAccounts(Settings serviceSettings, HttpClient httpClient, Logger logger) { - Settings accountsSettings = serviceSettings.getAsSettings("account"); - accounts = new HashMap<>(); - for (String name : accountsSettings.names()) { - Settings accountSettings = accountsSettings.getAsSettings(name); - PagerDutyAccount account = new PagerDutyAccount(name, accountSettings, serviceSettings, httpClient, logger); - accounts.put(name, account); - } - - String defaultAccountName = serviceSettings.get("default_account"); - if (defaultAccountName == null) { - if (accounts.isEmpty()) { - this.defaultAccountName = null; - } else { - PagerDutyAccount account = accounts.values().iterator().next(); - logger.info("default pager duty account set to [{}]", account.name); - this.defaultAccountName = account.name; - } - } else if (!accounts.containsKey(defaultAccountName)) { - throw new SettingsException("could not find default pagerduty account [" + defaultAccountName + "]"); - } else { - this.defaultAccountName = defaultAccountName; - } - } - - /** - * Returns the account associated with the given name. If there is not such account, {@code null} is returned. - * If the given name is {@code null}, the default account will be returned. - * - * @param name The name of the requested account - * @return The account associated with the given name, or {@code null} when requested an unknown account. - * @throws IllegalStateException if the name is null and the default account is null. - */ - public PagerDutyAccount account(String name) throws IllegalStateException { - if (name == null) { - if (defaultAccountName == null) { - throw new IllegalStateException("cannot find default pagerduty account as no accounts have been configured"); - } - name = defaultAccountName; - } - return accounts.get(name); - } -} diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyService.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyService.java index b348625bbb3..f8acc9a0374 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyService.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyService.java @@ -5,40 +5,32 @@ */ package org.elasticsearch.xpack.notification.pagerduty; -import org.elasticsearch.common.component.AbstractComponent; -import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.xpack.common.http.HttpClient; +import org.elasticsearch.xpack.notification.NotificationService; /** * A component to store pagerduty credentials. */ -public class PagerDutyService extends AbstractComponent { +public class PagerDutyService extends NotificationService { public static final Setting PAGERDUTY_ACCOUNT_SETTING = Setting.groupSetting("xpack.notification.pagerduty.", Setting.Property.Dynamic, Setting.Property.NodeScope); private final HttpClient httpClient; - private volatile PagerDutyAccounts accounts; public PagerDutyService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { super(settings); this.httpClient = httpClient; - clusterSettings.addSettingsUpdateConsumer(PAGERDUTY_ACCOUNT_SETTING, this::setPagerDutyAccountSetting); - setPagerDutyAccountSetting(PAGERDUTY_ACCOUNT_SETTING.get(settings)); + clusterSettings.addSettingsUpdateConsumer(PAGERDUTY_ACCOUNT_SETTING, this::setAccountSetting); + setAccountSetting(PAGERDUTY_ACCOUNT_SETTING.get(settings)); } - private void setPagerDutyAccountSetting(Settings settings) { - accounts = new PagerDutyAccounts(settings, httpClient, logger); + @Override + protected PagerDutyAccount createAccount(String name, Settings accountSettings) { + return new PagerDutyAccount(name, accountSettings, accountSettings, httpClient, logger); } - public PagerDutyAccount getDefaultAccount() { - return accounts.account(null); - } - - public PagerDutyAccount getAccount(String name) { - return accounts.account(name); - } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackAccounts.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackAccounts.java deleted file mode 100644 index 77f7ea487f6..00000000000 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackAccounts.java +++ /dev/null @@ -1,63 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.notification.slack; - -import org.apache.logging.log4j.Logger; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.settings.SettingsException; -import org.elasticsearch.xpack.common.http.HttpClient; - -import java.util.HashMap; -import java.util.Map; - -public class SlackAccounts { - - private final Map accounts; - private final String defaultAccountName; - - public SlackAccounts(Settings settings, HttpClient httpClient, Logger logger) { - Settings accountsSettings = settings.getAsSettings("account"); - accounts = new HashMap<>(); - for (String name : accountsSettings.names()) { - Settings accountSettings = accountsSettings.getAsSettings(name); - SlackAccount account = new SlackAccount(name, accountSettings, settings, httpClient, logger); - accounts.put(name, account); - } - - String defaultAccountName = settings.get("default_account"); - if (defaultAccountName == null) { - if (accounts.isEmpty()) { - this.defaultAccountName = null; - } else { - SlackAccount account = accounts.values().iterator().next(); - logger.info("default slack account set to [{}]", account.name); - this.defaultAccountName = account.name; - } - } else if (!accounts.containsKey(defaultAccountName)) { - throw new SettingsException("could not find default slack account [" + defaultAccountName + "]"); - } else { - this.defaultAccountName = defaultAccountName; - } - } - - /** - * Returns the account associated with the given name. If there is not such account, {@code null} is returned. - * If the given name is {@code null}, the default account will be returned. - * - * @param name The name of the requested account - * @return The account associated with the given name, or {@code null} when requested an unknown account. - * @throws IllegalStateException if the name is null and the default account is null. - */ - public SlackAccount account(String name) throws IllegalStateException { - if (name == null) { - if (defaultAccountName == null) { - throw new IllegalStateException("cannot find default slack account as no accounts have been configured"); - } - name = defaultAccountName; - } - return accounts.get(name); - } -} diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackService.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackService.java index 64554000fbb..6d5e33e6e35 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackService.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/notification/slack/SlackService.java @@ -5,46 +5,31 @@ */ package org.elasticsearch.xpack.notification.slack; -import org.elasticsearch.common.component.AbstractComponent; -import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.xpack.common.http.HttpClient; +import org.elasticsearch.xpack.notification.NotificationService; /** * A component to store slack credentials. */ -public class SlackService extends AbstractComponent { +public class SlackService extends NotificationService { private final HttpClient httpClient; public static final Setting SLACK_ACCOUNT_SETTING = Setting.groupSetting("xpack.notification.slack.", Setting.Property.Dynamic, Setting.Property.NodeScope); - private volatile SlackAccounts accounts; public SlackService(Settings settings, HttpClient httpClient, ClusterSettings clusterSettings) { super(settings); this.httpClient = httpClient; - clusterSettings.addSettingsUpdateConsumer(SLACK_ACCOUNT_SETTING, this::setSlackAccountSetting); - setSlackAccountSetting(SLACK_ACCOUNT_SETTING.get(settings)); + clusterSettings.addSettingsUpdateConsumer(SLACK_ACCOUNT_SETTING, this::setAccountSetting); + setAccountSetting(SLACK_ACCOUNT_SETTING.get(settings)); } - /** - * @return The default slack account. - */ - public SlackAccount getDefaultAccount() { - return accounts.account(null); + @Override + protected SlackAccount createAccount(String name, Settings accountSettings) { + return new SlackAccount(name, accountSettings, accountSettings, httpClient, logger); } - private void setSlackAccountSetting(Settings setting) { - accounts = new SlackAccounts(setting, httpClient, logger); - } - - /** - * @return The account identified by the given name. If the given name is {@code null} the default - * account will be returned. - */ - public SlackAccount getAccount(String name) { - return accounts.account(name); - } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/hipchat/ExecutableHipChatAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/hipchat/ExecutableHipChatAction.java index d108fbff027..ec5d892362b 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/hipchat/ExecutableHipChatAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/hipchat/ExecutableHipChatAction.java @@ -34,10 +34,7 @@ public class ExecutableHipChatAction extends ExecutableAction { @Override public Action.Result execute(final String actionId, WatchExecutionContext ctx, Payload payload) throws Exception { - HipChatAccount account = action.account != null ? - hipchatService.getAccount(action.account) : - hipchatService.getDefaultAccount(); - + HipChatAccount account = hipchatService.getAccount(action.account); // lets validate the message again, in case the hipchat service were updated since the // watch/action were created. account.validateParsedTemplate(ctx.id().watchId(), actionId, action.message); diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/pagerduty/ExecutablePagerDutyAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/pagerduty/ExecutablePagerDutyAction.java index 7971ff31bda..549a2270003 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/pagerduty/ExecutablePagerDutyAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/pagerduty/ExecutablePagerDutyAction.java @@ -34,10 +34,7 @@ public class ExecutablePagerDutyAction extends ExecutableAction @Override public Action.Result execute(final String actionId, WatchExecutionContext ctx, Payload payload) throws Exception { - PagerDutyAccount account = action.event.account != null ? - pagerDutyService.getAccount(action.event.account) : - pagerDutyService.getDefaultAccount(); - + PagerDutyAccount account = pagerDutyService.getAccount(action.event.account); if (account == null) { // the account associated with this action was deleted throw new IllegalStateException("account [" + action.event.account + "] was not found. perhaps it was deleted"); diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/slack/ExecutableSlackAction.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/slack/ExecutableSlackAction.java index 007875c1a3b..978a7a26c34 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/slack/ExecutableSlackAction.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/actions/slack/ExecutableSlackAction.java @@ -33,9 +33,7 @@ public class ExecutableSlackAction extends ExecutableAction { @Override public Action.Result execute(final String actionId, WatchExecutionContext ctx, Payload payload) throws Exception { - SlackAccount account = action.account != null ? - slackService.getAccount(action.account) : - slackService.getDefaultAccount(); + SlackAccount account = slackService.getAccount(action.account); if (account == null) { // the account associated with this action was deleted diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/Condition.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/Condition.java index f3552ba204b..edf9f9cb993 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/Condition.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/Condition.java @@ -39,7 +39,7 @@ public abstract class Condition implements ToXContent { return builder.startObject().endObject(); } - public static class Result implements ToXContent { + public static class Result implements ToXContent { // don't make this final - we can't mock final classes :( public Map getResolvedValues() { return resolveValues; diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/AccountsTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/AccountsTests.java index 22c3df0c1a4..785a603b861 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/AccountsTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/AccountsTests.java @@ -5,9 +5,13 @@ */ package org.elasticsearch.xpack.notification.email; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.notification.NotificationService; + +import java.util.Collections; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -19,12 +23,12 @@ public class AccountsTests extends ESTestCase { Settings.Builder builder = Settings.builder() .put("default_account", "account1"); addAccountSettings("account1", builder); - - Accounts accounts = new Accounts(builder.build(), null, logger); - Account account = accounts.account("account1"); + EmailService service = new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); + Account account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); } @@ -32,30 +36,31 @@ public class AccountsTests extends ESTestCase { public void testSingleAccountNoExplicitDefault() throws Exception { Settings.Builder builder = Settings.builder(); addAccountSettings("account1", builder); - - Accounts accounts = new Accounts(builder.build(), null, logger); - Account account = accounts.account("account1"); + EmailService service = new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); + Account account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); } public void testMultipleAccounts() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.email.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - Accounts accounts = new Accounts(builder.build(), null, logger); - Account account = accounts.account("account1"); + EmailService service = new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); + Account account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account2")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); } @@ -66,54 +71,53 @@ public class AccountsTests extends ESTestCase { addAccountSettings("account1", builder); addAccountSettings("account2", builder); - Accounts accounts = new Accounts(builder.build(), null, logger); - Account account = accounts.account("account1"); + EmailService service = new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); + Account account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name(), equalTo("account2")); - account = accounts.account(null); + account = service.getAccount(null); assertThat(account, notNullValue()); assertThat(account.name(), isOneOf("account1", "account2")); } public void testMultipleAccountsUnknownDefault() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.email.default_account", "unknown"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); try { - new Accounts(builder.build(), null, logger); + new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); fail("Expected SettingsException"); } catch (SettingsException e) { - assertThat(e.getMessage(), is("could not find default email account [unknown]")); + assertThat(e.getMessage(), is("could not find default account [unknown]")); } } public void testNoAccount() throws Exception { Settings.Builder builder = Settings.builder(); - Accounts accounts = new Accounts(builder.build(), null, logger); - try { - accounts.account(null); - fail("no accounts are configured so trying to get the default account should throw an IllegalStateException"); - } catch (IllegalStateException e) { - assertThat(e.getMessage(), is("cannot find default email account as no accounts have been configured")); - } + EmailService service = new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); + expectThrows(IllegalArgumentException.class, () -> service.getAccount(null)); } public void testNoAccountWithDefaultAccount() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.email.default_account", "unknown"); try { - new Accounts(builder.build(), null, logger); + new EmailService(builder.build(), null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); fail("Expected SettingsException"); } catch (SettingsException e) { - assertThat(e.getMessage(), is("could not find default email account [unknown]")); + assertThat(e.getMessage(), is("could not find default account [unknown]")); } } private void addAccountSettings(String name, Settings.Builder builder) { - builder.put("account." + name + ".smtp.host", "_host"); + builder.put("xpack.notification.email.account." + name + ".smtp.host", "_host"); } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/EmailServiceTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/EmailServiceTests.java index a6106c861e8..be622c6aba9 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/EmailServiceTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/EmailServiceTests.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.notification.email; -import org.apache.logging.log4j.Logger; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; @@ -23,26 +22,23 @@ import static org.mockito.Mockito.when; public class EmailServiceTests extends ESTestCase { private EmailService service; - private Accounts accounts; + private Account account; @Before public void init() throws Exception { - accounts = mock(Accounts.class); - service = new EmailService(Settings.EMPTY, null, + account = mock(Account.class); + service = new EmailService(Settings.builder().put("xpack.notification.email.account.account1.foo", "bar").build(), null, new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))) { @Override - protected Accounts createAccounts(Settings settings, Logger logger) { - return accounts; + protected Account createAccount(String name, Settings accountSettings) { + return account; } }; } public void testSend() throws Exception { - Account account = mock(Account.class); when(account.name()).thenReturn("account1"); - when(accounts.account("account1")).thenReturn(account); Email email = mock(Email.class); - Authentication auth = new Authentication("user", new Secret("passwd".toCharArray())); Profile profile = randomFrom(Profile.values()); when(account.send(email, auth, profile)).thenReturn(email); @@ -52,5 +48,4 @@ public class EmailServiceTests extends ESTestCase { assertThat(sent.email(), sameInstance(email)); assertThat(sent.account(), is("account1")); } - } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/ProfileTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/ProfileTests.java index c2b0897d854..1ef365eb736 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/ProfileTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/email/ProfileTests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.notification.email; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; @@ -14,6 +15,8 @@ import javax.mail.Session; import javax.mail.internet.MimeMessage; import javax.mail.internet.MimeMultipart; +import java.util.Collections; + import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -33,12 +36,13 @@ public class ProfileTests extends ESTestCase { .build(); Settings settings = Settings.builder() - .put("default_account", "foo") - .put("account.foo.smtp.host", "_host") + .put("xpack.notification.email.default_account", "foo") + .put("xpack.notification.email.account.foo.smtp.host", "_host") .build(); - Accounts accounts = new Accounts(settings, null, logger); - Session session = accounts.account("foo").getConfig().createSession(); + EmailService service = new EmailService(settings, null, + new ClusterSettings(Settings.EMPTY, Collections.singleton(EmailService.EMAIL_ACCOUNT_SETTING))); + Session session = service.getAccount("foo").getConfig().createSession(); MimeMessage mimeMessage = Profile.STANDARD.toMimeMessage(email, session); Object content = ((MimeMultipart) mimeMessage.getContent()).getBodyPart(0).getContent(); diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccountsTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccountsTests.java index 6d8e26e4c5f..ad53b19be97 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccountsTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatAccountsTests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.notification.hipchat; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.test.ESTestCase; @@ -17,6 +18,7 @@ import org.elasticsearch.xpack.watcher.test.MockTextTemplateEngine; import org.junit.Before; import org.mockito.ArgumentCaptor; +import java.util.Collections; import java.util.HashMap; import static org.hamcrest.Matchers.equalTo; @@ -24,7 +26,6 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.isOneOf; import static org.hamcrest.Matchers.notNullValue; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class HipChatAccountsTests extends ESTestCase { @@ -37,14 +38,14 @@ public class HipChatAccountsTests extends ESTestCase { public void testSingleAccount() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.hipchat.default_account", "account1"); addAccountSettings("account1", builder); - - HipChatAccounts accounts = new HipChatAccounts(builder.build(), httpClient, logger); - HipChatAccount account = accounts.account("account1"); + HipChatService service = new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); + HipChatAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } @@ -53,92 +54,99 @@ public class HipChatAccountsTests extends ESTestCase { Settings.Builder builder = Settings.builder(); addAccountSettings("account1", builder); - HipChatAccounts accounts = new HipChatAccounts(builder.build(), httpClient, logger); - HipChatAccount account = accounts.account("account1"); + HipChatService service = new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); + HipChatAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } public void testMultipleAccounts() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.hipchat.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - HipChatAccounts accounts = new HipChatAccounts(builder.build(), httpClient, logger); - HipChatAccount account = accounts.account("account1"); + HipChatService service = new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); + HipChatAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account2")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } public void testMultipleAccountsNoExplicitDefault() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.hipchat.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - HipChatAccounts accounts = new HipChatAccounts(builder.build(), httpClient, logger); - HipChatAccount account = accounts.account("account1"); + HipChatService service = new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); + HipChatAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account2")); - account = accounts.account(null); + account = service.getAccount(null); assertThat(account, notNullValue()); assertThat(account.name, isOneOf("account1", "account2")); } public void testMultipleAccountsUnknownDefault() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.hipchat.default_account", "unknown"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); try { - new HipChatAccounts(builder.build(), httpClient, logger); + new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); fail("Expected SettingsException"); } catch (SettingsException e) { - assertThat(e.getMessage(), is("could not find default hipchat account [unknown]")); + assertThat(e.getMessage(), is("could not find default account [unknown]")); } } public void testNoAccount() throws Exception { Settings.Builder builder = Settings.builder(); - HipChatAccounts accounts = new HipChatAccounts(builder.build(), httpClient, logger); + HipChatService service = new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); try { - accounts.account(null); + service.getAccount(null); fail("no accounts are configured so trying to get the default account should throw an IllegalStateException"); - } catch (IllegalStateException e) { - assertThat(e.getMessage(), is("cannot find default hipchat account as no accounts have been configured")); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), is("no account found for name: [null]")); } } public void testNoAccountWithDefaultAccount() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.hipchat.default_account", "unknown"); try { - new HipChatAccounts(builder.build(), httpClient, logger); + new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); fail("Expected SettingsException"); } catch (SettingsException e) { - assertThat(e.getMessage(), is("could not find default hipchat account [unknown]")); + assertThat(e.getMessage(), is("could not find default account [unknown]")); } } public void testProxy() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.hipchat.default_account", "account1"); addAccountSettings("account1", builder); - HipChatAccounts accounts = new HipChatAccounts(builder.build(), httpClient, logger); - HipChatAccount account = accounts.account("account1"); + HipChatService service = new HipChatService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(HipChatService.HIPCHAT_ACCOUNT_SETTING))); + HipChatAccount account = service.getAccount("account1"); HipChatMessage.Template template = new HipChatMessage.Template.Builder(new TextTemplate("foo")) .addRooms(new TextTemplate("room")) @@ -158,10 +166,10 @@ public class HipChatAccountsTests extends ESTestCase { private void addAccountSettings(String name, Settings.Builder builder) { HipChatAccount.Profile profile = randomFrom(HipChatAccount.Profile.values()); - builder.put("account." + name + ".profile", profile.value()); - builder.put("account." + name + ".auth_token", randomAsciiOfLength(50)); + builder.put("xpack.notification.hipchat.account." + name + ".profile", profile.value()); + builder.put("xpack.notification.hipchat.account." + name + ".auth_token", randomAsciiOfLength(50)); if (profile == HipChatAccount.Profile.INTEGRATION) { - builder.put("account." + name + ".room", randomAsciiOfLength(10)); + builder.put("xpack.notification.hipchat.account." + name + ".room", randomAsciiOfLength(10)); } } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatServiceTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatServiceTests.java index c0683088288..a383ff508ef 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatServiceTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/hipchat/HipChatServiceTests.java @@ -74,7 +74,7 @@ public class HipChatServiceTests extends ESTestCase { assertThat(((V1Account) account).defaults.notify, is(defaultNotify)); // with a single account defined, making sure that that account is set to the default one. - assertThat(service.getDefaultAccount(), sameInstance(account)); + assertThat(service.getAccount(null), sameInstance(account)); } public void testSingleAccountIntegration() throws Exception { @@ -117,7 +117,7 @@ public class HipChatServiceTests extends ESTestCase { assertThat(((IntegrationAccount) account).defaults.notify, is(defaultNotify)); // with a single account defined, making sure that that account is set to the default one. - assertThat(service.getDefaultAccount(), sameInstance(account)); + assertThat(service.getAccount(null), sameInstance(account)); } public void testSingleAccountIntegrationNoRoomSetting() throws Exception { @@ -179,7 +179,7 @@ public class HipChatServiceTests extends ESTestCase { assertThat(((UserAccount) account).defaults.notify, is(defaultNotify)); // with a single account defined, making sure that that account is set to the default one. - assertThat(service.getDefaultAccount(), sameInstance(account)); + assertThat(service.getAccount(null), sameInstance(account)); } public void testMultipleAccounts() throws Exception { @@ -190,7 +190,7 @@ public class HipChatServiceTests extends ESTestCase { String defaultAccount = "_a" + randomIntBetween(0, 4); settingsBuilder.put("xpack.notification.hipchat.default_account", defaultAccount); - boolean customGlobalServer = randomBoolean(); + final boolean customGlobalServer = randomBoolean(); if (customGlobalServer) { settingsBuilder.put("xpack.notification.hipchat.host", "_host_global"); settingsBuilder.put("xpack.notification.hipchat.port", 299); @@ -240,7 +240,7 @@ public class HipChatServiceTests extends ESTestCase { } } - assertThat(service.getDefaultAccount(), sameInstance(service.getAccount(defaultAccount))); + assertThat(service.getAccount(null), sameInstance(service.getAccount(defaultAccount))); } private void buildMessageDefaults(String account, Settings.Builder settingsBuilder, String room, String user, String from, diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccountsTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccountsTests.java index 704fe2c9a72..6ead551f983 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccountsTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/pagerduty/PagerDutyAccountsTests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.notification.pagerduty; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.test.ESTestCase; @@ -17,6 +18,7 @@ import org.elasticsearch.xpack.watcher.watch.Payload; import org.junit.Before; import org.mockito.ArgumentCaptor; +import java.util.Collections; import java.util.Map; import static org.hamcrest.Matchers.equalTo; @@ -37,14 +39,14 @@ public class PagerDutyAccountsTests extends ESTestCase { public void testSingleAccount() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.pagerduty.default_account", "account1"); addAccountSettings("account1", builder); - - PagerDutyAccounts accounts = new PagerDutyAccounts(builder.build(), httpClient, logger); - PagerDutyAccount account = accounts.account("account1"); + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + PagerDutyAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } @@ -53,85 +55,90 @@ public class PagerDutyAccountsTests extends ESTestCase { Settings.Builder builder = Settings.builder(); addAccountSettings("account1", builder); - PagerDutyAccounts accounts = new PagerDutyAccounts(builder.build(), httpClient, logger); - PagerDutyAccount account = accounts.account("account1"); + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + PagerDutyAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } public void testMultipleAccounts() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.pagerduty.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - PagerDutyAccounts accounts = new PagerDutyAccounts(builder.build(), httpClient, logger); - PagerDutyAccount account = accounts.account("account1"); + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + PagerDutyAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account2")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } public void testMultipleAccounts_NoExplicitDefault() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.pagerduty.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - PagerDutyAccounts accounts = new PagerDutyAccounts(builder.build(), httpClient, logger); - PagerDutyAccount account = accounts.account("account1"); + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + PagerDutyAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account2")); - account = accounts.account(null); + account = service.getAccount(null); assertThat(account, notNullValue()); assertThat(account.name, isOneOf("account1", "account2")); } public void testMultipleAccounts_UnknownDefault() throws Exception { - try { + expectThrows(SettingsException.class, () -> { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.pagerduty.default_account", "unknown"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - new PagerDutyAccounts(builder.build(), httpClient, logger); - fail("Expected a SettingsException to happen"); - } catch (SettingsException e) {} + new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + }); } public void testNoAccount() throws Exception { - try { + expectThrows(IllegalArgumentException.class, () -> { Settings.Builder builder = Settings.builder(); - PagerDutyAccounts accounts = new PagerDutyAccounts(builder.build(), httpClient, logger); - accounts.account(null); - fail("no accounts are configured so trying to get the default account should throw an IllegalStateException"); - } catch (IllegalStateException e) {} + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + service.getAccount(null); + }); } public void testNoAccount_WithDefaultAccount() throws Exception { try { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); - new PagerDutyAccounts(builder.build(), httpClient, logger); + .put("xpack.notification.pagerduty.default_account", "unknown"); + new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); fail("Expected a SettingsException to happen"); } catch (SettingsException e) {} } public void testProxy() throws Exception { - Settings.Builder builder = Settings.builder().put("default_account", "account1"); + Settings.Builder builder = Settings.builder().put("xpack.notification.pagerduty.default_account", "account1"); addAccountSettings("account1", builder); - PagerDutyAccounts accounts = new PagerDutyAccounts(builder.build(), httpClient, logger); - PagerDutyAccount account = accounts.account("account1"); + PagerDutyService service = new PagerDutyService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(PagerDutyService.PAGERDUTY_ACCOUNT_SETTING))); + PagerDutyAccount account = service.getAccount("account1"); ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(HttpRequest.class); when(httpClient.execute(argumentCaptor.capture())).thenReturn(new HttpResponse(200)); @@ -145,10 +152,10 @@ public class PagerDutyAccountsTests extends ESTestCase { } private void addAccountSettings(String name, Settings.Builder builder) { - builder.put("account." + name + ".service_api_key", randomAsciiOfLength(50)); + builder.put("xpack.notification.pagerduty.account." + name + ".service_api_key", randomAsciiOfLength(50)); Settings defaults = SlackMessageDefaultsTests.randomSettings(); for (Map.Entry setting : defaults.getAsMap().entrySet()) { - builder.put("message_defaults." + setting.getKey(), setting.getValue()); + builder.put("xpack.notification.pagerduty.message_defaults." + setting.getKey(), setting.getValue()); } } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/slack/SlackAccountsTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/slack/SlackAccountsTests.java index 2c8589a457e..47c278a17cb 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/slack/SlackAccountsTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/notification/slack/SlackAccountsTests.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.notification.slack; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.test.ESTestCase; @@ -12,6 +13,7 @@ import org.elasticsearch.xpack.common.http.HttpClient; import org.elasticsearch.xpack.notification.slack.message.SlackMessageDefaultsTests; import org.junit.Before; +import java.util.Collections; import java.util.Map; import static org.hamcrest.Matchers.equalTo; @@ -30,14 +32,15 @@ public class SlackAccountsTests extends ESTestCase { public void testSingleAccount() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.slack.default_account", "account1"); addAccountSettings("account1", builder); - SlackAccounts accounts = new SlackAccounts(builder.build(), httpClient, logger); - SlackAccount account = accounts.account("account1"); + SlackService service = new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); + SlackAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } @@ -46,91 +49,97 @@ public class SlackAccountsTests extends ESTestCase { Settings.Builder builder = Settings.builder(); addAccountSettings("account1", builder); - SlackAccounts accounts = new SlackAccounts(builder.build(), httpClient, logger); - SlackAccount account = accounts.account("account1"); + SlackService service = new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); + SlackAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } public void testMultipleAccounts() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.slack.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - SlackAccounts accounts = new SlackAccounts(builder.build(), httpClient, logger); - SlackAccount account = accounts.account("account1"); + SlackService service = new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); + SlackAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account2")); - account = accounts.account(null); // falling back on the default + account = service.getAccount(null); // falling back on the default assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); } public void testMultipleAccountsNoExplicitDefault() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "account1"); + .put("xpack.notification.slack.default_account", "account1"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); - SlackAccounts accounts = new SlackAccounts(builder.build(), httpClient, logger); - SlackAccount account = accounts.account("account1"); + SlackService service = new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); + SlackAccount account = service.getAccount("account1"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account1")); - account = accounts.account("account2"); + account = service.getAccount("account2"); assertThat(account, notNullValue()); assertThat(account.name, equalTo("account2")); - account = accounts.account(null); + account = service.getAccount(null); assertThat(account, notNullValue()); assertThat(account.name, isOneOf("account1", "account2")); } public void testMultipleAccountsUnknownDefault() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.slack.default_account", "unknown"); addAccountSettings("account1", builder); addAccountSettings("account2", builder); try { - new SlackAccounts(builder.build(), httpClient, logger); + new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); fail("Expected SettingsException"); } catch (SettingsException e) { - assertThat(e.getMessage(), is("could not find default slack account [unknown]")); + assertThat(e.getMessage(), is("could not find default account [unknown]")); } } public void testNoAccount() throws Exception { Settings.Builder builder = Settings.builder(); - SlackAccounts accounts = new SlackAccounts(builder.build(), httpClient, logger); + SlackService service = new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); try { - accounts.account(null); + service.getAccount(null); fail("no accounts are configured so trying to get the default account should throw an IllegalStateException"); - } catch (IllegalStateException e) { - assertThat(e.getMessage(), is("cannot find default slack account as no accounts have been configured")); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), is("no account found for name: [null]")); } } public void testNoAccountWithDefaultAccount() throws Exception { Settings.Builder builder = Settings.builder() - .put("default_account", "unknown"); + .put("xpack.notification.slack.default_account", "unknown"); try { - new SlackAccounts(builder.build(), httpClient, logger); + new SlackService(builder.build(), httpClient, new ClusterSettings(Settings.EMPTY, + Collections.singleton(SlackService.SLACK_ACCOUNT_SETTING))); fail("Expected SettingsException"); } catch (SettingsException e) { - assertThat(e.getMessage(), is("could not find default slack account [unknown]")); + assertThat(e.getMessage(), is("could not find default account [unknown]")); } } private void addAccountSettings(String name, Settings.Builder builder) { - builder.put("account." + name + ".url", "https://hooks.slack.com/services/" + randomAsciiOfLength(50)); + builder.put("xpack.notification.slack.account." + name + ".url", "https://hooks.slack.com/services/" + randomAsciiOfLength(50)); Settings defaults = SlackMessageDefaultsTests.randomSettings(); for (Map.Entry setting : defaults.getAsMap().entrySet()) { - builder.put("message_defaults." + setting.getKey(), setting.getValue()); + builder.put("xpack.notification.slack.message_defaults." + setting.getKey(), setting.getValue()); } } }