Consolidate StoredRequest configs, add validation for all data types

config/config.go

@@ -29,18 +29,19 @@ type Configuration struct {
 	EnableGzip  bool       `mapstructure:"enable_gzip"`
 	// StatusResponse is the string which will be returned by the /status endpoint when things are OK.
 	// If empty, it will return a 204 with no content.
-	StatusResponse  string             `mapstructure:"status_response"`
-	AuctionTimeouts AuctionTimeouts    `mapstructure:"auction_timeouts_ms"`
-	CacheURL        Cache              `mapstructure:"cache"`
-	ExtCacheURL     ExternalCache      `mapstructure:"external_cache"`
-	RecaptchaSecret string             `mapstructure:"recaptcha_secret"`
-	HostCookie      HostCookie         `mapstructure:"host_cookie"`
-	Metrics         Metrics            `mapstructure:"metrics"`
-	DataCache       DataCache          `mapstructure:"datacache"`
-	StoredRequests  StoredRequests     `mapstructure:"stored_requests"`
-	CategoryMapping StoredRequestsSlim `mapstructure:"category_mapping"`
+	StatusResponse    string          `mapstructure:"status_response"`
+	AuctionTimeouts   AuctionTimeouts `mapstructure:"auction_timeouts_ms"`
+	CacheURL          Cache           `mapstructure:"cache"`
+	ExtCacheURL       ExternalCache   `mapstructure:"external_cache"`
+	RecaptchaSecret   string          `mapstructure:"recaptcha_secret"`
+	HostCookie        HostCookie      `mapstructure:"host_cookie"`
+	Metrics           Metrics         `mapstructure:"metrics"`
+	DataCache         DataCache       `mapstructure:"datacache"`
+	StoredRequests    StoredRequests  `mapstructure:"stored_requests"`
+	StoredRequestsAMP StoredRequests  `mapstructure:"stored_amp_req"`
+	CategoryMapping   StoredRequests  `mapstructure:"category_mapping"`
 	// Note that StoredVideo refers to stored video requests, and has nothing to do with caching video creatives.
-	StoredVideo StoredRequestsSlim `mapstructure:"stored_video_req"`
+	StoredVideo StoredRequests `mapstructure:"stored_video_req"`
 
 	// Adapters should have a key for every openrtb_ext.BidderName, converted to lower-case.
 	// Se also: https://github.com/spf13/viper/issues/371#issuecomment-335388559
@@ -103,7 +104,10 @@ func (c configErrors) Error() string {
 func (cfg *Configuration) validate() configErrors {
 	var errs configErrors
 	errs = cfg.AuctionTimeouts.validate(errs)
-	errs = cfg.StoredRequests.validate(errs)
+	errs = cfg.StoredRequests.validate("stored_req", errs)
+	errs = cfg.StoredRequestsAMP.validate("stored_amp_req", errs)
+	errs = cfg.CategoryMapping.validate("categories", errs)
+	errs = cfg.StoredVideo.validate("stored_video_req", errs)
 	errs = cfg.Metrics.validate(errs)
 	if cfg.MaxRequestSize < 0 {
 		errs = append(errs, fmt.Errorf("cfg.max_request_size must be >= 0. Got %d", cfg.MaxRequestSize))
@@ -599,6 +603,9 @@ func New(v *viper.Viper) (*Configuration, error) {
 		c.BlacklistedAcctMap[c.BlacklistedAccts[i]] = true
 	}
 
+	// Migrate combo stored request config to separate stored_reqs and amp stored_reqs configs.
+	resolvedStoredRequestsConfig(&c)
+
 	glog.Info("Logging the resolved configuration:")
 	logGeneral(reflect.ValueOf(c), "  \t")
 	if errs := c.validate(); len(errs) > 0 {
@@ -716,6 +723,7 @@ func SetupViper(v *viper.Viper, filename string) {
 		v.AddConfigPath(".")
 		v.AddConfigPath("/etc/config")
 	}
+
 	// Fixes #475: Some defaults will be set just so they are accessible via environment variables
 	// (basically so viper knows they exist)
 	v.SetDefault("external_url", "http://localhost:8000")
@@ -773,7 +781,8 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("category_mapping.filesystem.enabled", true)
 	v.SetDefault("category_mapping.filesystem.directorypath", "./static/category-mapping")
 	v.SetDefault("category_mapping.http.endpoint", "")
-	v.SetDefault("stored_requests.filesystem", false)
+	v.SetDefault("stored_requests.filesystem.enabled", false)
+	v.SetDefault("stored_requests.filesystem.directorypath", "./stored_requests/data/by_id")
 	v.SetDefault("stored_requests.directorypath", "./stored_requests/data/by_id")
 	v.SetDefault("stored_requests.postgres.connection.dbname", "")
 	v.SetDefault("stored_requests.postgres.connection.host", "")
@@ -989,6 +998,22 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetEnvPrefix("PBS")
 	v.AutomaticEnv()
 	v.ReadInConfig()
+
+	// Migrate config settings to maintain compatibility with old configs
+	migrateConfig(v)
+}
+
+func migrateConfig(v *viper.Viper) {
+	// if stored_requests.filesystem is not a map in conf file as expected from defaults,
+	// means we have old-style settings; migrate them to new filesystem map to avoid breaking viper
+	if _, ok := v.Get("stored_requests.filesystem").(map[string]interface{}); !ok {
+		glog.Warning("stored_requests.filesystem should be changed to stored_requests.filesystem.enabled")
+		glog.Warning("stored_requests.directorypath should be changed to stored_requests.filesystem.directorypath")
+		m := v.GetStringMap("stored_requests.filesystem")
+		m["enabled"] = v.GetBool("stored_requests.filesystem")
+		m["directorypath"] = v.GetString("stored_requests.directorypath")
+		v.Set("stored_requests.filesystem", m)
+	}
 }
 
 func setBidderDefaults(v *viper.Viper, bidder string) {

config/config_test.go

@@ -3,6 +3,7 @@ package config
 import (
 	"bytes"
 	"net"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -120,6 +121,8 @@ func TestDefaults(t *testing.T) {
 	cmpBools(t, "account_adapter_details", cfg.Metrics.Disabled.AccountAdapterDetails, false)
 	cmpBools(t, "adapter_connections_metrics", cfg.Metrics.Disabled.AdapterConnectionMetrics, true)
 	cmpStrings(t, "certificates_file", cfg.PemCertsFile, "")
+	cmpBools(t, "stored_requests.filesystem.enabled", false, cfg.StoredRequests.Files.Enabled)
+	cmpStrings(t, "stored_requests.filesystem.directorypath", "./stored_requests/data/by_id", cfg.StoredRequests.Files.Path)
 }
 
 var fullConfig = []byte(`
@@ -271,6 +274,12 @@ adapters:
      usersync_url: http:\\tag.adkernel.com/syncr?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&r=
 `)
 
+var oldStoredRequestsConfig = []byte(`
+stored_requests:
+  filesystem: true
+  directorypath: "/somepath"
+`)
+
 func cmpStrings(t *testing.T, key string, a string, b string) {
 	t.Helper()
 	assert.Equal(t, a, b, "%s: %s != %s", key, a, b)
@@ -423,17 +432,53 @@ func TestUnmarshalAdapterExtraInfo(t *testing.T) {
 func TestValidConfig(t *testing.T) {
 	cfg := Configuration{
 		StoredRequests: StoredRequests{
-			Files: true,
+			Files: FileFetcherConfig{Enabled: true},
+			InMemoryCache: InMemoryCache{
+				Type: "none",
+			},
+		},
+		StoredVideo: StoredRequests{
+			Files: FileFetcherConfig{Enabled: true},
 			InMemoryCache: InMemoryCache{
 				Type: "none",
 			},
 		},
+		CategoryMapping: StoredRequests{
+			Files: FileFetcherConfig{Enabled: true},
+		},
 	}
 
+	resolvedStoredRequestsConfig(&cfg)
 	err := cfg.validate()
 	assert.Nil(t, err, "OpenRTB filesystem config should work. %v", err)
 }
 
+func TestMigrateConfig(t *testing.T) {
+	v := viper.New()
+	SetupViper(v, "")
+	v.SetConfigType("yaml")
+	v.ReadConfig(bytes.NewBuffer(oldStoredRequestsConfig))
+	migrateConfig(v)
+	cfg, err := New(v)
+	assert.NoError(t, err, "Setting up config should work but it doesn't")
+	cmpBools(t, "stored_requests.filesystem.enabled", true, cfg.StoredRequests.Files.Enabled)
+	cmpStrings(t, "stored_requests.filesystem.path", "/somepath", cfg.StoredRequests.Files.Path)
+}
+
+func TestMigrateConfigFromEnv(t *testing.T) {
+	if oldval, ok := os.LookupEnv("PBS_STORED_REQUESTS_FILESYSTEM"); ok {
+		defer os.Setenv("PBS_STORED_REQUESTS_FILESYSTEM", oldval)
+	} else {
+		defer os.Unsetenv("PBS_STORED_REQUESTS_FILESYSTEM")
+	}
+	os.Setenv("PBS_STORED_REQUESTS_FILESYSTEM", "true")
+	v := viper.New()
+	SetupViper(v, "")
+	cfg, err := New(v)
+	assert.NoError(t, err, "Setting up config should work but it doesn't")
+	cmpBools(t, "stored_requests.filesystem.enabled", true, cfg.StoredRequests.Files.Enabled)
+}
+
 func TestInvalidAdapterEndpointConfig(t *testing.T) {
 	v := viper.New()
 	SetupViper(v, "")