Skip to content

MinItems and MaxItems validations#22530

Merged
jbardin merged 2 commits intomasterfrom
jbardin/validations
Aug 20, 2019
Merged

MinItems and MaxItems validations#22530
jbardin merged 2 commits intomasterfrom
jbardin/validations

Conversation

@jbardin
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

Development

Successfully merging this pull request may close these issues.

Multiple dynamic blocks for , where only one will ultimately resolve, not working for blocks that only allow 1 max

2 participants