in-toto-golang icon indicating copy to clipboard operation
in-toto-golang copied to clipboard

Error handling

Open shibumi opened this issue 4 years ago • 4 comments

Please fill in the fields below to submit an issue or feature request. The more information that is provided, the better.

Description of issue or feature request: With Go 13, Go got better error handling with functions like errors.Is() or errors.As(). We might want to adapt our code, so developers can use this functions with our library.

Current behavior:

Right now we are excessively using fmt.Errorf("..") for error messages. Instead we should declare global Error types like in the Symlink implementation and return error types:

// ErrSymCycle signals a detected symlink cycle in our RecordArtifacts() function.
var ErrSymCycle = errors.New("symlink cycle detected")

This way we have error documentation and can use errors.Is(err, ErrSymCycle) for error checking. This is a huge benefit over string checking with if err.Error() == "..." { .. }, because with the latter we can run into a nil pointer dereference if Error() returns nil.

Expected behavior:

Use documented error types like:

// ErrSymCycle signals a detected symlink cycle in our RecordArtifacts() function.
var ErrSymCycle = errors.New("symlink cycle detected")

More about new Go 1.13 error handling can be found here: https://blog.golang.org/go1.13-errors

Feedback @SantiagoTorres @lukpueh ?

shibumi avatar Jul 09 '20 15:07 shibumi

This sounds like a better, more idomatic way to do error handling. It also sounds it'd be super helpful to improve testing.

Is there a way to do this gradually?

SantiagoTorres avatar Jul 09 '20 16:07 SantiagoTorres

@SantiagoTorres Yes, I just pushed 3 new error types in my latest in-toto-run commit. The error handling gets so much better with this.

We get basically from something like this:

        expectedError := "Could not find a public key PEM block"
	err = key.LoadRSAPublicKey("demo.layout.template")
	if err == nil || !strings.Contains(err.Error(), expectedError) {
		t.Errorf("LoadRSAPublicKey returned (%s), expected '%s' error", err,
			expectedError)
	}

to:

        err = key.LoadRSAPublicKey("demo.layout.template")
	if !errors.Is(err, ErrNoPEMBlock) {
            t.Errorf("LoadRSAPublicKey returned (%s), expected '%s' error", err,
			ErrNoPEMBlock.Error())
	}

shibumi avatar Jul 09 '20 17:07 shibumi

Cross-referencing with #5.

lukpueh avatar Jul 10 '20 07:07 lukpueh

@shibumi is this issue still up ?

sarthaksarthak9 avatar Apr 14 '24 11:04 sarthaksarthak9