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

Raising Clear::SQL::RecordNotFoundError error instead of NilAssertionError on find!(id) #94

Closed
jackturnbull opened this issue Mar 17, 2019 · 1 comment · Fixed by #96

Comments

@jackturnbull
Copy link
Contributor

jackturnbull commented Mar 17, 2019

More of a discussion, as there are multiple ways to handle this but I quite like the way ActiveRecord rasies a RecordNotFound error message when performing find operations.

It's somewhat minor (and up for debate if this is the correct approach) but throwing a Clear::SQL namespaced error would allow me to add a generic handler for Clear::SQL::RecordNotFoundError in my parent API controllers and mean that I could skip guarding each of my update find methods with a method that checks for the presence of the model.

The main motivation for a more specific class is that catching NilAssertionError is too generic of an error to reliably assert behaviour upon.

def self.find!(x)
find(x).not_nil!
end

Could become something like:

def find!
  result = find(x)
  raise Clear::SQL::RecordNotFoundError.new unless result
  result
end

If this is something you agree with and want to hand over to me, feel free to let me know and I'll raise a PR 😄

Ref: [1]

@anykeyh
Copy link
Owner

anykeyh commented Mar 18, 2019

I think all the errors should be mapped to a specific type as much as possible.
So I'm more than ok for the PR ! 😄

jackturnbull added a commit to jackturnbull/clear that referenced this issue Mar 18, 2019
fixes anykeyh#94

Opt to use a Clear provided RecordNotFoundError class when performing a dangerous find!. This has the advantage of allowing the consumer to use error paths to control handling of unresolved records.

Same approach as https://api.rubyonrails.org/classes/ActiveRecord/RecordNotFound.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants