feat: use go-playground/validator by jrschumacher · Pull Request #8 · Fesaa/iCalMerger

jrschumacher · 2025-01-05T16:28:50Z

Closes #5

jrschumacher

@Fesaa There might be some additional checks you want to make, but I think this is a good start. The validator is good, but its a bit confusing if you don't know Go since it will format the errors like the struct not the yaml.

jrschumacher · 2025-01-05T16:31:29Z

main.go

+		slog.Error("Error loading config")
+		for _, l := range strings.Split(e.Error(), "\n") {
+			slog.Error(l)
+		}


This will give error messages like:

2025/01/05 10:07:38 ERROR Error loading config 2025/01/05 10:07:38 ERROR config validation errors: 2025/01/05 10:07:38 ERROR Config.Sources[1].Heartbeat is required 2025/01/05 10:07:38 ERROR Config.Sources[1].Name is required 2025/01/05 10:07:38 ERROR Config.Sources[1].Info is required panic: config validation errors: Config.Sources[1].Heartbeat is required Config.Sources[1].Name is required Config.Sources[1].Info is required goroutine 1 [running]: main.main() /Users/ryan/Projects/iCalMerger/main.go:58 +0x4d4 exit status 2

I'm not sure, I like the weird splitting on new line here.

After you've made the changes to []error slice, and using errors.Join. Can you show me what slog.Error("Error loading config", "error", e) looks like?

If it looks better by calling slog on each error, we should return a slice of errors instead of splitting on newline

jrschumacher · 2025-01-05T16:34:12Z

config/config.go

+		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"))


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

All good, clear enough

jrschumacher · 2025-01-05T16:37:22Z

config/config.go

 	Name          string   `yaml:"name,omitempty"`
 	Component     string   `yaml:"component,omitempty"`
-	Check         string   `yaml:"check"`
+	Check         string   `yaml:"check" validate:"required"`


@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.

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

jrschumacher · 2025-01-05T16:38:34Z

config/config.go


 type Rule struct {
 	Name          string   `yaml:"name,omitempty"`
 	Component     string   `yaml:"component,omitempty"`


@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

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

Fesaa · 2025-01-06T11:23:48Z

To keep you in the loop, my exams have/are starting so it may be a while before I can properly review this.

But from a glance it looks fine, but I haven't had the chance to pull it down and test yet.

jrschumacher · 2025-01-07T07:05:51Z

No worries. Just wanted to follow up on your request. Best of luck on your exams @Fesaa!

Fesaa

Amazing work, thanks!

Left some comments here and there that will need to be resolved before merging. I will look into the TODO's you've left in the tests (firstOf) after my exams and resolve them, thanks. Thanks for bringing these up, looks like test are amazing!

Fesaa · 2025-01-20T19:23:46Z

config/config.go


 type Rule struct {
 	Name          string   `yaml:"name,omitempty"`
 	Component     string   `yaml:"component,omitempty"`


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

Fesaa · 2025-01-20T19:25:13Z

config/config.go

 	Name          string   `yaml:"name,omitempty"`
 	Component     string   `yaml:"component,omitempty"`
-	Check         string   `yaml:"check"`
+	Check         string   `yaml:"check" validate:"required"`


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

Fesaa · 2025-01-20T19:26:58Z

config/config.go

-	}
+	validate := validator.New(validator.WithRequiredStructEnabled())
+	if err := validate.Struct(config); err != nil {
+		verr := err.(validator.ValidationErrors)


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.

Fesaa · 2025-01-20T19:27:55Z

config/config.go

+		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"))


All good, clear enough

Fesaa · 2025-01-20T19:29:21Z

config/config.go

+				errs[i] = fmt.Sprintf("%s is %s", e.Namespace(), e.Tag())
+			}
 		}
+		return nil, fmt.Errorf("config validation errors:\n%v", strings.Join(errs, "\n"))


Please use errors.Join instead of strings.Join.

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

Fesaa · 2025-01-20T19:30:23Z

config/config_test.go

 	}
-
-	err := cfg.Validate()
+	validate := validator.New()


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

Fesaa · 2025-01-20T19:31:46Z

config/config_test.go

+	"github.com/stretchr/testify/require"
 )

 func TestLoadConfig(t *testing.T) {


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

Fesaa · 2025-01-20T19:32:57Z

config/config_test.go

 			},
 			{
-				EndPoint:  "http://example.com/endpoint2",
+				EndPoint:  "/endpoint2",


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.

Fesaa · 2025-01-20T19:33:37Z

config/config_test.go

 	}
-
-	err := info.Validate()
+	validate := validator.New()


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

Fesaa · 2025-01-20T19:35:53Z

main.go

+		slog.Error("Error loading config")
+		for _, l := range strings.Split(e.Error(), "\n") {
+			slog.Error(l)
+		}


I'm not sure, I like the weird splitting on new line here.

After you've made the changes to []error slice, and using errors.Join. Can you show me what slog.Error("Error loading config", "error", e) looks like?

If it looks better by calling slog on each error, we should return a slice of errors instead of splitting on newline

feat: use go-playground/validator

d8e3d65

Closes Fesaa#5

jrschumacher commented Jan 5, 2025

View reviewed changes

Fesaa requested changes Jan 20, 2025

View reviewed changes

Conversation

jrschumacher commented Jan 5, 2025

Uh oh!

jrschumacher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fesaa commented Jan 6, 2025

Uh oh!

jrschumacher commented Jan 7, 2025

Uh oh!

Fesaa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants