Spec for Array types leaking index into subsequent Hash validations

[lsimoneau]

This confirms the issue raised in this comment: #1131 (comment) by @Galkon

It was quite difficult to track down when this issue was occurring because it actually depends on a required Array parameter being defined before the Hash parameter in the params block. Somehow the existence of an index is leaking between the parameters.

@Galkon and I spent a bit of time trying to find the source of the issue, but decided to open a PR with the spec in case it turned out to be much easier for someone with a deeper familiarity with the codebase.

[dblock]

Thanks for the spec, lets get this fixed, cc: @towanda

[lsimoneau]

I'm looking into this, and I've made some progress, though it's actually quite tricky to figure out. Will let you know if I give up 😝

[lsimoneau]

Ok, so, there's some bad news. Basically with the way the code is currently structured it is impossible to get the correct array indices for parameter validations. This is because each ParamsScope is validated independently, so in the case of:

params do
  group :children, type: Array do
  requires :name
  group :parents, type: Array do
    requires :name
  end
end

When we get to validating the :parents scope, all we have is an array of all the parents hashes, without the context of which child they come from. It would take a pretty significant restructure of the way the validations and AttributesIterator work to make this possible.

Here are a few options:

1. Do the restructure.

Pros:

• possibly make the code a bit easier to follow. As is I feel like it's grown organically and it's very difficult to trace the flow of execution from validators to the iterator through scopes and back again.
• Enable the array indices feature to work

Cons:

• Big job, will take a long time. Possibly no one has the time to do it.
• May introduce regressions (test coverage seems quite good though, so this may not be a big issue)

2. Revert the array indices feature

This sucks, but as is the feature is completely broken: it leaks into non-array scopes, and always shows [0] for every index, which is IMO worse than showing nothing at all.

Pros:

• Easy.
• Makes everything behave consistently again.

Cons:

• (obvs.) does not give us array indices

3. Show array indices only for the innermost scope.

Pros:

• Easy. With the work I have on my local branch I could get this done in 10 minutes.
• Still gives some indication of array index. For non-nested arrays, it will work totally fine (so you'll still get customers[3][name] is missing

Cons:

• Still relies on mutating the state of the scope within the iterator, which is an aspect of the original feature code that makes me quite uncomfortable.
• Is confusing for nested arrays: customers[orders][0][address] is missing if there's more than one customer is arguably worse than just customers[orders][address] is missing. But if we assume that top-level arrays are more common than nested, maybe it's best to do the thing that helps the most people and call this inconsistency a known issue for now.

Conclusion

These options are not mutually exclusive. We could start by doing 2. or 3., but keep in mind to properly address iterating through scoped parameters in a way that preserves context at some point in the future (like whenever someone has the time and inclination).

Personally I feel like option 3 strikes a good balance by preserving what feels to me to be the most common use case of the feature with minimal effort. But maybe given that it doesn't work properly in all cases and relies on some "hacky" stuff, it would be best to remove it until it can be done correctly?

[dblock]

I vote for doing (2) and thus undoing a regression, then documenting very thoroughly what we want in an issue with failing specs so that we can fix it properly (which will take a brave person).

[lsimoneau]

Hi. First of all, sorry I abandoned this back in March. We were under some time pressure so we ended up reverting to 0.13 and rolling with that. We've recently had another look at this and it's still causing us problems, so I was wondering if your preference was still to yank the array indices?

If you're still happy to revert the bug (though at this point I realize it's been several releases so people may be depending on the broken behaviour), I'll go ahead and do a PR.

[grape-bot]

	1 Message
📖	We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Generated by 🚫 danger

[lsimoneau]

Sorry again. On closer inspection it looks like this has already been fixed by #1463. The specs in this PR now pass and I've rebased them against master.

If you think it's worthwhile keeping a test against the leakage you could keep them, otherwise I'm happy to close this off.

Thanks!

[dblock]

Lets keep the tests, merging.