Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MinItems and MaxItems validations #22530

Merged
merged 2 commits into from
Aug 20, 2019
Merged

MinItems and MaxItems validations #22530

merged 2 commits into from
Aug 20, 2019

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Aug 20, 2019

This PR is reduced in scope from #22484, only removing the validation that we cannot reliably handle. The dedicated Validated method contained a lot of duplicated logic, and added minimal value in the validation process (types and structure should match at this point), only really being able to catch MaxItems validations at the last moment.

We can evaluate if Validate has a use in this are later on, or if full validation should simply be under the purview of the value recipient which will have other constraint checks anyway. Making better use of InternalValidate can also be decided separately from this change.

--

Due to both the nature of dynamic blocks, and the need for resources to
sometimes communicate incomplete values, we cannot validate MinItems and
MaxItems in CoerceValue. While most of the checks were removed already, the
NestingSingle, NestingGroup blocks were still being checked.

During decode, we can only validate MinItems >= 1 (equiv to "Required"),
as dynamic blocks each only decode as a single block. MaxItems
cannot be validated at all, also because of dynamic blocks, which may
have any number of blocks in the config.

Fixes #22414

Due to both the nature of dynamic blocks, and the need for resources to
sometimes communicate incomplete values, we cannot validate MinItems and
MaxItems in CoerceValue.
We can only validate MinItems >= 1 (equiv to "Required") during
decoding, as dynamic blocks each only decode as a single block. MaxItems
cannot be validated at all, also because of dynamic blocks, which may
have any number of blocks in the config.
@jbardin jbardin requested a review from a team August 20, 2019 14:22
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable to me!

@jbardin jbardin merged commit 9f5fa2a into master Aug 20, 2019
@jbardin jbardin deleted the jbardin/validations branch August 20, 2019 15:20
@ghost
Copy link

ghost commented Sep 20, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants