Adding a better error message for download mode #1492

arschles · 2019-12-12T01:45:56Z

What is the problem I am trying to address?

There are a lot of configuration options and it's easy to forget something, misconfigure Athens, and get stuck when you make a configuration mistake.

DownloadMode is a good example because it cannot be empty, and when it is, we just say "it can't be empty" without any extra context.

See #1491 and #1336 for more discussion on DownloadMode

How is the fix applied?

I've added a nicer error message in cases where DownloadMode is empty, or it is set to an invalid value.

If this code is acceptable, I'd like to apply it to a few other config values (in a follow-up PR) that are easy to get wrong.

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.

ref #1491, since this checks existence and validates values for download mode, and also reports errors in a more human-friendly way.

cc/ @bluekeyes

@bluekeyes

When it's missing, we write a short message saying so, and when it's invalid, we do approximately the same thing. Athens has a lot of configuration options, so it's naturally easy to get them wrong or forget something. Nicer error messages should help. If this code is acceptable, I'd like to apply it to a few other config values (in a follow-up PR) that are easy to get wrong. ref gomods#1491, since this checks existence and validates values for download mode, and also reports errors in a more human-friendly way cc/ @bluekeyes

ghost

I love it, i wish all error messages were like this

marwan-at-work

pretty cool ✌️ -- open for comments :)

marwan-at-work · 2019-12-13T17:55:57Z

pkg/errors/config.go

+// and it knows how to print out the actual configuration name and other helpful information.
+type ConfigErrFn func(string) error
+
+type configErr struct {


do we need a new error type? I think we can leverage the existing one. And instead of making a new error type, we can just make a new error Kind like ConfigErrorKind or something.

marwan-at-work · 2019-12-13T17:58:03Z

pkg/errors/config.go

+// ConfigErrFn is a function that can create a new configuration error. You pass it a
+// message specific to the error you found when you were validating configuration,
+// and it knows how to print out the actual configuration name and other helpful information.
+type ConfigErrFn func(string) error


Same here since this is specifically a ConfigErrFn, we can probably just put it in the Config package itself.

@marwan-at-work I wanted to but can't because the config package imports mode which needs all the config error stuff. Do you think it should be somewhere besides there?

Sorry, I meant the mode package...which makes me realize ConfigErrFn makes no sense here, since it's not a Config package error, it's a mode error :)

So that said, all of my suggestion are meant for keeping this functionality local to wherever the error is happening, in this case inside mode.go

Thanks @arschles ✌️

marwan-at-work · 2019-12-13T17:59:19Z

pkg/errors/config.go

+//
+//	downloadModeFn := ConfigError("DownloadMode (ATHENS_DOWNLOAD_MODE)")
+//	err := doThingsWithDownloadMode(configStruct, downloadModeFn)
+func ConfigError(field string, url string) ConfigErrFn {


This is pretty cool and functional :) but we can probably move this into the config package, because nobody else is leveraging it and so no need to increase the API surface of the errors package, we can just use the already exposed functionality.

marwan-at-work · 2019-12-13T17:59:51Z

pkg/errors/config.go

+		if url != "" {
+			slc = append(slc, fmt.Sprintf("See %s for more information.", url))
+		}
+		return &configErr{str: strings.Join(slc, "\n")}


I'd just return errors.E(op, strings.Join(slc, "\n"), whateverKindIfNeedBe)

marwan-at-work · 2019-12-13T18:01:15Z

pkg/errors/kinds.go

@@ -4,3 +4,9 @@ package errors
 func IsNotFoundErr(err error) bool {
 	return Kind(err) == KindNotFound
 }
+
+// IsConfigErr returns true if the given err is a configuration error
+func IsConfigErr(err error) bool {


we probably don't need this if we just do errors.Is(err, errors.ConfigErrorKind) or whatever kind name we'll eventually pick.

But furthermore, I don't think we need a new config error kind, because we don't programmatically do anything about it anyway, we just log the error and exit.

marwan-at-work · 2019-12-13T18:02:16Z

cmd/proxy/actions/app.go

@@ -132,6 +133,9 @@ func App(conf *config.Config) (http.Handler, error) {
 		lggr,
 		conf,
 	); err != nil {
+		if errors.IsConfigErr(err) {


so all that we're doing here is choosing not to wrap the error with "error adding proxy routes (message)". I think, instead of doing that, we can just pick a better wrapper-message and then we don't need this exposed API at all, we can just say: "error initializing Athens: %s" below.

marwan-at-work · 2019-12-13T18:04:21Z

cmd/proxy/actions/app_proxy.go

@@ -98,7 +99,10 @@ func addProxyRoutes(
 	}
 	st := stash.New(mf, s, stash.WithPool(c.GoGetWorkers), withSingleFlight)

-	df, err := mode.NewFile(c.DownloadMode, c.DownloadURL)
+	df, err := mode.NewFile(c.DownloadMode, c.DownloadURL, errors.ConfigError(


though this functional pattern is pretty awesome, it's a bit of an overkill just to add 2 lines of text to an error:

We never have to customize this line

We can just add those two lines into the errors themselves inside mod.NewFIle

We can still keep it functional by keeping the mode.NewFile signature exactly the same, but having this functional pattern internal and private to the package itself so that you don't have to manually do it a few times

@marwan-at-work

cc/ @marwan-at-work, I think I've addressed almost all of your comments here, but let me know if this overall design is what you had in mind. Tests are still failing, I'm fixing those shortly

arschles · 2019-12-13T23:22:19Z

@marwan-at-work I made some changes to (hopefully) address your comments. let me know what you think ✌️

marwan-at-work

last few nits and good to go 👍

marwan-at-work · 2019-12-20T00:05:19Z

cmd/proxy/main.go

@@ -32,6 +33,9 @@ func main() {
 	if err != nil {
 		log.Fatalf("could not load config file: %v", err)
 	}
+	if err := conf.Validate(errors.Op("main")); err != nil {


We already have a validateConfig function here: https://github.com/gomods/athens/blob/master/pkg/config/config.go#L238

It gets automatically called when you call config.Load

So this is a bit confusing :)

In the same fashion, the mode.Validate function in this case is getting called twice, once here and once in mode.NewFile.

I think we can just:

remove this line here

remove config.Validate function

make mode.Validate private

@marwan-at-work I added a Validate function on config structs so that we can nest validations. I'll try to move all of the logic into validate struct tags though

marwan-at-work · 2019-12-20T00:07:24Z

pkg/download/mode/mode.go

 )

+// Validate ensures that m is a valid mode. If this function returns nil, you are
+// guaranteed that m is valid
+func (m Mode) Validate(op errors.Op) error {


Instead of it taking an op, just make a mode.Validate operation under this line and wrap the error with it. This way, people can get the entire call stack by the time the server fails to boot up.

marwan-at-work · 2019-12-20T00:11:49Z

pkg/download/mode/mode.go

 // NewFile takes a mode and returns a DownloadFile.
 // Mode can be one of the constants declared above
 // or a custom HCL file. To pass a custom HCL file,
 // you can either point to a file path by passing
 // file:/path/to/file OR custom:<base64-encoded-hcl>
 // directly.
 func NewFile(m Mode, downloadURL string) (*DownloadFile, error) {
+	const downloadModeURL = "https://docs.gomods.io/configuration/download/"


I would make this a package level constant (private) as it is being used 3 more times in the pr

marwan-at-work · 2019-12-20T00:13:32Z

pkg/download/mode/mode_test.go

@@ -127,12 +129,12 @@ func TestNewFile_err(t *testing.T) {
 		{
 			name:     "empty mode",
 			mode:     "",
-			expected: downloadModeErr,
+			expected: Mode("").Validate(op).Error(),


hmm, we probably shouldn't do string matching here: either

change expected string to hasErr bool

or, change expected string to expected errors.Kind

marwan-at-work · 2019-12-20T00:16:13Z

pkg/errors/errors.go

@@ -18,6 +18,7 @@ const (
 	KindRateLimit      = http.StatusTooManyRequests
 	KindNotImplemented = http.StatusNotImplemented
 	KindRedirect       = http.StatusMovedPermanently
+	KindConfigError    = -1


I believe the HTTP protocol gives you some room for custom codes
In this case this is a client (user) error so maybe we can give it a code between 451 and 499: https://golang.org/pkg/net/http/#pkg-constants

It doesn't really matter but just being a bit more conventional :)

422 / http.StatusUnprocessableEntity is a common response when a client provides syntactically valid but semantically invalid objects to an API and could be appropriate here, if you want to stick with valid HTTP codes.

@marwan-at-work fair enough! I'll take @bluekeyes's suggestion and go with 422, just so we can use a constant.

and setting a default

Also removing the op from DownloadFile.Validate

Just checking to see if there's an error. Also adding tests for valid modes

arschles · 2020-02-21T18:15:04Z

@marwan-at-work I made a few fixes in here. Regarding #1492 (comment), I put all the calls to Validate inside validateConfig. I think that the validate struct tags and custom validators are useful in some cases, but the nested Validate functions seem to be a lot more understandable than validator. Are you cool with leaving this pattern in?

Looks like validate tags don't have a prefix checker. This seems easier than building a custom validator

marwan-at-work

nice work so far, there's a potential regression for some people in this PR who use full addresses and a few comments/nits.

✌️

marwan-at-work · 2020-02-22T16:35:59Z

pkg/config/config.go

+// Validate implements Validator
+func (c *Config) Validate() error {
+	const op errors.Op = "Config.Validate"
+	if !strings.HasPrefix(c.Port, ":") {


I think this will break people who are binding 0.0.0.0:<port> -- plus our code in ensurePortFormat will always add the ":", so I think it's best we remove this validation and if the PORT configuration that the user put is wrong, Go will tell them when the server doesn't boot up.

marwan-at-work · 2020-02-22T16:59:42Z

pkg/config/validator.go

+// Validator can validate a config struct. If you implement this,
+// validate all of the configuration in your struct. It will
+// automatically be called when Athens starts
+type Validator interface {


Definitely correct me if I'm wrong but:

Do we need a validator interface? I don't see any code actually dealing with the "interface" aspect of things. All the Validate functions are being called explicitly. AKA nothing is being abstracted here :)

I think we can make a type Validator func() error -- but even then it might be more for documentation than an actual use case.

Also the documentation here I think might be a little misleading:

If you implement this, validate all of the configuration in your struct. It will automatically be called when Athens starts

To the same point above, we need to explicitly call "Validate" and so there's nothing automatic happening right?

You're right, I got ahead of myself here. My intention was to have the config.Validate function look for Validators in each of its fields, calling validate on each of them. Right now, Validate is called as necessary on each field that implements it, in various other parts of the code.

I'll leave Validator out of this PR

marwan-at-work · 2020-02-22T17:02:29Z

pkg/errors/config.go

+//
+// Generally these kinds of errors should make Athens crash because it was configured
+// improperly
+func Config(op Op, field, helpText, url string) error {


will this ever be called from outside the config package? why not just define it there and define it locally to minimize the API surface of this package?

Nope. I put it here just because I think it makes more sense to group a configuration error under the errors package. I'm willing to be convinced otherwise 😄

pkg/errors/config.go

pkg/download/mode/mode.go

marwan-at-work · 2020-02-22T17:13:27Z

pkg/download/mode/mode.go

@@ -53,8 +85,8 @@ type DownloadPath struct {
 func NewFile(m Mode, downloadURL string) (*DownloadFile, error) {
 	const op errors.Op = "downloadMode.NewFile"

-	if m == "" {
-		return nil, errors.E(op, downloadModeErr)
+	if err := m.Validate(); err != nil {


I think this error is now less readable, because before it used to say download mode is not set but now it will say isn't a valid value (notice the space in the beginning because there's nothing)

I think in the Sprintf of the validate function we should pass a %q so it would say "" isn't a valid value or we should keep the old error here for better messaging.

How about if we pass %q as you said, and also prefix the error with "Download mode ..."?
The result would then be Download mode "" isn't a valid value

That sounds great

marwan-at-work · 2020-02-22T17:16:42Z

pkg/download/mode/mode.go

-	default:
-		return nil, errors.E(op, errors.KindBadRequest, fmt.Sprintf(invalidModeErr, m))
+	retFile := &DownloadFile{Mode: m, DownloadURL: downloadURL}
+	if err := retFile.Validate(); err != nil {


Correct me if I'm wrong but:

The old code that was removed here no longer works because in the old code we are checking the top level Mode, but in this validate function only the internal d.Paths[n].Mode is checked.

However, since top level Mode is now already checked above, then this Validate function is not necessary because retFile has no Paths to begin with.

@marwan-at-work you're right. I left a few things out of this Validate and some tangential functions

DownloadFile.Validate needs to check DownloadURL for well-formedness

^^ also needs to call DownloadPath.Validate for each d.Paths[n] - that does more than just validate its mode

Mode.Validate does not check for custom: and file: values

I've made changes to address all this

I've fixed all of that

…msgs

arschles · 2020-03-10T23:56:41Z

@marwan-at-work I think I changed code or responded to all your comments. Let me know what you think or if I missed something! 💚

marwan-at-work · 2020-03-11T04:06:24Z

@arschles thank you for fixing all the comments 💯 💯 will take a look as soon as possible 🙏

arschles · 2020-03-12T22:32:31Z

Take your time @marwan-at-work! This PR is a "slow burn" 😄

arschles requested a review from a team as a code owner December 12, 2019 01:45

arschles changed the title ~~Adding a better error message for download mode~~ [WIP] Adding a better error message for download mode Dec 12, 2019

arschles mentioned this pull request Dec 12, 2019

Deprecate the GlobalEndpoint #1493

Open

Fixing test

fa2631c

ghost approved these changes Dec 13, 2019

View reviewed changes

marwan-at-work requested changes Dec 13, 2019

View reviewed changes

arschles added 2 commits December 13, 2019 13:11

Making some progress toward removing the specific config error

834eba4

cc/ @marwan-at-work, I think I've addressed almost all of your comments here, but let me know if this overall design is what you had in mind. Tests are still failing, I'm fixing those shortly

Fixing last test compile error

f45a03b

arschles mentioned this pull request Dec 19, 2019

Audit validation tags for configuration properties #1491

Open

Merge branch 'master' into config-msgs

4715103

marwan-at-work requested changes Dec 20, 2019

View reviewed changes

arschles mentioned this pull request Dec 20, 2019

Add an endpoint that will output Athens configuration #1503

Open

Merge branch 'master' into config-msgs

e928a0f

arschles mentioned this pull request Feb 14, 2020

Consider using defaults for missing properties in config files #1490

Open

arschles and others added 8 commits February 13, 2020 16:39

Moving downloadModeURL to package-global

32e7c28

using a valid HTTP error code instead of -1

a846557

removing the config.Validate function

25b5860

making the DownloadMode field required

36b6d62

and setting a default

validating that the port starts with ':'

2c6b90b

Merge branch 'master' into config-msgs

5b85142

Not passing an op to Validate functions

65662f5

Also removing the op from DownloadFile.Validate

Not checking for error strings in mode test

e962a4d

Just checking to see if there's an error. Also adding tests for valid modes

arschles added 2 commits February 21, 2020 10:15

more doc for Validator

1b29dcf

removing struct tag for config port validation

d6a7cbd

Looks like validate tags don't have a prefix checker. This seems easier than building a custom validator

marwan-at-work requested changes Feb 22, 2020

View reviewed changes

arschles and others added 2 commits March 10, 2020 15:23

Merge branch 'master' into config-msgs

de71cb7

Removing the unnecessary validator interface

cc70e12

arschles added 7 commits March 10, 2020 15:39

Joining error strings with newline+tabs instead of just a space

a5eda16

s/retFile/df

198d574

validating each download path in DownloadFile.Validate

ee8f4d1

Checking for custom or file modes

2cc9f84

Checking top-level DownloadURL

9695c36

Merge branch 'config-msgs' of github.com:arschles/athens into config-…

726b77c

…msgs

validating the download path mode only in DownloadPath.Validate

4320101

arschles changed the title ~~[WIP] Adding a better error message for download mode~~ Adding a better error message for download mode Mar 10, 2020

arschles changed the base branch from master to main June 15, 2020 19:21

Adding a better error message for download mode #1492

Are you sure you want to change the base?

Adding a better error message for download mode #1492

Conversation

arschles commented Dec 12, 2019 • edited

ghost left a comment

Choose a reason for hiding this comment

marwan-at-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Dec 13, 2019

marwan-at-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes Dec 20, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Feb 21, 2020

marwan-at-work left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Mar 10, 2020

marwan-at-work commented Mar 11, 2020

arschles commented Mar 12, 2020

arschles commented Dec 12, 2019 •

edited

bluekeyes Dec 20, 2019 •

edited