Add support for bounded enumerables and stateless Procs in inclusion validation

[lavoiesl]

Add support for a few variations of inclusion constraints.

The motivating factor behind this is to be able to resolve to a list of classes that can only be resolved at runtime.

For example, this is not stable, it depends in which order the classes are loaded:

validates :model, inclusion: { in: ActiveRecord::Base.descendants.map(&:name) }

It needs a lambda:

validates :model, inclusion: { in: ->() { ActiveRecord::Base.descendants.map(&:name) } }

However, stateful lambda are prohibited, because they depend on the task instance:

validates :model, inclusion: { in: ->() { |task| task.some_method } }

---

While at it, this PR also adds support for bounded enumerables:

validates :timeout, inclusion: { in: (0..30).step(5) }

---

See the update test and comments for a full explanation.

[gmcgibbon]

Suggested change

	# - Procs and lambdas, but only if they take no arguments and return a value meeting the requirements above.
	# - Procs and lambdas, if they take no arguments and return a value that meets above criteria

[bony2023]

Amazing, thank you! 🎉
Few comments but not a blocker to merge

[bony2023]

nit, un-necessary as .take already returns an Array:

Suggested change

	items = in_option.take(MAX_INCLUSION_ITEMS + 1).to_a
	items = in_option.take(MAX_INCLUSION_ITEMS + 1)

[lavoiesl]

Take doesn't return an array if in_option is a lazy enumerator:

(1..).lazy.take(10)
=> #<Enumerator::Lazy: ...>

[etiennebarrie]

So this is now not showing a value that is accepted by the validation in the dropdown, making it impossible to choose it. E.g. in: (1..2000).to_a would show a dropdown with all the values, and you could choose 2000 if you wanted.

[lavoiesl]

Previously, 1..2000 would show a dropdown with 2000 values. I would argue that this isn't a great idea. It's a lot of value to generate.

Instead, this PR changes it such that it'll display a bare input text field, but the validation will still happen as normal.

I think it is perfectly acceptable.

[etiennebarrie]

Ah cool 👍

[bony2023]

non blocker but it would be good to also test the MAX_INCLUSION_ITEMS logic?

Suggested change

	validates_inclusion_of :text_integer_attr_enumerable, in: (100..120).step(5), allow_nil: true
	validates_inclusion_of :text_integer_attr_enumerable, in: (100..120).step(5), allow_nil: true
	validates_inclusion_of :text_integer_attr_large_enumerable, in: (1..1001).step(1), allow_nil: true

[lavoiesl]

Unbounded is already covered by text_integer_attr_unbounded_range. I think leaving the specifics undefined are reasonable. Here, 1000 is just an arbitrarily large but reasonable number

[etiennebarrie]

I don't think we want this. We're doing a best effort job of showing possible values when the inclusion validator is simple enough, but we shouldn't go and call random code, this is called in a view after all.

We have a way to extend the form by adding a maintenance_tasks/tasks/custom partial in the application. (It's not documented BTW sorry). I guess you could use this to conditionally add fields, hide the previous fields for an attribute, if you wanted to go down that route.

I think it's problematic that some validations can now break your views or make choices inaccessible. There should be some kind of opt-in at the very least.

Or maybe it's just not something that should be done in the engine. We do mention "Normally maintenance tasks are ephemeral, so they are used briefly and then deleted." in the README. The initial use case of wanting to list the application records feels like something that might be built in the application directly instead in some kind of admin interface, if it needs to access any and all models at any time.

https://github.com/Shopify/maintenance_tasks#should-i-use-maintenance-tasks

Otherwise one can always do Rails.application.eager_load!; ActiveRecord::Base.descendants in a console and stick the result in the validation of the task's param (since the task is temporary, it should be fine). Or just live with the fact that the form will not include a dropdown for the model, the target user for the engine being a developer, they should be able to deal with that situation.

[etiennebarrie]

So this is now not showing a value that is accepted by the validation in the dropdown, making it impossible to choose it. E.g. in: (1..2000).to_a would show a dropdown with all the values, and you could choose 2000 if you wanted.

[lavoiesl]

We're doing a best effort job of showing possible values when the inclusion validator is simple enough, but we shouldn't go and call random code, this is called in a view after all.

I don't understand this point. I wouldn't qualify this as random code, it's very explicitly the proc to generate the options and it should be fast and reliable, since it's meant to be used in validations.

---

We have a way to extend the form by adding a maintenance_tasks/tasks/custom partial in the application. (It's not documented BTW sorry). I guess you could use this to conditionally add fields, hide the previous fields for an attribute, if you wanted to go down that route.

This seems very convoluted. Using the partial is only really useful to add fields. How would it work to hide previous fields?

If we were to say that the standard way of customizing fields is through overriding, we should make it easy in the gem, by making a dedicated _field partial or introducing some override mechanism on the fields, like callbacks.

---

I think it's problematic that some validations can now break your views or make choices inaccessible.

I don't understand this point. What would break? Are you referring to when it has more than 1000 items? If so, I replied elsewhere.

There should be some kind of opt-in at the very least.

As a compromise, I would be ok with a MaintenanceTasks.max_inclusion_items config that defaults to 0 (disabled behaviour).

---

Or maybe it's just not something that should be done in the engine. We do mention "Normally maintenance tasks are ephemeral, so they are used briefly and then deleted." in the README. The initial use case of wanting to list the application records feels like something that might be built in the application directly instead in some kind of admin interface, if it needs to access any and all models at any time.

I would argue that just because the gem is not intended for permanent tasks isn't a reason to not support making that a viable option.

---

Otherwise one can always do Rails.application.eager_load!; ActiveRecord::Base.descendants in a console and stick the result in the validation of the task's param (since the task is temporary, it should be fine).

We have cases with ~100 classes to pick from. This quickly becomes noise, a maintenance nightmare, or an annoyance.

Or just live with the fact that the form will not include a dropdown for the model, the target user for the engine being a developer, they should be able to deal with that situation.

This increases the chances of errors.

---

👉🏼 I think my most important point is that ActiveModel is designed to support dynamic inclusion constraints, but this gem decided to opt-out of that support. I understand that stateful constraints can't be supported, but I don't see why we shouldn't support stateless lambdas.

[etiennebarrie]

As a compromise, I would be ok with a MaintenanceTasks.max_inclusion_items config that defaults to 0 (disabled behaviour).

I think that would be good.

I would argue that just because the gem is not intended for permanent tasks isn't a reason to not support making that a viable option.

Yes because there are a thousand no's for every yes. ☺️
First, we added params because this gem replaced enqueuing jobs from migrations, and people were passing arguments to the jobs. And it's useful to be able to test your task on a reduced amount of data without having to redeploy. But then we added dropdowns because if it's easy to do and it makes it easier on the user, why not. And here we are, adding another tweak to that simple thing and adding an arbitrary limit that needs configuration (in part to prevent regressions).
I hope you can see how supporting out-of-scope features tends to creep up.

I think my most important point is that ActiveModel is designed to support dynamic inclusion constraints, but this gem decided to opt-out of that support.

We support it just fine, we're just not making it easy for the user by providing the potential values as a dropdown.

I understand that stateful constraints can't be supported, but I don't see why we shouldn't support stateless lambdas.

So what are stateful constraints by the way? Lambdas that take the record? Also in: :some_method? We do have the object handy (although it's only initialized with its defaults obviously):

maintenance_tasks/app/helpers/maintenance_tasks/tasks_helper.rb

Line 148 in 502b213

inclusion_values = resolve_inclusion_value(form_builder.object.class, parameter_name)

I guess they raise more issues, but I wonder why we wouldn't support that as well at this point.

---

Do we need to deal with potential exceptions raised from the lambda? Before we were just getting a value from the task, now we're calling code, so I guess there's more potential for failure here.

---

We should also document the dropdown feature (and the scope of the support for views with this PR) in the README as well. I guess that could be an indicator that the feature is too complex if we need too many words to explain it. Before: "if you have an inclusion validator on your parameter, if the allowed values is a Array, it will be used to build a dropdown". Now we'll need to add some more words.

Also just realized that if there's an if: or unless: on a validation, we're not respecting that (and ignoring the validation altogether), and always forcing the values to be in the allowed list with the dropdown, when the validation might not apply.

Interested to hear what @adrianna-chang-shopify and @nvasilevski think. And anyone else actually.

[lavoiesl]

So what are stateful constraints by the way? Lambdas that take the record? Also in: :some_method? We do have the object handy (although it's only initialized with its defaults obviously).

I guess they raise more issues, but I wonder why we wouldn't support that as well at this point.

Yeah, validations are designed to run within the context of the object with the values at the right place, so adding that limitation seems fair to me.

If someone wants to call a method, they can do so in a proc:

validates :model, inclusion: { in: ->() { MyClass.allowed_model_values } }

---

Do we need to deal with potential exceptions raised from the lambda? Before we were just getting a value from the task, now we're calling code, so I guess there's more potential for failure here.

Indeed, but if the code is throwing an exception, I think it's perfectly reasonable to let it raise unhandled

---

We should also document the dropdown feature (and the scope of the support for views with this PR) in the README as well. I guess that could be an indicator that the feature is too complex if we need too many words to explain it. Before: "if you have an inclusion validator on your parameter, if the allowed values is a Array, it will be used to build a dropdown". Now we'll need to add some more words.

Yeah, I can do that once we settle 👍🏼

---

Also just realized that if there's an if: or unless: on a validation, we're not respecting that (and ignoring the validation altogether), and always forcing the values to be in the allowed list with the dropdown, when the validation might not apply.

That's a valid concern, but it's also already true today. I don't think this PR makes the status quo any different.