Add support for Bcrypt algorithm version 2b

[docelic]

Fixes #11584

Thanks @naqvis for investigating.

[docelic]

@straight-shoota Thanks, made the suggested changes!

[straight-shoota]

Please use an array. Tuple is inappropriate for a potentially expandable list.

[docelic]

Will do. On a side note, I wrote this based on the following template from existing Crystal source:

src/http/common.cr:  SUPPORTED_VERSIONS = {"HTTP/1.0", "HTTP/1.1"}

[straight-shoota]

That's wrong. Let's not repeat that.

With tuples, the type of the constant would change as soon as a new supported version is added. That's unacceptable.

[Sija]

The type is Enumerable(String), and that should be enough. No need for concrete types here as there's no need to extend them.

[docelic]

I've updated the tests a while ago; unless you have other comments/requests I think this could be good to go.

[HertzDevil]

What happens if one calls Crypto::Bcrypt::Password::SUPPORTED_VERSIONS.clear? I don't think that should be allowed in a public API.

[straight-shoota]

That will break things until #10944 gets resolved. It's a generic problem for constants with mutable values. I don't think it makes sense to use Tuple here just to make the constant immutable.

[asterite]

To frame @HertzDevil's question a bit differently: is there any need to expose that constant in a public API? Why not make it at least private? And what's the harm of using a tuple, or even inlining the values?

[straight-shoota]

I think it makes sense to expose this information so users of the API can tell what is supported.

[asterite]

In that case, would it make sense to spend a few minutes trying to think of a public API that users can't easily break? What's the issue with using a tuple, or even a method that returns a copy of the array?

[oprypin]

@straight-shoota is thinking of the right API without being distracted by the language's temporary limitations.

[asterite]

If that's the case, then we shouldn't reject adding more exception types because it slows down the compiler, thinking that this will be solved too. I just don't think it's certain we'll ever implement immutable arrays, so we should design the API using what we have right now.

[HertzDevil]

There are ways to expose this information without using a public constant:

private SUPPORTED_VERSIONS = ["2", "2a", "2b"]

def self.supported_versions
  SUPPORTED_VERSIONS.dup
end

private SUPPORTED_VERSIONS = ["2", "2a", "2b"]

def self.version_supported?(ver : String)
  ver.in?(SUPPORTED_VERSIONS)
end

As for informing the users of the actual supported versions, that can always be done in the documentation of Crypto::Bcrypt::Password itself.

[Sija]

The easiest way atm would be to just use Tuple.

[docelic]

Seems like the only blocker here are the varying opinions/instructions given re. how to define SUPPORTED_VERSIONS.
Could you reach an agreement so that I can update the PR (if it turns out needed)?

[straight-shoota]

Yeah, sorry to see this obstructed by higher level questions.

I'd prefer simply using a constant with array value. That's the most intuitive thing. Everything else is just trying to work around language limitations. Yes, that array is mutable. So you must make sure not to do that or you risk unexpected behaviour. I'm pretty sure that's not a very relevant restriction though.

Also note that it can actually be useful to mutate the array. For example, you could reopen the class to add support for another version and then add that to SUPPORTED_VERSIONS.

[HertzDevil]

To err on the side of caution I still would prefer making SUPPORTED_VERSIONS private first. Nowhere in the fix of the original issue requires this constant to be public or even exist. It is easier to expose it later once that design decision is made, than to add it and change its representation afterwards.

[docelic]

I think we're good to go at this point! Thanks everyone!

[Blacksmoke16]

While we're already in here, should we throw in 2y? Is the version that is used in PHP based implementations.

[docelic]

@Blacksmoke16 sure, I'll just do it in a separate PR since I'd also like to add tests etc., so it's not a 1 line change.