Add in ability to dynamically load in deny_listed domains

[scouttyg]

Working off of https://github.com/micke/valid_email2/issues/222, I wanted to add in the ability to load in deny-listed domains from other places outside a deny_listed_email_domains.yml file.

This new feature should support:

• Arrays
• Sets
• Procs
• ActiveRecord queries
• Boolean (TrueClass/FalseClass) support for backwards compatibility

Let me know what you think!

[scouttyg]

Small bump here @micke -- Let me know what you think!

[micke]

Thank you for your work and sorry for taking time to review this PR, I've been super busy with work and haven't had any time to spare for this.

I can appreciate that you've implemented a nice extendable API, but i'm sorry i don't see how the added complexity makes it worth it. In my opinion we could achieve the same result by allowing the user to supply a proc to allow_list and deny_list that takes the domain as a argument and returns true or false.

[scouttyg]

@micke You'll need to outline your potential solution more, and perhaps I can refactor this to support that, as I'm not quite sure what you mean.

E.g. if we want to support a dynamically generated domain list (a supplied array, a query from ActiveRecord, etc), somewhere we'll need to provide the list of dynamically generated domains, so we could potentially just do a proc like:

validates :email, 'valid_email_2/email': { deny_list: proc { |domain_list, domain| domain_list.include?(domain) } }

But this proc would take at the very least two arguments: the domain_list (which we can dynamically generate / supply), and a domain -- but since we can only get domain from ValidEmail2::Address.new(email).address.domain (unless we want to parse it out using separate logic ourself at validation time), the logic would have to look something like:

validates :email, 'valid_email_2/email': { deny_list: proc { |domain_list, domain| domain_list.include?(ValidEmail2::Address.new(self.email).address.domain) } }

Is this what you were thinking?

[micke]

@scouttyg I'm sorry if i'm unclear.

What I'm trying to say is that i appreciate that you've made a nice API to support multiple types of lists, but the way i see it we can achieve that by accepting a proc like this:

validates :email, 'valid_email_2/email': { deny_list: ->(domain){ ["google.com"].include?(domain) } }
validates :email, 'valid_email_2/email': { deny_list: ->(domain){ DeniedDomain.where(domain:).exists? } }

Or we could also simplify it further by supporting:

• proc that returns a boolean
• Enumerable (by using the #include? method)

That would enable something like this:

validates :email, 'valid_email_2/email': { deny_list: ["google.com"] } }
validates :email, 'valid_email_2/email': { deny_list: Set["google.com"] } }
validates :email, 'valid_email_2/email': { deny_list: ->(domain){ DeniedDomain.where(domain:).exists? } }

In both versions we would parse the domain and pass it to the proc as the only argument or call #include?(domain) on it if it's a Enumerable.

[scouttyg]

@scouttyg I'm sorry if i'm unclear.

What I'm trying to say is that i appreciate that you've made a nice API to support multiple types of lists, but the way i see it we can achieve that by accepting a proc like this:

validates :email, 'valid_email_2/email': { deny_list: ->(domain){ ["google.com"].include?(domain) } }
validates :email, 'valid_email_2/email': { deny_list: ->(domain){ DeniedDomain.where(domain:).exists? } }

Or we could also simplify it further by supporting:

• proc that returns a boolean
• Enumerable (by using the #include? method)

That would enable something like this:

validates :email, 'valid_email_2/email': { deny_list: ["google.com"] } }
validates :email, 'valid_email_2/email': { deny_list: Set["google.com"] } }
validates :email, 'valid_email_2/email': { deny_list: ->(domain){ DeniedDomain.where(domain:).exists? } }

In both versions we would parse the domain and pass it to the proc as the only argument or call #include?(domain) on it if it's a Enumerable.

@micke Hmmm, I think the main challenges would lie with the first two cases you list in the last example above, as they are not procs/lambdas:

validates :email, 'valid_email_2/email': { deny_list: ["google.com"] } }
validates :email, 'valid_email_2/email': { deny_list: Set["google.com"] } }

Because of this, if we are just relying on the "truthiness" of the object, later on we'll still need to inspect that object to determine how to deal with it (e.g. if it's an Enumerable, use include?, if it's a TrueClass used the supplied yml list in the gem, otherwise raise InvalidInputError or something like that), or else deny_listed? within ValidEmail2::Address won't know how to deal with it, right?

If we are just mainly supporting procs/lambdas, however, that would make more sense, as we could basically just do:

# In ValidEmail2::Address
def deny_listed?
  valid? && (domain_is_in?(ValidEmail2.blacklist) || deny_list_lambda.call(address.domain))
end

This would still require though that we pass probably a good amount of the options eventually down to address though, as we'd need to use them if they were potentialy lambdas/procs.

Is this more in line with what you are thinking?

[scouttyg]

Hey @micke, I've tried to simplify things down a bit, while still doing some validation on the procs / lambdas to make sure things are correct. Let me know what you think, and if this moves us in the right direction!

[phoet]

i think a straight forward simple and flexible API would be to use a method-name symbol { deny_list: :check_stuff_in_db } and pass the relevant info to that method on invocation.

[phoet]

This change will also break compatibility with address.deny_listed? as it does not have access to the provided inline-configuration. This will make debugging a lot more confusing. I think we should make ValidEmail2.deny_list configurable instead. This is a pretty common practice to have it in an initializer providing a custom list or a callable returning a list.