Repository: bwplotka/mdox
Pull request #94 (2022-07-29 14:55:38)

Isolate and refactor caching logic
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

The cache logic has been moved to its own package.

The refactor improves on: 

- Using a typed string for the cache type, to provide some additional safety. 
- Makes it harder to access the underlying string fields of configuration options that are time durations to, to help avoiding mistakes.

## Breaking change 

* This PR changes the SQLite cache type configuration option from `SQLite` to `sqlite`. Helps avoid issues with upper/lower case mistakes.

douglascamata: @bwplotka feedback taken care of. 👍 🎉 

Patch:
diff --git a/go.mod b/go.mod
index a8b807a..26584e3 100644
--- a/go.mod
+++ b/go.mod
@@ -18,6 +18,7 @@ require (
 	github.com/mattn/go-shellwords v1.0.12
 	github.com/mattn/go-sqlite3 v1.14.17
 	github.com/oklog/run v1.1.0
+	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.16.0
 	github.com/prometheus/common v0.44.0
 	github.com/sergi/go-diff v1.0.0
@@ -64,7 +65,6 @@ require (
 	github.com/niklasfasching/go-org v1.7.0 // indirect
 	github.com/olekukonko/tablewriter v0.0.5 // indirect
 	github.com/pelletier/go-toml/v2 v2.0.8 // indirect
-	github.com/pkg/errors v0.9.1 // indirect
 	github.com/prometheus/client_model v0.4.0 // indirect
 	github.com/prometheus/procfs v0.10.1 // indirect
 	github.com/rivo/uniseg v0.2.0 // indirect
diff --git a/pkg/cache/config.go b/pkg/cache/config.go
new file mode 100644
index 0000000..f901969
--- /dev/null
+++ b/pkg/cache/config.go
@@ -0,0 +1,101 @@
+// Copyright (c) Bartłomiej Płotka @bwplotka
+// Licensed under the Apache License 2.0.
+
+package cache
+
+import (
+	"time"
+
+	"github.com/pkg/errors"
+	"gopkg.in/yaml.v3"
+)
+
+type cacheType string
+
+const (
+	timeDay              = 24 * time.Hour
+	defaultCacheValidity = 5 * timeDay
+	cacheTypeEmpty       = cacheType("")
+	cacheTypeNone        = cacheType("none")
+	cacheTypeSQLite      = cacheType("sqlite")
+)
+
+// Config holds the cache configuration.
+type Config struct {
+	// type is the type of the cache.
+	cacheType cacheType
+	// Validity is the duration for which the cache is valid.
+	Validity time.Duration
+	// Jitter is the jitter to apply when considering a cached entry valid or not.
+	Jitter time.Duration
+
+	cacheParser *configParser
+}
+
+// NewConfig is the constructor for Config.
+func NewConfig() Config {
+	return Config{
+		cacheParser: newConfigParser(),
+	}
+}
+
+// IsSet tell whether a cache configuration is present.
+func (c *Config) IsSet() bool {
+	return c.cacheType != cacheTypeNone && c.cacheType != cacheTypeEmpty
+}
+
+// UnmarshalYAML puts the unmarshalled yaml data into the internal cache parser
+// struct. This prevents access to the string data of jitter and validity.
+func (c *Config) UnmarshalYAML(value *yaml.Node) error {
+	if err := value.Decode(c.cacheParser); err != nil {
+		return err
+	}
+	if err := c.load(); err != nil {
+		return err
+	}
+	return nil
+}
+
+// load validates the cache configuration from the parser and copy it
+// into the configuration.
+func (c *Config) load() error {
+	switch c.cacheParser.Type {
+	case cacheTypeSQLite:
+		if c.cacheParser.Validity != "" {
+			var err error
+			c.Validity, err = time.ParseDuration(c.cacheParser.Validity)
+			if err != nil {
+				return errors.Wrap(err, "parsing cache validity duration")
+			}
+		}
+
+		if c.cacheParser.Jitter != "" {
+			var err error
+			c.Jitter, err = time.ParseDuration(c.cacheParser.Jitter)
+			if err != nil {
+				return errors.Wrap(err, "parsing cache jitter duration")
+			}
+		}
+	case cacheTypeNone, cacheTypeEmpty:
+	default:
+		return errors.New("unsupported cache type")
+	}
+	c.cacheType = c.cacheParser.Type
+	return nil
+}
+
+// configParser represents a cache configuration that can be parsed.
+// These fields are not embed in a unified Config struct to avoid accidental
+// usage of the duration fields (i.e. Validity and Jitter) as strings.
+type configParser struct {
+	Type     cacheType `yaml:"type"`
+	Validity string    `yaml:"validity"`
+	Jitter   string    `yaml:"jitter"`
+}
+
+// newConfigParser is the constructor for ConfigParser.
+func newConfigParser() *configParser {
+	return &configParser{
+		Validity: defaultCacheValidity.String(),
+	}
+}
diff --git a/pkg/mdformatter/linktransformer/config.go b/pkg/mdformatter/linktransformer/config.go
index b98c664..8760583 100644
--- a/pkg/mdformatter/linktransformer/config.go
+++ b/pkg/mdformatter/linktransformer/config.go
@@ -13,13 +13,14 @@ import (
 	"regexp"
 	"time"
 
+	"github.com/bwplotka/mdox/pkg/cache"
 	"gopkg.in/yaml.v3"
 )
 
 type Config struct {
 	Version int
 
-	Cache CacheConfig `yaml:"cache"`
+	Cache cache.Config `yaml:"cache"`
 
 	ExplicitLocalValidators bool              `yaml:"explicitLocalValidators"`
 	Validators              []ValidatorConfig `yaml:"validators"`
@@ -34,14 +35,6 @@ type Config struct {
 	randomDelay time.Duration
 }
 
-type CacheConfig struct {
-	Type     string `yaml:"type"`
-	Validity string `yaml:"validity"`
-	Jitter   string `yaml:"jitter"`
-	validity time.Duration
-	jitter   time.Duration
-}
-
 type ValidatorConfig struct {
 	// Regex for type of validator. For `githubPullsIssues` this is: (^http[s]?:\/\/)(www\.)?(github\.com\/){ORG_NAME}\/{REPO_NAME}(\/pull\/|\/issues\/).
 	Regex string `yaml:"regex"`
@@ -78,12 +71,6 @@ const (
 
 const (
 	gitHubAPIURL = "https://api.github.com/repos/%v/%v?sort=created&direction=desc&per_page=1"
-
-	none   = "None"
-	sqlite = "SQLite"
-
-	timeDay              = 24 * time.Hour
-	defaultCacheValidity = 5 * timeDay
 )
 
 type GitHubResponse struct {
@@ -91,7 +78,7 @@ type GitHubResponse struct {
 }
 
 func ParseConfig(c []byte) (Config, error) {
-	cfg := Config{Cache: CacheConfig{Validity: defaultCacheValidity.String()}}
+	cfg := Config{Cache: cache.NewConfig()}
 	dec := yaml.NewDecoder(bytes.NewReader(c))
 	dec.KnownFields(true)
 	if err := dec.Decode(&cfg); err != nil {
@@ -118,28 +105,6 @@ func ParseConfig(c []byte) (Config, error) {
 		return Config{}, errors.New("parsing parallelism, has to be > 0")
 	}
 
-	switch cfg.Cache.Type {
-	case sqlite:
-		if cfg.Cache.Validity != "" {
-			var err error
-			cfg.Cache.validity, err = time.ParseDuration(cfg.Cache.Validity)
-			if err != nil {
-				return Config{}, fmt.Errorf("parsing cache validity duration: %w", err)
-			}
-		}
-
-		if cfg.Cache.Jitter != "" {
-			var err error
-			cfg.Cache.jitter, err = time.ParseDuration(cfg.Cache.Jitter)
-			if err != nil {
-				return Config{}, fmt.Errorf("parsing cache jitter duration: %w", err)
-			}
-		}
-	case none, "":
-	default:
-		return Config{}, errors.New("unsupported cache type")
-	}
-
 	if len(cfg.Validators) <= 0 {
 		return cfg, nil
 	}
diff --git a/pkg/mdformatter/linktransformer/link.go b/pkg/mdformatter/linktransformer/link.go
index 837e076..7460855 100644
--- a/pkg/mdformatter/linktransformer/link.go
+++ b/pkg/mdformatter/linktransformer/link.go
@@ -264,9 +264,9 @@ func NewValidator(ctx context.Context, logger log.Logger, linksValidateConfig []
 		v.c.SetRequestTimeout(config.timeout)
 	}
 
-	if v.validateConfig.Cache.Type != none && storage != nil {
+	if v.validateConfig.Cache.IsSet() && storage != nil {
 		v.storage = storage
-		if err = v.storage.Init(v.validateConfig.Cache.validity, v.validateConfig.Cache.jitter); err != nil {
+		if err = v.storage.Init(v.validateConfig.Cache.Validity, v.validateConfig.Cache.Jitter); err != nil {
 			return nil, err
 		}
 	}


Test patch:
diff --git a/pkg/mdformatter/linktransformer/link_test.go b/pkg/mdformatter/linktransformer/link_test.go
index deea3e8..8e156c1 100644
--- a/pkg/mdformatter/linktransformer/link_test.go
+++ b/pkg/mdformatter/linktransformer/link_test.go
@@ -386,7 +386,7 @@ func TestValidator_TransformDestination(t *testing.T) {
 		}
 
 		diff, err := mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer(
-			MustNewValidator(logger, []byte("version: 1\n\ncache:\n  type: 'SQLite'"), anchorDir, testStorage),
+			MustNewValidator(logger, []byte("version: 1\n\ncache:\n  type: 'sqlite'"), anchorDir, testStorage),
 		))
 
 		testutil.Ok(t, err)
@@ -420,9 +420,14 @@ func TestValidator_TransformDestination(t *testing.T) {
 			Filename: filepath.Join(tmpDir, "repo", "docs", "test", "mdoxcachetest2"),
 		}
 
-		diff, err := mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer(
-			MustNewValidator(logger, []byte("version: 1\n\ncache:\n  type: 'None'"), anchorDir, testStorage),
-		))
+		diff, err := mdformatter.IsFormatted(
+			context.TODO(),
+			logger,
+			[]string{testFile},
+			mdformatter.WithLinkTransformer(
+				MustNewValidator(logger, []byte("version: 1\n\ncache:\n  type: 'none'"), anchorDir, testStorage),
+			),
+		)
 
 		testutil.Ok(t, err)
 		testutil.Equals(t, 0, len(diff), diff.String())
@@ -451,7 +456,7 @@ func TestValidator_TransformDestination(t *testing.T) {
 		testutil.Equals(t, 0, len(diff), diff.String())
 
 		_, err = mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer(
-			MustNewValidator(logger, []byte("version: 1\n\ncache:\n  type: 'SQLite'"), anchorDir, testStorage),
+			MustNewValidator(logger, []byte("version: 1\n\ncache:\n  type: 'sqlite'"), anchorDir, testStorage),
 		))
 		testutil.NotOk(t, err)