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

Support early-return of falsey values from ready/authorized #4585

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

eapache-opslevel
Copy link
Contributor

@eapache-opslevel eapache-opslevel commented Aug 16, 2023

If you write a resolver's authorized method with a return false, nil in it (to early-return a value of nil), it was treating this just as return false and raising, because the early-return value was not truthy. Instead explicitly check for an array value before splitting it in two.

I also did ready? while I was here since it had the same pattern.

The tests are "correct" but won't fail even without my "fix" because the test schema doesn't implement def unauthorized_object(unauthorized_error), and there are a bunch of tests already relying on it being not implemented. Not sure what the best approach is.
edit: I realized after posting that it was easier and more correct to test for a false/nil distinction which sidesteps the unauthorized_object issue

@rmosolgo @DougEdey

@eapache-opslevel eapache-opslevel changed the title Support early-return of nil value from ready/authorized Support early-return of falsey values from ready/authorized Aug 16, 2023
Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@rmosolgo rmosolgo added this to the 2.0.27 milestone Aug 18, 2023
@rmosolgo rmosolgo merged commit 279b0d6 into rmosolgo:master Aug 28, 2023
13 checks passed
@eapache-opslevel eapache-opslevel deleted the support-early-return-nil branch August 28, 2023 17:59
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.

2 participants