Feature: Support validations on array items

[LukasKnuth]

This is my first stab at implementing #49

---

To this end, I have implemented a new validate_array_basic_field/5 function, which is called by validate_nested_fields/3 for any supported basic type.

It works much like the existing validate_array_field/4 function (which has been renamed), but it does some mangling of the changesets in order to have all errors be listed for the {:array, type} field.

Please also see the tests for the expected usage/output of this.

---

Questions/Ideas

• I'm not final on the name of the rules for the nested items. Perhaps simply calling it items: reads better in the DSL?
• I've done some mangling or error types to make them fit into Changesets myself, instead of relying on your logic of parsing nested changesets. Do you agree here?
• There are some Todos left in the code. We should discuss and get rid of those
• Some code cleanup is perhaps in order. Open to suggestions!

ToDo

• More tests for edge-case scenarios (which are?)
• Update documentation to add new feature

[LukasKnuth]

I've sat down again and noticed that allowing the usage of min, max and is for lists was almost free, so I enabled them for lists, too. More power for lists 💪

Maybe is for array is a bit unclear though. Should we rename it in this case?

[martinthenth]

Thanks for your PR. I'll review your code in the coming days. In general, good edge cases to test for are:

1. Valid params
2. Invalid params
3. Partially valid params
4. Required and optional fields
5. Integration with other validations (to check it doesn't overwrite changeset errors from other fields)

[martinthenth]

Another interesting test case is the array length. For example, a test where 2 items are passed but it is limited to 1. Will it return the parent error (max 1) or also the field errors. I think returning only the parent error is desirable, so that when the client solves the parent error the field errors show up.

[LukasKnuth]

I think returning only the parent error is desirable, so that when the client solves the parent error the field errors show up.

The current implementation will return both the array error and and errors to the items. This is because we don't stop validating once we find an error:

list: [
  "item should be at least 3 character(s)",
  "should have at most 1 item(s)"
]

I feel it makes sense to keep this behavior for two reasons:

1. It's consistent with Ecto. Changesets don't have a concept of "this change depends on the previous one to work", so you'll always get the errors "greedy".
2. It reduces the amount of trial-and-error users have to do to get to a valid result.

That said, I'm open to change this if you feel strongly about it :)

---

While writing new test cases, my implementation also has a bug first mentioned in #44. Since that isn't merged yet, I have gone ahead and fixed both instances I could find and added some specific tests to verify this behavior.

[martinthenth]

Looks very good! 👏🏻 I left some suggestions and a question

[LukasKnuth]

@martinthenth I have implemented most of your suggestions and have commented on the ones I'd challenge. Have a look :)

[LukasKnuth]

I'm having a hard time figuring out why these tests fail on CI.

I've tested an erlang/elixir version as close to what the CI job runs (elixir 1.13.4-opt-24 with erlang 24.3.4.11) as I can install on my computer. The tests all succeed.

@martinthenth can you run them locally?

[martinthenth]

I'm having a hard time figuring out why these tests fail on CI.

I've tested an erlang/elixir version as close to what the CI job runs (elixir 1.13.4-opt-24 with erlang 24.3.4.11) as I can install on my computer. The tests all succeed.

@martinthenth can you run them locally?

I tried them locally and they pass 🤔 So I tried updating from the most recent master branch, and it looks your branch was on an older ecto version (3.9.5 vs 3.10.1). Then there are indeed 3 tests that fail. It looks like it doesn't change the implementation much

Maybe it has to do with empty_values or force_changes? https://github.com/elixir-ecto/ecto/blob/master/CHANGELOG.md

[LukasKnuth]

@martinthenth You are correct, the changes to what cast/4 treats as "empty values" are causing the tests to fail.

As far as I understand, the old logic was that cast/4 would only consider empty strings "" as empty. If they where elements in an array, they where removed.
The validate_required/3 function however would go a step further and consider trimmed empty strings as empty.

With version 3.10.0, they changed this behavior and cast/4 now also (by default) considers input strings empty when they only contain white-space.

---

I have adopted the tests to the new behavior.

I have also rebased my fork branch onto your main, so that it merges again and has clean history.

[martinthenth]

@martinthenth You are correct, the changes to what cast/4 treats as "empty values" are causing the tests to fail.

As far as I understand, the old logic was that cast/4 would only consider empty strings "" as empty. If they where elements in an array, they where removed. The validate_required/3 function however would go a step further and consider trimmed empty strings as empty.

With version 3.10.0, they changed this behavior and cast/4 now also (by default) considers input strings empty when they only contain white-space.

I have adopted the tests to the new behavior.

I have also rebased my fork branch onto your main, so that it merges again and has clean history.

I recall there was an issue with required fields with empty values. 🤔 The default settings of Ecto exclude empty fields from the changeset, which is problematic when a field is intended to be changed to nil or "" after having been set to a value previously.

In general, if the client sends the parameters and the backend expects those fields then empty values should not be invalidated unless they are explicitly invalid (from the rules)

I have adopted the tests to the new behavior.

Great! I will take a look at the updated code soon 🙂

[martinthenth]

Great work! 💯 I can prepare a release that includes these changes soon 🙏🏻