Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default account configuration and account status support #959

Merged
merged 6 commits into from
Jan 15, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/application-settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ There are two ways to configure application settings: database and file. This do
- `default-integration` - Default integration to assume.
- `analytics-config.auction-events.<channel>` - defines which channels are supported by analytics for this account
- `bid-validations.banner-creative-max-size` - Overrides creative max size validation for banners.
- `status` - allows to mark account as `active` or `inactive`.

```
Purpose | Purpose goal | Purpose meaning for PBS (n\a - not affected)
Expand Down Expand Up @@ -70,6 +71,7 @@ accounts:
analytics-config:
auction-events:
amp: true
status: active
gdpr:
enabled: true
integration-enabled:
Expand Down Expand Up @@ -359,7 +361,7 @@ Query used to get an account:

```sql
SELECT uuid, price_granularity, banner_cache_ttl, video_cache_ttl, events_enabled, enforce_ccpa, tcf_config,
analytics_sampling_factor, truncate_target_attr, default_integration, analytics_config, bid_validations
analytics_sampling_factor, truncate_target_attr, default_integration, analytics_config, bid_validations, status
FROM accounts_account where uuid = %ACCOUNT_ID%
LIMIT 1
```
14 changes: 14 additions & 0 deletions docs/config-app.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,20 @@ For HTTP data source available next options:
For account processing rules available next options:
- `settings.enforce-valid-account` - if equals to `true` then request without account id will be rejected with 401.

It is possible to specify default account configuration values that will be assumed if account config have them
unspecified or missing at all. Example:
```yaml
settings:
default-account-config:
events-enabled: true
enforce-ccpa: true
gdpr: '{"enabled": true}'
analytics-sampling-factor: 1
default-integration: pbjs
analytics-config: '{"auction-events":{"amp":true}}'
```
See [application settings](application-settings.md) for full reference of available configuration parameters.

For caching available next options:
- `settings.in-memory-cache.ttl-seconds` - how long (in seconds) data will be available in LRU cache.
- `settings.in-memory-cache.cache-size` - the size of LRU cache.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import org.prebid.server.proto.openrtb.ext.response.BidType;
import org.prebid.server.settings.ApplicationSettings;
import org.prebid.server.settings.model.Account;
import org.prebid.server.settings.model.AccountStatus;
import org.prebid.server.util.HttpUtil;
import org.prebid.server.validation.RequestValidator;
import org.prebid.server.validation.model.ValidationResult;
Expand Down Expand Up @@ -778,7 +779,7 @@ private Future<Account> accountFrom(BidRequest bidRequest, Timeout timeout, Rout
return blankAccountId
? responseForEmptyAccount(routingContext)
: applicationSettings.getAccountById(accountId, timeout)
.recover(exception -> accountFallback(exception, accountId, routingContext));
.compose(this::ensureAccountActive, exception -> accountFallback(exception, accountId, routingContext));
}

/**
Expand Down Expand Up @@ -844,6 +845,15 @@ private Future<Account> responseForUnknownAccount(String accountId) {
: Future.succeededFuture(Account.empty(accountId));
}

private Future<Account> ensureAccountActive(Account account) {
final String accountId = account.getId();

return account.getStatus() == AccountStatus.inactive
? Future.failedFuture(
new UnauthorizedAccountException(String.format("Account %s is inactive", accountId), accountId))
: Future.succeededFuture(account);
}

private BidRequest enrichBidRequestWithAccountAndPrivacyData(
BidRequest bidRequest, Account account, PrivacyContext privacyContext) {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package org.prebid.server.settings;

import io.vertx.core.Future;
import org.prebid.server.execution.Timeout;
import org.prebid.server.settings.model.Account;
import org.prebid.server.settings.model.StoredDataResult;
import org.prebid.server.settings.model.StoredResponseDataResult;

import java.util.Objects;
import java.util.Set;

public class EnrichingApplicationSettings implements ApplicationSettings {

private final ApplicationSettings delegate;
private final Account defaultAccount;

public EnrichingApplicationSettings(ApplicationSettings delegate, Account defaultAccount) {
this.delegate = Objects.requireNonNull(delegate);
this.defaultAccount = Objects.equals(Account.builder().build(), defaultAccount) ? null : defaultAccount;
}

@Override
public Future<Account> getAccountById(String accountId, Timeout timeout) {
final Future<Account> accountFuture = delegate.getAccountById(accountId, timeout);

if (defaultAccount == null) {
return accountFuture;
}

return accountFuture
.map(account -> account.merge(defaultAccount))
.otherwise(Account.empty(accountId).merge(defaultAccount));
}

@Override
public Future<String> getAdUnitConfigById(String adUnitConfigId, Timeout timeout) {
return delegate.getAdUnitConfigById(adUnitConfigId, timeout);
}

@Override
public Future<StoredDataResult> getStoredData(String accountId,
Set<String> requestIds,
Set<String> impIds,
Timeout timeout) {

return delegate.getStoredData(accountId, requestIds, impIds, timeout);
}

@Override
public Future<StoredResponseDataResult> getStoredResponses(Set<String> responseIds, Timeout timeout) {
return delegate.getStoredResponses(responseIds, timeout);
}

@Override
public Future<StoredDataResult> getAmpStoredData(String accountId,
Set<String> requestIds,
Set<String> impIds,
Timeout timeout) {

return delegate.getAmpStoredData(accountId, requestIds, impIds, timeout);
}

@Override
public Future<StoredDataResult> getVideoStoredData(String accountId,
Set<String> requestIds,
Set<String> impIds,
Timeout timeout) {

return delegate.getVideoStoredData(accountId, requestIds, impIds, timeout);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.prebid.server.settings.model.AccountAnalyticsConfig;
import org.prebid.server.settings.model.AccountBidValidationConfig;
import org.prebid.server.settings.model.AccountGdprConfig;
import org.prebid.server.settings.model.AccountStatus;
import org.prebid.server.settings.model.StoredDataResult;
import org.prebid.server.settings.model.StoredResponseDataResult;
import org.prebid.server.vertx.jdbc.JdbcClient;
Expand Down Expand Up @@ -108,7 +109,8 @@ public JdbcApplicationSettings(JdbcClient jdbcClient,
*/
@Override
public Future<Account> getAccountById(String accountId, Timeout timeout) {
return jdbcClient.executeQuery(selectAccountQuery,
return jdbcClient.executeQuery(
selectAccountQuery,
Collections.singletonList(accountId),
result -> mapToModelOrError(result, row -> Account.builder()
.id(row.getString(0))
Expand All @@ -123,6 +125,7 @@ public Future<Account> getAccountById(String accountId, Timeout timeout) {
.defaultIntegration(row.getString(9))
.analyticsConfig(toModel(row.getString(10), AccountAnalyticsConfig.class))
.bidValidations(toModel(row.getString(11), AccountBidValidationConfig.class))
.status(toAccountStatus(row.getString(12)))
.build()),
timeout)
.compose(result -> failedIfNull(result, accountId, "Account"));
Expand Down Expand Up @@ -171,6 +174,14 @@ private <T> T toModel(String source, Class<T> targetClass) {
}
}

private static AccountStatus toAccountStatus(String status) {
try {
return AccountStatus.valueOf(status);
} catch (IllegalArgumentException e) {
throw new PreBidException(e.getMessage());
}
}

/**
* Runs a process to get stored requests by a collection of ids from database
* and returns {@link Future&lt;{@link StoredDataResult }&gt;}.
Expand Down
22 changes: 22 additions & 0 deletions src/main/java/org/prebid/server/settings/model/Account.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import lombok.Builder;
import lombok.Value;
import org.apache.commons.lang3.ObjectUtils;

@Builder
@Value
Expand Down Expand Up @@ -31,6 +32,27 @@ public class Account {

AccountBidValidationConfig bidValidations;

AccountStatus status;

public Account merge(Account another) {
return Account.builder()
.id(ObjectUtils.firstNonNull(id, another.id))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor. i'd rather suggest to use ObjectUtils.defaultIfNull(..) instead since we have only 2 possible values.

.priceGranularity(ObjectUtils.firstNonNull(priceGranularity, another.priceGranularity))
.bannerCacheTtl(ObjectUtils.firstNonNull(bannerCacheTtl, another.bannerCacheTtl))
.videoCacheTtl(ObjectUtils.firstNonNull(videoCacheTtl, another.videoCacheTtl))
.eventsEnabled(ObjectUtils.firstNonNull(eventsEnabled, another.eventsEnabled))
.enforceCcpa(ObjectUtils.firstNonNull(enforceCcpa, another.enforceCcpa))
.gdpr(ObjectUtils.firstNonNull(gdpr, another.gdpr))
.analyticsSamplingFactor(ObjectUtils.firstNonNull(
analyticsSamplingFactor, another.analyticsSamplingFactor))
.truncateTargetAttr(ObjectUtils.firstNonNull(truncateTargetAttr, another.truncateTargetAttr))
.defaultIntegration(ObjectUtils.firstNonNull(defaultIntegration, another.defaultIntegration))
.analyticsConfig(ObjectUtils.firstNonNull(analyticsConfig, another.analyticsConfig))
.bidValidations(ObjectUtils.firstNonNull(bidValidations, another.bidValidations))
.status(ObjectUtils.firstNonNull(status, another.status))
.build();
}

public static Account empty(String id) {
return Account.builder()
.id(id)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package org.prebid.server.settings.model;

public enum AccountStatus {

active, inactive
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
import org.prebid.server.settings.ApplicationSettings;
import org.prebid.server.settings.CachingApplicationSettings;
import org.prebid.server.settings.CompositeApplicationSettings;
import org.prebid.server.settings.EnrichingApplicationSettings;
import org.prebid.server.settings.FileApplicationSettings;
import org.prebid.server.settings.HttpApplicationSettings;
import org.prebid.server.settings.JdbcApplicationSettings;
import org.prebid.server.settings.SettingsCache;
import org.prebid.server.settings.service.HttpPeriodicRefreshService;
import org.prebid.server.settings.service.JdbcPeriodicRefreshService;
import org.prebid.server.spring.config.model.AccountConfigurationProperties;
import org.prebid.server.spring.config.model.CircuitBreakerProperties;
import org.prebid.server.vertx.ContextRunner;
import org.prebid.server.vertx.http.HttpClient;
Expand Down Expand Up @@ -326,21 +328,41 @@ CompositeApplicationSettings compositeApplicationSettings(
}
}

@Configuration
static class EnrichingSettingsConfiguration {

@Bean
@ConfigurationProperties("settings.default-account-config")
AccountConfigurationProperties defaultAccountConfigurationProperties() {
return new AccountConfigurationProperties();
}

@Bean
EnrichingApplicationSettings enrichingApplicationSettings(
CompositeApplicationSettings compositeApplicationSettings,
AccountConfigurationProperties defaultAccountConfigurationProperties,
JacksonMapper mapper) {

return new EnrichingApplicationSettings(
compositeApplicationSettings, defaultAccountConfigurationProperties.toAccount(mapper));
}
}

@Configuration
static class CachingSettingsConfiguration {

@Bean
@ConditionalOnProperty(prefix = "settings.in-memory-cache", name = {"ttl-seconds", "cache-size"})
CachingApplicationSettings cachingApplicationSettings(
CompositeApplicationSettings compositeApplicationSettings,
EnrichingApplicationSettings enrichingApplicationSettings,
ApplicationSettingsCacheProperties cacheProperties,
@Qualifier("settingsCache") SettingsCache cache,
@Qualifier("ampSettingsCache") SettingsCache ampCache,
@Qualifier("videoSettingCache") SettingsCache videoCache,
Metrics metrics) {

return new CachingApplicationSettings(
compositeApplicationSettings,
enrichingApplicationSettings,
cache,
ampCache,
videoCache,
Expand All @@ -356,8 +378,8 @@ static class ApplicationSettingsConfiguration {
@Bean
ApplicationSettings applicationSettings(
@Autowired(required = false) CachingApplicationSettings cachingApplicationSettings,
@Autowired(required = false) CompositeApplicationSettings compositeApplicationSettings) {
return ObjectUtils.defaultIfNull(cachingApplicationSettings, compositeApplicationSettings);
EnrichingApplicationSettings enrichingApplicationSettings) {
return ObjectUtils.defaultIfNull(cachingApplicationSettings, enrichingApplicationSettings);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package org.prebid.server.spring.config.model;

import lombok.Data;
import lombok.NoArgsConstructor;
import org.prebid.server.json.JacksonMapper;
import org.prebid.server.settings.model.Account;
import org.prebid.server.settings.model.AccountAnalyticsConfig;
import org.prebid.server.settings.model.AccountGdprConfig;
import org.prebid.server.settings.model.AccountStatus;
import org.springframework.validation.annotation.Validated;

@Validated
Copy link
Contributor

@DGarbar DGarbar Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this annotation required ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It's been copied along with other annotations that we put on Spring configuration properties POJOs but apparently there are no constraints imposed on any fields in this class so it's redundant. We may put sensible constraints in future and easily overlook adding @Validated annotation though so I'm hesitant about this one.

@Data
@NoArgsConstructor
public class AccountConfigurationProperties {

private String priceGranularity;

private Integer bannerCacheTtl;

private Integer videoCacheTtl;

private Boolean eventsEnabled;

private Boolean enforceCcpa;

private String gdpr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cant we use AccountGdprConfig class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we don't want to add @Data @NoArgsConstructor annotations to AccountGdprConfig, i.e. make it a Spring configuration properties POJO.

Copy link
Contributor

@DGarbar DGarbar Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you mean AccountAnalyticsConfig because AccountGdprConfig is already have this annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. AccountGdprConfig shouldn't have had these annotations in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably my mistake. I copy from GdprConfig which we get from yaml.
But why we don't want to make this @DaTa ? Because of mutability ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a convention for value objects throughout PBS codebase to aspire for immutability unless it is impractical.

Another reason to take into consideration - using AccountGdprConfig to represent data in two different places (PBS configuration and account configuration in DB/file/etc) strongly couples these sources and forces changes in one place (account config for example) to affect another place (PBS config) and vice versa. Actually This is what Single Responsibility Principle is called to protect from.


private Integer analyticsSamplingFactor;

private Integer truncateTargetAttr;

private String defaultIntegration;

private String analyticsConfig;

private AccountStatus status;

public Account toAccount(JacksonMapper mapper) {
return Account.builder()
.priceGranularity(getPriceGranularity())
.bannerCacheTtl(getBannerCacheTtl())
.videoCacheTtl(getVideoCacheTtl())
.eventsEnabled(getEventsEnabled())
.enforceCcpa(getEnforceCcpa())
.gdpr(toModel(mapper, getGdpr(), AccountGdprConfig.class))
.analyticsSamplingFactor(getAnalyticsSamplingFactor())
.truncateTargetAttr(getTruncateTargetAttr())
.defaultIntegration(getDefaultIntegration())
.analyticsConfig(toModel(mapper, getAnalyticsConfig(), AccountAnalyticsConfig.class))
.status(getStatus())
.build();
}

private static <T> T toModel(JacksonMapper mapper, String source, Class<T> targetClass) {
return source != null ? mapper.decodeValue(source, targetClass) : null;
}
}
Loading