refactor: system and user settings as value objects [DHIS2-18061]

[jbee]

Summary

Implements SystemSettings and UserSettings as immutable value objects.

Qualities of settings...

• are key-value pairs with simple values like booleans, numbers and string based types
• are converted from String to typed values on access
• are never undefined or cause errors to parse (default value supplied by accessor are used)
• are an open set that can be extended with more pairs by users
• are not exposed in APIs when confidential (unless a user has ALL authority)
• JSON type used depends on provided default value otherwise pattern matching, string as last resort

Usage

UserSettings: Access can (and must) be static (since we need that in BaseIdentifiableObject.getTranslation therefore the static SessionUserSettings solution)

UserSettings settings = UserSettings.getCurrentSettings();

SystemSettings: Access must use SystemSettingsProvider (or SystemSettingsService) as the initialisation requires beans

SystemSettings settings = settingsProvider.getCurrentSettings();

Settings are created from a Map<String, String> using UserSettings.of and SystemSettings.of. This is used when instances are created from DB but can also be used to manually create them in tests. As settings use interfaces they can also be mocked (not that I recommend it).

API Nuances

Settings and translations for settings are not a single feature but two features.
To request a translation a locale must be specified to indicate the translation endpoint.
To access the translation in the current user's locale the locale is provided as empty, like so &locale=.
This means /api/systemSettings/{key} without a locale will not return a translation in the current user's locale, whereas /api/systemSettings/{key}?locale= will.

Representation

Settings are key-value pairs. Both keys and values are Strings in DB. In code the raw String value is converted (parsed) into different types on access. LazySettings are used for both SystemSettings and UserSettings as the class implements a general set of settings using a String[] for both keys and another for raw values. The arrays are sorted by keys in alphabetical order to allow O(log n) access while having a minimal memory footprint. A Serializable[] holds the converted typed values. The conversion is encoded in the specific accessor methods of SystemSettings and UserSettings interfaces which implement these as default methods based on the abstract methods defined by Settings which LazySettings implement.

This means settings can generally hold any set of settings. This mainly targets the settings used in the backend code for which there are accessor methods. Through these their type and default is known. Settings without such method are not used in the backend and therefore always stay String.

This means both SettingKey and UserSettingKey enums have been removed. Their type and default value have become the default methods in SystemSettings and UserSettings interfaces.

Life-cycle

Settings are always used as sets of settings not individual values. The entire set is an immutable value. If a value in the set is outdated the entire set is reloaded. The re-leading differs for SystemSettings and UserSettings.

For SystemSettings the service holds the recent set in a field. Being immutable the instance can be shared freely. When it becomes outdated by a write the field is set null causing the next access to reload it from DB. In addition the set is kept in a ThreadLocal for each thread that is initialized implicitly on access so that during a request the settings always stay constant.

For UserSettings the SessionUserSettings holds the settings for the duration of the user session. The set includes the fallbacks to the values of the SystemSettings. In addition the set it kept in a ThreadLoacal (in ThreadUserSettings) for each thread that is cleared at the start and end of each request and initialized on access from the SessionUserSettings. This way the UserSettings.getCurrentSettings() never changes during a request and allows to override individual settings just in the scope of a request.

Confidential

Some settings are considered a secret and are therefore encoded.
This affects the value as stored in DB. When settings are in memory in form of a LazySettings object the values are always decoded (plain). Confidential settings are marked as such by annotating their accessor method using a new annotation @Confidential. A confidential setting is not included in the set of settings accessible through the API. The only exception is plain single key access as superuser (ALL) allows to read any setting to preserve existing behaviour.

Manipulation

Settings are handled as key-value pairs. This is equally reflected in the API to manipulate them as they ask for keys and values as a Map or as a single key and value.

When updating a setting value the value needs to be presented as a String. However, as settings were converted between several classes and their String a conversion other than toString() is required. This is handled by Settings.valueOf method. When individual settings are passed to a service this conversion is implicit and does not need to be applied by the caller.

Translations

The translations feature has been extracted to a new dedicated interface SystemSettingsTranslationService. The implementation is now backed by the datastore (the translations are no longer saved in the systemsetting table).
The translations use the namespace settings-translations which is protected with the existing authority required to use settings F_SYSTEM_SETTING. Internally reading the translation via the service will use the SystemUser to make access possible without having the authority. Direct read access through the datastore however will require the authority.

Set vs Effective Settings

For settings that do have a default value the effective value might not be the one set in case it fails to parse to the requested target type which will then use the default value.

Therefore the toJson which is used to support the REST API returns the effective values whereas toMap returns the values as present (set).

Automatic Tests

A LOT of tests need to be modified to make this work. The main reasons are API changes to set and get settings and that system settings are now stable within a request. While this is a good feature in production in tests this tends to work against the intent since a change to settings only becomes visible in the thread after calling SystemSettingsService.clearCurrentSettings. But this has worked in a positive way still as it made visible which tests do depend on settings without previously showing this in the code. In that sense many tests have become more descriptive.

New unit tests were added for the code that is entirely new. I removed any settings store level tests and replaced it with service level tests. I also removed several tests methods in other tests that no longer apply as the scenario does not exist any longer due to semantic changes of the settings when it comes to null-ness.

Manual Tests

Settings affect most features of the server. A few obvious things to test

• system settings app
• user settings app
• data administration app; run some data integrity checks, resource table and analytics table generation

Next

• The DB mapped types SystemSetting and UserSetting are no longer used I think, we might actually not need them any more. Needs to be checked, the schema might be the last reason to have them.
• The are settings which remember facts about past or future events. As such they clearly are not actual settings as you cannot chose (configure) something else in a meaningful way as a user. These should be moved to a different tech, most likely the datatstore.
• provide a settings design in package-info.java (I started)
• set vs effective settings API support
• identify and remove unused settings

[david-mackessy]

First time seeing an index used like this. This might not be clear to readers who don't know how pre/post increment works. Just an observation.

[jbee]

Hehe, then it is good you haven't looked at some of the parser code I wrote :) But for clarity we can move the increment out to its own statement.

[david-mackessy]

did we always allow storing json as a system setting? This seems like something that would be more suited in the datastore.

[jbee]

The toJson (and asJson) part is purely to not break our REST API where settings were exposed as JSON in some cases and so I tried to have them stay in the original JSON type. I would hope that we can simplify this at some point and just treat all of them as JSON string as well. As you say JSON values better fit the datastore and as of now there are no complex values any more in settings.

[david-mackessy]

I wonder would it be useful to have all system namespaces declared somewhere so we know what namespaces are used? Kind of like authorities. So they're not scattered around the codebase.

[jbee]

Maybe. Something that needs some consideration and time to arrive at a conclusion.

[david-mackessy]

looks nice Jan 👍
I have a question about translations but maybe we can chat at the next meeting. When I hear translations, I think there's 1 service that looks after that, but I see we have a settings translation service here.

[jbee]

I have a question about translations but maybe we can chat at the next meeting. When I hear translations, I think there's 1 service that looks after that, but I see we have a settings translation service here.

Yes, lets do that. You are fully correct that ideally we have a universal translation service not one scoped to settings. But that is a another very big step.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
11 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarCloud