Closed codeguru42 closed 2 years ago
Hi!
While I believe it's also the case that all my resolve methods should typically return a scope (and if none applies, return a none scope), I'm worried that returning nil
could be a valid use-case out there somewhere in the wild.
Making a nil
result much more visible would definitely aid debugging by surfacing the problem sooner, but it's also a breaking change and doesn't actually solve the underlying problem: the implementation of the scope is incomplete. I believe the way to deal with this is by having good coverage in your test suite.
All of that said, this decision is not absolute and it's possible that the other contributors have a different opinion, but with the information I have on hand right now it's a no.
@Burgestrand Thanks for responding to this issue. It's been more than a month since I posted this ticket and I see now on reading my OP that some details are missing. My controller tests were failing because of the ill-formed scope. An exception was thrown in pundit and I had to debug into the gem to find my error. If returning nil
is a valid use-case in the wild, then pundit shoudn't throw an exception.
I see that I need to add more code to show the problem. I'll do so in the near future when I can make some time.
@Burgestrand While attempting to create a more thorough code example, I found out the problem I'm seeing in my project isn't in pundit
directly. I'm also using jsonapi-resource
and jsonapi-authorization
. One of these is calling the scope and doesn't handle a nil
return value correctly. Thanks for your time looking at this. I'll file a bug report on one of those projects once I narrow it down further.
When
MyPolicy::Scope.result
returnsnil
, the calling function silently continues and blows up later at some point whenrecords
is used. It would be nice to have some sanity checks on the return value and output an informative message that the return value must not benil
.Example policy:
When
user.system_admin?
isfalse
,resolve
returnsnil
. I understand this is an error in my code and would like to know about it sooner rather than later. It took an hour of debugging through pundit code to find the mistake I made.