Skip to content

feat: use go-playground/validator #8

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.

Sign up for GitHub

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

Open
jrschumacher wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: use go-playground/validator #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
156 changes: 45 additions & 111 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,17 @@ package config

import (
"fmt"
"net/url"
"os"
"slices"
"strings"

"github.com/go-playground/validator/v10"
"gopkg.in/yaml.v3"
)

type Rule struct {
Name string `yaml:"name,omitempty"`
Component string `yaml:"component,omitempty"`
Copy link
Contributor Author

@jrschumacher jrschumacher Jan 5, 2025

Choose a reason for hiding this comment

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

@Fesaa can this really be empty? All the checks are expecting the component to be defined https://github.com/Fesaa/iCalMerger/blob/master/ical/checks.go#L71

Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

The firstOfX rules do not need a name, or component. They can stay as omitempty

Check string `yaml:"check"`
Check string `yaml:"check" validate:"required"`
Copy link
Contributor Author

@jrschumacher jrschumacher Jan 5, 2025

Choose a reason for hiding this comment

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

@Fesaa I'm not sure how the FirstOfYear, FirstOfMonth, and FirstOfDay work exactly. If Rules and modifiers can both use them then we can add a oneof.

Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

Yes, the filters inside a modifier can use all the rules avaible.

CaseSensitive bool `yaml:"case"`
Data []string `yaml:"data,omitempty"`
}
Expand Down Expand Up @@ -42,19 +41,40 @@ const (
)

type Modifier struct {
Name string `yaml:"name"`
Name string `yaml:"name" validate:"required"`
Component string `yaml:"component,omitempty"`
Action Action `yaml:"action"`
Data string `yaml:"data"`
Filters []Rule `yaml:"rules,omitempty"`
Action Action `yaml:"action" validate:"required,oneof=APPEND REPLACE PREPEND ALARM"`
Data string `yaml:"data" validate:"required"`
Filters []Rule `yaml:"rules,omitempty" validate:"omitempty,dive"`
}

type Config struct {
Hostname string `yaml:"hostname"`
Port string `yaml:"port"`
Hostname string `yaml:"hostname" validate:"omitempty,hostname"`
Port string `yaml:"port" validate:"number"`

Notification Notification `yaml:"notification"`
Sources []Source `yaml:"sources"`
Notification Notification `yaml:"notification" validate:"omitempty"`

// Unique endpoints will not return a very useful error message
Sources []Source `yaml:"sources" validate:"required,unique=EndPoint,dive"`
}

type Notification struct {
Url string `yaml:"url" validate:"required,url"`
Service string `yaml:"service" validate:"required,oneof=DISCORD"`
}

type Source struct {
EndPoint string `yaml:"end_point" validate:"required"`
Heartbeat int `yaml:"heartbeat" validate:"required,number,min=1"`
Name string `yaml:"xwr_name" validate:"required"`
Info []SourceInfo `yaml:"info" validate:"required,dive"`
}

type SourceInfo struct {
Name string `yaml:"name" validate:"required"`
Url string `yaml:"url" validate:"required,url"`
Rules []Rule `yaml:"rules,omitempty" validate:"omitempty,dive"`
Modifiers []Modifier `yaml:"modifiers,omitempty" validate:"omitempty,dive"`
}

var defaultConfig = Config{
Expand All @@ -81,108 +101,22 @@ func LoadConfig(filePath string) (*Config, error) {
config.Port = defaultConfig.Port
}

if err := config.Validate(); err != nil {
return nil, err
}

return config, nil
}

func (c *Config) Validate() error {
var endpoints []string

// Validate notification if set - not required
if c.Notification != (Notification{}) {
if err := c.Notification.Validate(); err != nil {
return fmt.Errorf(".Notification: %s", err)
}
}

for i, source := range c.Sources {
// Ensure that the endpoint is unique
if slices.Contains(endpoints, source.EndPoint) {
return fmt.Errorf(".Source.%d: EndPoint is not unique", i)
}
endpoints = append(endpoints, source.EndPoint)

if err := source.Validate(); err != nil {
return fmt.Errorf(".Source.%d: %s", i, err)
}
}

return nil
}

type Notification struct {
Url string `yaml:"url"`
Service string `yaml:"service"`
}

func (n *Notification) Validate() error {
if n.Url == "" {
return fmt.Errorf("url is missing")
}

if n.Service == "" {
return fmt.Errorf("service is missing")
}

n.Service = strings.ToUpper(n.Service)
switch NotificationService(n.Service) {
case NotifyDiscord:
break
default:
return fmt.Errorf("service is invalid")
}

return nil
}

type Source struct {
EndPoint string `yaml:"end_point"`
Heartbeat int `yaml:"heartbeat"`
Name string `yaml:"xwr_name"`
Info []SourceInfo `yaml:"info"`
}

func (c *Source) Validate() error {
if c.Heartbeat <= 0 {
return fmt.Errorf("heartbeat must be greater than 0")
}
validate := validator.New(validator.WithRequiredStructEnabled())
if err := validate.Struct(config); err != nil {
verr := err.(validator.ValidationErrors)
Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

Type assertion on errors fails on wrapped errors - my IDE

I don't know if it could ever return a wrapped error, but worth following.


for i, info := range c.Info {
if err := info.Validate(); err != nil {
return fmt.Errorf(".Info.%d: %s", i, err)
errs := make([]string, len(verr))
for i, e := range verr {
if e.StructNamespace() == "Config.Sources" {
if e.Tag() == "unique" {
errs[i] = fmt.Sprintf("%s.%s is not unique", e.Namespace(), e.Param())
}
} else {
errs[i] = fmt.Sprintf("%s is %s", e.Namespace(), e.Tag())
}
}
return nil, fmt.Errorf("config validation errors:\n%v", strings.Join(errs, "\n"))
Comment on lines +108 to +118
Copy link
Contributor Author

@jrschumacher jrschumacher Jan 5, 2025

Choose a reason for hiding this comment

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

This is required since the unique=EndPoint tag will not return a good error unlike the other validation tags. There is still no way to denote the location of the infringing config line (i.e. Sources[1].EndPoint) so the best we can do is

2025/01/05 10:00:41 ERROR Error loading config
2025/01/05 10:00:41 ERROR validation error: Config.Sources.EndPoint is not unique
panic: validation error: Config.Sources.EndPoint is not unique

goroutine 1 [running]:
main.main()
        /Users/ryan/Projects/iCalMerger/main.go:58 +0x4d4
exit status 2

Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

All good, clear enough

Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

Please use errors.Join instead of strings.Join.

And make errs a slice of errors, and then fmt.Errorf. Thanks

}

return nil
}

type SourceInfo struct {
Name string `yaml:"name"`
Url string `yaml:"url"`
Rules []Rule `yaml:"rules,omitempty"`
Modifiers []Modifier `yaml:"modifiers,omitempty"`
}

func (c *SourceInfo) Validate() error {
if c.Name == "" {
return fmt.Errorf("name is missing")
}

if c.Url == "" {
return fmt.Errorf("URL is missing")
}

u, err := url.Parse(c.Url)
if err != nil {
return fmt.Errorf("URL is invalid")
}

if u.Hostname() == "" {
return fmt.Errorf("URL is invalid (hostname)")
}

return nil
return config, nil
}
119 changes: 59 additions & 60 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import (
"testing"

"github.com/Fesaa/ical-merger/config"
"github.com/go-playground/validator/v10"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestLoadConfig(t *testing.T) {
Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

testing.T provides a t.TempDir() method, I would opt to use this as we're in a testing env.

Expand All @@ -19,6 +21,13 @@ func TestLoadConfig(t *testing.T) {
content := strings.Join([]string{
"hostname: example.com",
"port: 4040",
"sources:",
" - xwr_name: Source1",
" end_point: /endpoint1",
" heartbeat: 60",
" info:",
" - name: Info1",
" url: http://example.com/info1",
}, "\n")

if _, err := tempFile.Write([]byte(content)); err != nil {
Expand All @@ -33,9 +42,10 @@ func TestLoadConfig(t *testing.T) {

func TestConfigValidation(t *testing.T) {
cfg := &config.Config{
Port: "4040",
Sources: []config.Source{
{
EndPoint: "http://example.com/endpoint1",
EndPoint: "/endpoint1",
Heartbeat: 60,
Name: "Source1",
Info: []config.SourceInfo{
Expand All @@ -46,7 +56,7 @@ func TestConfigValidation(t *testing.T) {
},
},
{
EndPoint: "http://example.com/endpoint2",
EndPoint: "/endpoint2",
Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

Does the test fail without these changes? If so, great! If not, we'll have to make sure it does. And add a test for it.

Or change the code to allow it. But I believe it would break at this moment.

Heartbeat: 60,
Name: "Source2",
Info: []config.SourceInfo{
Expand All @@ -58,15 +68,26 @@ func TestConfigValidation(t *testing.T) {
},
},
}

err := cfg.Validate()
validate := validator.New()
Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

Use the same options (validator.WithRequiredStructEnabled()) as in the software

err := validate.Struct(cfg)
assert.NoError(t, err)
// Test port validation
cfg.Port = ""
err = validate.Struct(cfg)
require.Error(t, err)
assert.Contains(t, err.Error(), "'Port' failed on the 'number'")
// Test unique source endpoint
cfg.Port = "4040"
cfg.Sources[1].EndPoint = "/endpoint1"
err = validate.Struct(cfg)
require.Error(t, err)
assert.Contains(t, err.Error(), "'Sources' failed on the 'unique'")
}

func TestSourceValidation(t *testing.T) {
source := &config.Source{
EndPoint: "http://example.com/endpoint",
Heartbeat: 60,
EndPoint: "/endpoint",
Heartbeat: 1,
Name: "Source",
Info: []config.SourceInfo{
{
Expand All @@ -75,66 +96,44 @@ func TestSourceValidation(t *testing.T) {
},
},
}

err := source.Validate()
assert.NoError(t, err)
}

func TestSourceValidationHeartbeat(t *testing.T) {
source := &config.Source{
EndPoint: "http://example.com/endpoint",
Heartbeat: 0,
Name: "Source",
Info: []config.SourceInfo{
{
Name: "Info",
Url: "http://example.com/info",
},
},
}

err := source.Validate()
assert.Error(t, err)
assert.Equal(t, "heartbeat must be greater than 0", err.Error())
validate := validator.New()
err := validate.Struct(source)
require.NoError(t, err)
// Test endpoint validation
source.EndPoint = ""
err = validate.Struct(source)
require.Error(t, err)
assert.Contains(t, err.Error(), "'EndPoint' failed on the 'required'")
// Test heartbeat validation (0 is the same as missing)
source.EndPoint = "/endpoint"
source.Heartbeat = 0
err = validate.Struct(source)
require.Error(t, err)
assert.Contains(t, err.Error(), "'Heartbeat' failed on the 'required'")
// Test heartbeat validation
source.Heartbeat = -1
err = validate.Struct(source)
require.Error(t, err)
assert.Contains(t, err.Error(), "'Heartbeat' failed on the 'min'")
}

func TestSourceInfoValidation(t *testing.T) {
info := &config.SourceInfo{
Name: "Info",
Url: "http://example.com/info",
}

err := info.Validate()
validate := validator.New()
Copy link
Owner

@Fesaa Fesaa Jan 20, 2025

Choose a reason for hiding this comment

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

Same as above, make sure to use the same options for the validator (validator.WithRequiredStructEnabled())

err := validate.Struct(info)
assert.NoError(t, err)
}

func TestSourceInfoValidationMissingName(t *testing.T) {
info := &config.SourceInfo{
Url: "http://example.com/info",
}

err := info.Validate()
assert.Error(t, err)
assert.Equal(t, "name is missing", err.Error())
}

func TestSourceInfoValidationMissingUrl(t *testing.T) {
info := &config.SourceInfo{
Name: "Info",
}

err := info.Validate()
assert.Error(t, err)
assert.Equal(t, "URL is missing", err.Error())
}

func TestSourceInfoValidationInvalidUrl(t *testing.T) {
info := &config.SourceInfo{
Name: "Info",
Url: "invalid-url",
}

err := info.Validate()
assert.Error(t, err)
assert.Equal(t, "URL is invalid (hostname)", err.Error())
// Test URL validation
info.Url = "invalid-url"
err = validate.Struct(info)
require.Error(t, err)
assert.Contains(t, err.Error(), "'Url' failed on the 'url'")
// Test Name Missing
info.Url = "http://example.com/info"
info.Name = ""
err = validate.Struct(info)
require.Error(t, err)
assert.Contains(t, err.Error(), "'Name' failed on the 'required'")
}
9 changes: 9 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,20 @@ go 1.18

require (
github.com/arran4/golang-ical v0.2.8
github.com/go-playground/validator/v10 v10.23.0
github.com/stretchr/testify v1.10.0
gopkg.in/yaml.v3 v3.0.1
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/gabriel-vasile/mimetype v1.4.3 // indirect
github.com/go-playground/locales v0.14.1 // indirect
github.com/go-playground/universal-translator v0.18.1 // indirect
github.com/leodido/go-urn v1.4.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
golang.org/x/crypto v0.19.0 // indirect
golang.org/x/net v0.21.0 // indirect
golang.org/x/sys v0.17.0 // indirect
golang.org/x/text v0.14.0 // indirect
)
Loading
Loading