Skip to content
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.

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

Implemented support for checkable errors #131

Merged
merged 9 commits into from
May 12, 2024
Merged

Conversation

PatrLind
Copy link
Contributor

This is a possible fix for issue #86.

I have added two custom error types that can be used to check if an error is coming from the uuid package.
Not sure if the package maintainers agree with my implementation, but feel free to tell me if it is suitable or not.
Also not sure if there needs to be two errors types. Perhaps one is good enough?

No error messages should be different from before. err.Error() should return the same string.
It should now also be possible to check for uuid errors by using the errors.Is() and errors.As() functions.

I added a test function to verify that the implementation is OK.

@dylan-bourque
Copy link
Member

Personally, I'm not a fan of exporting custom error types. What's the use case for needing to determine whether or not an error was generated by this package?

As it's written today, any error returned by any function/method is, by definition, generated by this package since all of them are constructed directly. There's no need for anyone to use errors.Is() or errors.As() to check that.

@PatrLind
Copy link
Contributor Author

Well... I had a need for it and saw that no one had done anything to fix the bug that was reported 4 years ago.
My use case is that I have an error handler for gRPC that checks for some types of errors that can be returned. One of the errors I check is if the UUID could be parsed correctly. If it was not parsed correctly I send can send back the error to the user in clear text. Otherwise I just send a generic Internal Server Error. So this is a way to filter out internal errors that should not be sent back to the user while letting some specific errors thru. And since the UUID comes from the caller this is important information to know (that the parsing failed).
Right now I need to check if the error string contains "uuid: in" or "uuid: UUID must". This is a bit brittle, and it seems to me that looking for strings in an error message in order to determine if they are from the goofr/uuid package is not really a good practice.

In my experience most packages (at least the ones I use) export errors I can check against with errors.Is().

I'm not exactly sure what you mean by "any error returned by any function/method is, by definition, generated by this package since all of them are constructed directly"?

@dylan-bourque
Copy link
Member

I see. Your code is not parsing/decoding the UUID values directly but you want to be able to detect that the error returned by gRPC came from gofrs/uuid, right?

If that's the case, I would personally define a couple of sentinel errors and refactor those fmt.Errorf() calls to to return them. I've kind of decided that I don't like to include "extra" information (like the values of passed-in parameters) in error messages, but not everyone agrees with that.

// alias for string so that we can declare "constant" errors
// see https://dave.cheney.net/2016/04/07/constant-errors
type Error string
func (e Error) Error() string { return string(e) }

const (
    ErrInvalidDataLength = Error("uuid: ....")
    ErrInvalidFormat.    = Error("uuid: incorrect UUID format in string")
)
...

return ErrInvalidFormat
-- OR --
return fmt.Errorf("%w: %q", ErrInvalidFormat, s)

Consumers could still use errors.Is(err, uuid.ErrInvalidFormat) to determine which kind of error happened without introducing the extra complexity of a local error type "hierarchy".

@PatrLind
Copy link
Contributor Author

Sure, declaring pre-allocated errors like in your example would have been great!
But since I'm pretty sure people have come to depend on the exact error strings in may ways I don't know about, so the current error messages cannot change (the string value returned from Error()). And since a lot of the error messages are including details on how the parsing failed, so those cannot be pre-allocated like that (right?).

I can refactor my code a bit and try to simplify it a bit.

@dylan-bourque
Copy link
Member

Thinking out loud ... we could probably use the "constant" sentinel errors and keep the current error strings with careful applications of fmt.Errorf()

Assuming ErrInvalidFormat is defined the way I mentioned before, the string returned by .Error() should be the same for the 2 expressions below

fmt.Errorf("uuid: incorrect UUID format in string %q", s)
fmt.Errorf("%w %q", ErrInvalidFormat, s)

@PatrLind PatrLind changed the title Implemented ErrUUID and ErrUUIDInvalidFormat error types Implemented support for checkable errors Apr 8, 2024
error_test.go Outdated Show resolved Hide resolved
error.go Show resolved Hide resolved
error.go Outdated Show resolved Hide resolved
error_test.go Outdated Show resolved Hide resolved
error_test.go Outdated Show resolved Hide resolved
Copy link
Member

@dylan-bourque dylan-bourque left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the submission and for working with me to resolve the feedback.

@cameracker
Copy link
Collaborator

Hi @PatrLind , I'm finally getting some time to take a look at the open PRs. I decided to merge in the updated RFC one because it seemed like it'd have the least disruption if I merged it in first. Apologies if this generates conflicts. Also, codecov hasnt been running for a bit so the next time a change is made to this review it may come up with some missing coverage. I'll only ask for test coverage on areas you introduced.

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (22c52c2) to head (ec01242).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #131   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         5    +1     
  Lines          513       447   -66     
=========================================
- Hits           513       447   -66     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cameracker cameracker merged commit 4a2de11 into gofrs:master May 12, 2024
7 checks passed
nono referenced this pull request in cozy/cozy-stack May 13, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/gofrs/uuid/v5](https://togithub.com/gofrs/uuid) | `v5.1.0`
-> `v5.2.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fgofrs%2fuuid%2fv5/v5.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fgofrs%2fuuid%2fv5/v5.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fgofrs%2fuuid%2fv5/v5.1.0/v5.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fgofrs%2fuuid%2fv5/v5.1.0/v5.2.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gofrs/uuid (github.com/gofrs/uuid/v5)</summary>

### [`v5.2.0`](https://togithub.com/gofrs/uuid/releases/tag/v5.2.0)

[Compare
Source](https://togithub.com/gofrs/uuid/compare/v5.1.0...v5.2.0)

This minor release updates the UUID v6 and v7 features of package to be
complaint to draft RFC-9562, which replaces the previous family of uuid
drafts. Additionally, it adds more specific error types, and tunes up
Github Actions safety practices

#### What's Changed

- Update to RFC 9562 by
[@&#8203;kohenkatz](https://togithub.com/kohenkatz) in
[https://github.com/gofrs/uuid/pull/117](https://togithub.com/gofrs/uuid/pull/117)
- \[StepSecurity] Apply security best practices by
[@&#8203;step-security-bot](https://togithub.com/step-security-bot) in
[https://github.com/gofrs/uuid/pull/135](https://togithub.com/gofrs/uuid/pull/135)
- Bump actions/checkout from 2.7.0 to 4.1.5 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gofrs/uuid/pull/137](https://togithub.com/gofrs/uuid/pull/137)
- Bump ossf/scorecard-action from 2.0.6 to 2.3.3 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gofrs/uuid/pull/136](https://togithub.com/gofrs/uuid/pull/136)
- Bump actions/dependency-review-action from 2.5.1 to 4.3.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gofrs/uuid/pull/138](https://togithub.com/gofrs/uuid/pull/138)
- Bump actions/upload-artifact from 3.1.3 to 4.3.3 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gofrs/uuid/pull/140](https://togithub.com/gofrs/uuid/pull/140)
- Bump codecov/codecov-action from 2.1.0 to 4.3.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gofrs/uuid/pull/139](https://togithub.com/gofrs/uuid/pull/139)
- Implemented support for checkable errors by
[@&#8203;PatrLind](https://togithub.com/PatrLind) in
[https://github.com/gofrs/uuid/pull/131](https://togithub.com/gofrs/uuid/pull/131)

#### New Contributors

- [@&#8203;kohenkatz](https://togithub.com/kohenkatz) made their first
contribution in
[https://github.com/gofrs/uuid/pull/117](https://togithub.com/gofrs/uuid/pull/117)
- [@&#8203;step-security-bot](https://togithub.com/step-security-bot)
made their first contribution in
[https://github.com/gofrs/uuid/pull/135](https://togithub.com/gofrs/uuid/pull/135)
- [@&#8203;dependabot](https://togithub.com/dependabot) made their first
contribution in
[https://github.com/gofrs/uuid/pull/137](https://togithub.com/gofrs/uuid/pull/137)
- [@&#8203;PatrLind](https://togithub.com/PatrLind) made their first
contribution in
[https://github.com/gofrs/uuid/pull/131](https://togithub.com/gofrs/uuid/pull/131)

**Full Changelog**:
gofrs/uuid@v5.1.0...v5.2.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone
Europe/Paris, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/cozy/cozy-stack).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNTEuMiIsInVwZGF0ZWRJblZlciI6IjM3LjM1MS4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbXX0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants