Skip to content

Commit

Permalink
feat: settings closed set with validation [DHIS2-18217] (#18817)
Browse files Browse the repository at this point in the history
* feat: settings closed set with validation [DHIS2-18217]

* fix: set timezone for date string format

* fix: tests

* fix: e2e tests

* feat: user settings validation and co

* chore: code format

* fix: always route to translation endpoint for all accept headers
  • Loading branch information
jbee authored Oct 17, 2024
1 parent ad04ebf commit ad49941
Show file tree
Hide file tree
Showing 28 changed files with 434 additions and 196 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.hisp.dhis.jsontree.JsonMap;
import org.hisp.dhis.jsontree.JsonMixed;
import org.hisp.dhis.jsontree.JsonValue;
import org.hisp.dhis.translation.Translatable;

/**
* {@link SystemSettings} or {@link UserSettings} represented by a set of keys and their values.
Expand All @@ -86,6 +87,8 @@
final class LazySettings implements SystemSettings, UserSettings {

private static final Set<String> CONFIDENTIAL_KEYS = new HashSet<>();
private static final Set<String> TRANSLATABLE_KEYS = new HashSet<>();

private static final Map<String, Serializable> DEFAULTS_SYSTEM_SETTINGS =
extractDefaults(SystemSettings.class);
private static final Map<String, Serializable> DEFAULTS_USER_SETTINGS =
Expand All @@ -106,6 +109,10 @@ static boolean isConfidential(@Nonnull String key) {
return CONFIDENTIAL_KEYS.contains(key);
}

static boolean isTranslatable(@Nonnull String key) {
return TRANSLATABLE_KEYS.contains(key);
}

@Nonnull
static LazySettings of(Class<? extends Settings> type, @Nonnull Map<String, String> settings) {
return of(type, settings, (k, v) -> v);
Expand Down Expand Up @@ -243,12 +250,15 @@ public JsonMap<JsonMixed> toJson(boolean includeConfidential, @Nonnull Set<Strin
.asMap(JsonMixed.class);
}

private static final DateTimeFormatter ISO_DATE_TIME =
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS").withZone(ZoneId.systemDefault());

@Nonnull
private JsonValue asJson(String key) {
Serializable defaultValue = getDefault(key);
if (defaultValue instanceof String s) return Json.of(asString(key, s));
if (defaultValue instanceof Date d)
return Json.of(DateTimeFormatter.ISO_INSTANT.format(asDate(key, d).toInstant()));
return Json.of(ISO_DATE_TIME.format(asDate(key, d).toInstant()));
if (defaultValue instanceof Double d) return Json.of(asDouble(key, d));
if (defaultValue instanceof Number n) return Json.of(asInt(key, n.intValue()));
if (defaultValue instanceof Boolean b) return Json.of(asBoolean(key, b));
Expand All @@ -262,6 +272,25 @@ private JsonValue asJson(String key) {
return Json.of(value);
}

@Override
public boolean isValid(String key, String value) {
Serializable defaultValue = getDefault(key);
if (defaultValue == null || defaultValue instanceof String) return true;
if (defaultValue instanceof Boolean) return "true".equals(value) || "false".equals(value);
try {
// Note: The != null is just a dummy test the parse yielded anything
if (defaultValue instanceof Double) return Double.valueOf(value) != null;
if (defaultValue instanceof Number) return Integer.valueOf(value) != null;
if (defaultValue instanceof Date) return parseDate(value) != null;
if (defaultValue instanceof Locale) return LocaleUtils.toLocale(value) != null;
if (defaultValue instanceof Enum<?>)
return Enum.valueOf(((Enum<?>) defaultValue).getDeclaringClass(), value) != null;
return true;
} catch (Exception ex) {
return false;
}
}

private int indexOf(String key) {
if (keys.length == 0) return -1;
int i = Arrays.binarySearch(keys, key);
Expand Down Expand Up @@ -334,6 +363,8 @@ private static Map<String, Serializable> extractDefaults(Class<? extends Setting
defaults.put(key, defaultValue);
if (lastDefault[0].isAnnotationPresent(Confidential.class))
CONFIDENTIAL_KEYS.add(key);
if (lastDefault[0].isAnnotationPresent(Translatable.class))
TRANSLATABLE_KEYS.add(key);
}
return args[1];
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,6 @@ static String valueOf(@CheckForNull Serializable value) {
double asDouble(@Nonnull String key, double defaultValue);

boolean asBoolean(@Nonnull String key, boolean defaultValue);

boolean isValid(String key, String value);
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import org.hisp.dhis.period.RelativePeriodEnum;
import org.hisp.dhis.scheduling.JobConfiguration;
import org.hisp.dhis.security.LoginPageLayout;
import org.hisp.dhis.translation.Translatable;

/**
* A complete set of system settings.
Expand Down Expand Up @@ -106,6 +107,14 @@ static boolean isConfidential(@Nonnull String key) {
return LazySettings.isConfidential(key);
}

/**
* @param key a setting name
* @return true, if it can have a translation, false otherwise
*/
static boolean isTranslatable(@Nonnull String key) {
return LazySettings.isTranslatable(key);
}

/*
settings used in core
*/
Expand Down Expand Up @@ -134,22 +143,27 @@ default String getTrackerDashboardLayout() {
return asString("keyTrackerDashboardLayout", "");
}

@Translatable
default String getApplicationTitle() {
return asString("applicationTitle", "DHIS 2");
}

@Translatable
default String getApplicationIntro() {
return asString("keyApplicationIntro", "");
}

@Translatable
default String getApplicationNotification() {
return asString("keyApplicationNotification", "");
}

@Translatable
default String getApplicationFooter() {
return asString("keyApplicationFooter", "");
}

@Translatable
default String getApplicationRightFooter() {
return asString("keyApplicationRightFooter", "");
}
Expand Down Expand Up @@ -674,6 +688,7 @@ default int getTrackedEntityMaxLimit() {
return asInt("KeyTrackedEntityMaxLimit", 50000);
}

@Translatable
default String getLoginPopup() {
return asString("loginPopup", "");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import java.util.Set;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import org.hisp.dhis.feedback.BadRequestException;
import org.hisp.dhis.feedback.NotFoundException;

/**
* @author Jan Bernitt (refactored version)
Expand All @@ -48,6 +50,9 @@ public interface SystemSettingsService extends SystemSettingsProvider {
* Saves a single setting with {@link Settings#valueOf(Serializable)} applied to the provided
* value.
*
* <p>This method is for internal use only. Therefore, if the key does not exist no exception is
* thrown but an error is logged.
*
* @param key of the setting to insert or update
* @param value of the setting, null or empty to delete
*/
Expand All @@ -57,8 +62,10 @@ public interface SystemSettingsService extends SystemSettingsProvider {
* Saves the given system setting key and value.
*
* @param settings the new values, null or empty values delete the setting
* @throws NotFoundException if any of the provided keys is not a key contained in {@link
* SystemSettings#keysWithDefaults()}
*/
void putAll(@Nonnull Map<String, String> settings);
void putAll(@Nonnull Map<String, String> settings) throws NotFoundException, BadRequestException;

/**
* Deletes the system setting with the given name.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public interface SystemSettingsTranslationService {
* @param locale locale of the translation (should be a language tag)
* @param translation translation text, null or empty to delete
*/
void saveSystemSettingTranslation(
void putSystemSettingTranslation(
@Nonnull String key, @Nonnull String locale, @CheckForNull String translation)
throws ForbiddenException, BadRequestException;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
public @interface Translatable {
/** Property name for enabling translation */
@Nonnull
String propertyName();
String propertyName() default "";

/**
* Translation key for storing translation in json format. If not defined then property name is
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import java.util.Map;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import org.hisp.dhis.feedback.BadRequestException;
import org.hisp.dhis.feedback.ConflictException;
import org.hisp.dhis.feedback.NotFoundException;
import org.hisp.dhis.setting.UserSettings;

Expand Down Expand Up @@ -62,27 +64,36 @@ public interface UserSettingsService {
*
* @param key the user setting key.
* @param value the setting value, null or empty to delete
* @throws NotFoundException when a setting with the key does not exist for users
* @throws BadRequestException when the value isn't valid for the key
*/
void put(@Nonnull String key, @CheckForNull Serializable value);
void put(@Nonnull String key, @CheckForNull Serializable value)
throws NotFoundException, BadRequestException;

/**
* Saves the name/value pair as a user setting connected to user.
*
* @param key the user setting key.
* @param value the setting value, null or empty to delete
* @param username owner/target of the settings update
* @throws NotFoundException when a setting with the key does not exist for users
* @throws BadRequestException when the value isn't valid for the key
* @throws ConflictException when no user with the given username exists
*/
void put(@Nonnull String key, @CheckForNull Serializable value, @Nonnull String username)
throws NotFoundException;
throws NotFoundException, BadRequestException, ConflictException;

/**
* Updates all settings in the provided collection.
*
* @param settings settings to store, null or empty values are deleted
* @param username owner/target of the settings update
* @throws NotFoundException when a setting with the key does not exist for users
* @throws BadRequestException when the value isn't valid for the key
* @throws ConflictException when no user with the given username exists
*/
void putAll(@Nonnull Map<String, String> settings, @Nonnull String username)
throws NotFoundException;
throws NotFoundException, BadRequestException, ConflictException;

/**
* Deletes all settings of a user. If no such user exists this has no effect.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
*/
package org.hisp.dhis.analytics.table;

import static java.util.Map.entry;
import static org.hisp.dhis.common.collection.CollectionUtils.emptyIfNull;
import static org.hisp.dhis.scheduling.JobProgress.FailurePolicy.SKIP_STAGE;
import static org.hisp.dhis.util.DateUtils.toLongDate;
Expand All @@ -48,7 +47,6 @@
import org.hisp.dhis.analytics.cache.OutliersCache;
import org.hisp.dhis.resourcetable.ResourceTableService;
import org.hisp.dhis.scheduling.JobProgress;
import org.hisp.dhis.setting.Settings;
import org.hisp.dhis.setting.SystemSettingsService;
import org.hisp.dhis.system.util.Clock;
import org.springframework.stereotype.Service;
Expand Down Expand Up @@ -118,19 +116,11 @@ public void generateAnalyticsTables(AnalyticsTableUpdateParams params0, JobProgr

private void updateLastSuccessfulSystemSettings(AnalyticsTableUpdateParams params, Clock clock) {
if (params.isLatestUpdate()) {
settingsService.putAll(
Map.ofEntries(
entry(
"keyLastSuccessfulLatestAnalyticsPartitionUpdate",
Settings.valueOf(params.getStartTime())),
entry("keyLastSuccessfulLatestAnalyticsPartitionRuntime", clock.time())));
settingsService.put("keyLastSuccessfulLatestAnalyticsPartitionUpdate", params.getStartTime());
settingsService.put("keyLastSuccessfulLatestAnalyticsPartitionRuntime", clock.time());
} else {
settingsService.putAll(
Map.ofEntries(
entry(
"keyLastSuccessfulAnalyticsTablesUpdate",
Settings.valueOf(params.getStartTime())),
entry("keyLastSuccessfulAnalyticsTablesRuntime", clock.time())));
settingsService.put("keyLastSuccessfulAnalyticsTablesUpdate", params.getStartTime());
settingsService.put("keyLastSuccessfulAnalyticsTablesRuntime", clock.time());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@
*/
package org.hisp.dhis.i18n.ui.locale;

import static com.google.common.base.Preconditions.checkNotNull;

import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import javax.annotation.CheckForNull;
import lombok.RequiredArgsConstructor;
import org.hisp.dhis.feedback.BadRequestException;
import org.hisp.dhis.feedback.NotFoundException;
import org.hisp.dhis.i18n.locale.LocaleManager;
import org.hisp.dhis.i18n.ui.resourcebundle.ResourceBundleManager;
import org.hisp.dhis.i18n.ui.resourcebundle.ResourceBundleManagerException;
Expand All @@ -45,28 +46,12 @@
* @author Torgeir Lorange Ostby
*/
@Component("org.hisp.dhis.i18n.locale.LocaleManager")
@RequiredArgsConstructor
public class UserSettingLocaleManager implements LocaleManager {
// -------------------------------------------------------------------------
// Dependencies
// -------------------------------------------------------------------------

private final UserSettingsService userSettingsService;

private final ResourceBundleManager resourceBundleManager;

public UserSettingLocaleManager(
UserSettingsService userSettingsService, ResourceBundleManager resourceBundleManager) {
checkNotNull(userSettingsService);
checkNotNull(resourceBundleManager);

this.userSettingsService = userSettingsService;
this.resourceBundleManager = resourceBundleManager;
}

// -------------------------------------------------------------------------
// LocaleManager implementation
// -------------------------------------------------------------------------

@Override
public Locale getCurrentLocale() {

Expand All @@ -84,7 +69,12 @@ public Locale getCurrentLocale() {

@Override
public void setCurrentLocale(Locale locale) {
userSettingsService.put("keyUiLocale", locale);
try {
userSettingsService.put("keyUiLocale", locale);
} catch (NotFoundException | BadRequestException ex) {
// this should never happen as this key-value combination is valid
throw new IllegalArgumentException(ex);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import java.util.SortedMap;
import java.util.TreeMap;
import lombok.RequiredArgsConstructor;
import org.hisp.dhis.feedback.BadRequestException;
import org.hisp.dhis.feedback.NotFoundException;
import org.hisp.dhis.i18n.I18n;
import org.hisp.dhis.i18n.I18nManager;
import org.hisp.dhis.user.CurrentUserUtil;
Expand Down Expand Up @@ -77,7 +79,12 @@ public void setSystemStyle(String style) {

@Override
public void setUserStyle(String style) {
userSettingsService.put("keyStyle", style);
try {
userSettingsService.put("keyStyle", style);
} catch (NotFoundException | BadRequestException ex) {
// this should never happen as this key-value combination is valid
throw new IllegalArgumentException(ex);
}
}

@Override
Expand Down
Loading

0 comments on commit ad49941

Please sign in to comment.