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

Bug: Support cases where ignore_not_found is not provided #29

Merged
merged 2 commits into from
Jan 8, 2024

Conversation

GrantBirki
Copy link
Member

This pull request fixes a bug where entitlements would throw an exception and crash if ignore_not_found is not provided via a configuration. This is an issue with the way Ruby's Contracts Gem handles optional input params. If the ignore_not_found value is not provided, it will default to false but the way Contracts is setup it expects the input option to be a Boolean. This pull request fixes that with the Maybe keyword.

related: #28

@GrantBirki GrantBirki added the bug Something isn't working label Dec 22, 2023
@GrantBirki GrantBirki self-assigned this Dec 22, 2023
@jasonmacgowan
Copy link
Contributor

I know we use [FalseClass, TrueClass] elsewhere and default to false -- I wonder how we're enforcing the contract there.

@GrantBirki
Copy link
Member Author

GrantBirki commented Dec 28, 2023

I believe what is happening here (with my limited understanding of contracts), is that contracts is guarding what is passed into the initialize() method and not what it ends up being.

Here is an example:

# initialize the class but ensure all params have a value "passed in" even when they have defaults set (comments below)
Entitlements::Backend::GitHubTeam::Service.new(
  addr: nil, # value passed in
  org: "github",
  token: "secret",
  ou: "blah",
  ignore_not_found: false # value passed in
)

The code seen above will pass the checks that contracts puts in place because both addr and ignore_not_found have values assigned to them. These values that were assigned satisfy the guards that contracts has in place. However, if we look at the example below, this will raise an error as we are seeing today:

# initialize the class but use the defaults that the class defines for `addr` and `ignore_not_found`
Entitlements::Backend::GitHubTeam::Service.new(
  # addr: nil,
  org: "github",
  token: "secret",
  ou: "blah",
  # ignore_not_found: false
)

This will raise an exception on ignore_not_found but not on addr. This is because the addr variable is defined with the Maybe keyword in contracts This allows the value being passed in to "maybe" be a string, but it can also be nil (or unset). However, the ignore_not_found variable is missing this Maybe keyword. So we cannot omit it because if we do, its value is nil and nil is not an accepted value in the contracts definition. It can only be a Bool as defined prior to this pull request.

Hopefully that all makes sense and my understanding of contracts is correct here! 🙇


TL;DR: I don't think we can safely enforce the type on ignore_not_found while also allowing for it to be nil (unset) without updating everything everywhere to pass in the correct value without using defaults.

@GrantBirki GrantBirki merged commit 1e87a3c into main Jan 8, 2024
7 checks passed
@GrantBirki GrantBirki deleted the bug/ignore_not_found branch January 8, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants