Consider removing panics

[sudo-bmitch]

Hi maintainers! I think there's a universal desire to not panic in a critical package, but how to get there is less easy. Considering how widely used this is, I thought it might be useful to create a proof of concept for what a v2 could look like. That implementation is up at:

https://github.com/sudo-bmitch/oci-digest

The biggest downside is the type changes and methods with errors in the returns would break existing users (hence the v2). I'm happy to see any of the changes in my package upsteamed and can assist in any PRs you'd like.

[thaJeztah]

Yeah, the panics have been a bit of a pain, and I've also been hit by the implicit nature of registering the algorithms (moby/moby#42755 (comment))

go-digest/digest.go

Lines 121 to 148 in 1e56c6d

	// Algorithm returns the algorithm portion of the digest. It panics if
	// the underlying digest is not in a valid format.
	func (d Digest) Algorithm() Algorithm {
	alg, _, ok := strings.Cut(string(d), ":")
	if !ok {
	panic(fmt.Sprintf("no ':' separator in digest %q", d))
	}
	return Algorithm(alg)
	}
	// Verifier returns a writer object that can be used to verify a stream of
	// content against the digest. If the digest is invalid, the method will panic.
	func (d Digest) Verifier() Verifier {
	return hashVerifier{
	hash: d.Algorithm().Hash(),
	digest: d,
	}
	}
	// Encoded returns the encoded portion of the digest. It panics if the
	// underlying digest is not in a valid format.
	func (d Digest) Encoded() string {
	_, encoded, ok := strings.Cut(string(d), ":")
	if !ok {
	panic(fmt.Sprintf("no ':' separator in digest %q", d))
	}
	return encoded
	}

The changes I made in this PR (not yet in a release) should alleviate some of that pain, as they would at least register the default algorithms;

• make sure the standard algorithms are registered #64

The trouble with the current methods is not only that it can panic, but also that it's not immediately advertised "on the tin". In an ideal situation, we would have had more idiomatic, distinct functions for this, similar to how (e.g.) Go stdlib rexexp has;

• regexp.Compile with an error-return
• regexp.MustCompile no error-return, can panic.

Changing the existing signatures is a breaking change though, and while we could do a major version update, we likely still need to share the same types to be used, given that types like Digest may have found their way in many code-bases.

possibly making v1 a dependency of v2, and aliasing the v2.Digest to v1.Digest could work (haven't tried if that's an option).

Alternatively;

• keep the current methods (like Algorithm()), admit that they're poorly named, and come with a name for new methods (TryAlgorithm() (Algorithm, err0r))
• add a v2 package (or some better name) that is still part of the v1 module (and same types), but provides the better API (MustAlgorithm() and Algorithm())

[thaJeztah]

FWIW; it would probably still be good to do a new (v1.1.0?) release in either case, assuming that the changes in master are all OK; v1.0.0...master

[sudo-bmitch]

I've also been hit by the implicit nature of registering the algorithms

I chose the register the func() hash.Hash function directly instead of the crypto.Hash interface so I could avoid worrying about whether the algorithm was available. It also felt cleaner to not have an _ import.

we likely still need to share the same types to be used, given that types like Digest may have found their way in many code-bases

Indeed. I had opted to change the type to allow me to vet the value once when setting it, and know any non-zero value is valid on all future methods. Without that, I think extra validation checks would been needed on the other methods. One potential downside of validating on initialization is unmarshaling json and text can now fail, but I couldn't think of many scenarios where I would want to unmarshal in invalid digest and fail later. Perhaps if a tool was passing through the digests as a string without using the digests, but in those scenarios, they could just use a string.

If you went the v2 route I believe a v1 and v2 type would be a different type regardless of the underlying definition. So all downstream upgrades would be breaking changes if they exposed those types. Something else that wouldn't work is a type cast from a string to a Digest. I also had to add IsZero and Equal methods to handle scenarios where I was previously doing string comparisons.

[cyphar]

I think we should still have Must versions of some of these functions -- most programs register algorithms in init so the usual concerns about panics causing issues for long-running processes don't apply to most users. I also somewhat like that digests are just strings with additional properties, but I guess that removing that is necessary in order to avoid making error returns from the methods tedious.

I'm also quite worried about the impact of a v2 on the wider ecosystem. I think we probably want to have a way to convert between v1 and v2 Digests (at least) in the v2 version. I'm not sure if go mod can handle v2 importing v1 though. This would need a lot of testing and I have some suspicions that this will be incredibly difficult to transition effectively.