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

fix: defaults in ordered array are not filled #2548

Merged
merged 10 commits into from
Feb 8, 2021

Conversation

iifawzi
Copy link
Contributor

@iifawzi iifawzi commented Jan 27, 2021

Fixes #2404

This will ensure that, if any remaining ordered contains a default constraint, it will be pushed to the value array.

        if (ordered._flags.hasOwnProperty('default')) {
            value.push(ordered._flags.default);
        }

also, i changed the name of fillOrderedErrors method to checkRemainingOrdereds for clarity, since it's now not only filling errors.

@iifawzi
Copy link
Contributor Author

iifawzi commented Jan 27, 2021

i've read the contributing rules, i decided to go further fixing it, as i found it already marked as a bug, and it's such a great feature to be fixed. i hope it helps, and i hope i'm not violating the rules.

@brianle1301
Copy link
Contributor

@iifawzi This doesn't work with refs.

lib/types/array.js Outdated Show resolved Hide resolved
@brianle1301
Copy link
Contributor

Not sure how others would like it, but I'd do something like the object type. Instead of automatically filling default values, you need to explicitly call default() on the array schema to trigger this process. This offers more flexibility imo.

lib/types/array.js Outdated Show resolved Hide resolved
lib/types/array.js Outdated Show resolved Hide resolved
@brianle1301
Copy link
Contributor

brianle1301 commented Feb 3, 2021

@iifawzi
To sum up, here are a few areas of improvement:

  • Have 2 separate loops, 1 for filling errors and 1 for filling default values (keep the current fillOrderedErrors function)
  • Retain item order even if we have to generate sparse arrays. Ignore trailing undefineds. A few example outputs:
[1, 2]; // OK
[1, undefined, 2]; // OK (although empty slots are more preferable than undefineds, if an item is undefined just increase the insert pointer i)
[1, undefined, 2, undefined]; // Not OK -> [1, undefined, 2] instead
[undefined, 1, 2] // OK

The best way to implement this, as I said, would be to loop the schemas reversely, and discard any undefineds before an actual value is encountered.

  • Tests (normal values, references, templates, nested objects, nested arrays, trailing sparsed items, etc.)
  • (Not entirely sure about this) Call state.localize() with path and ancestors and pass this localised state instead of raw state to $_validate() so references and links are resolved correctly. Refer to how ordered schemas are validated within the same file.

Also cc @hueniverse for validation and approval.

@iifawzi
Copy link
Contributor Author

iifawzi commented Feb 3, 2021

@iifawzi
To sum up, here are a few areas of improvement:

  • Have 2 separate loops, 1 for filling errors and 1 for filling default values (keep the current fillOrderedErrors function)
  • Retain item order even if we have to generate sparse arrays. Ignore trailing undefineds. A few example outputs:
[1, 2]; // OK
[1, undefined, 2]; // OK (although empty slots are more preferable than undefineds, if an item is undefined just keep and increase the insert pointer i)
[1, undefined, 2, undefined]; // Not OK -> [1, undefined, 2] instead
[undefined, 1, 2] // OK

The best way to implement this, as I said, would be to loop the schemas reversely, and discard any undefineds before an actual value is encountered.

  • Tests (normal values, references, templates, nested objects, nested arrays, trailing sparsed items, etc.)
  • (Not entirely sure about this) Call state.localize() with path and ancestors and pass this localised state instead of raw state to $_validate() so references and links are resolved correctly. Refer to how ordered schemas are validated within the same file.

Also cc @hueniverse for validation and approval.

Great, this's clear now. I will commit the changes tomorrow night, as it's 3 AM here (GMT+2), thanks @brianle1301 for your valuable time, and for summing this up.

@iifawzi iifawzi force-pushed the fix-defaults-ordered-not-filled branch from 2193d16 to 36f24d2 Compare February 3, 2021 18:47
@iifawzi
Copy link
Contributor Author

iifawzi commented Feb 3, 2021

Hi, @brianle1301, i've made all mentioned points, added more tests, it seems much better now, dynamic schemas are resolved, a separate loop is used. i hope we're covering all cases now, waiting for your review.

@hueniverse, When you got a time to check, Is it looks good? awaiting for your review.

@brianle1301
Copy link
Contributor

@iifawzi Can you please mark all the previous reviews as resolved? Just to clean things up a bit

lib/types/array.js Outdated Show resolved Hide resolved
lib/types/array.js Outdated Show resolved Hide resolved
test/types/array.js Outdated Show resolved Hide resolved
test/types/array.js Outdated Show resolved Hide resolved
test/types/array.js Show resolved Hide resolved
test/types/array.js Outdated Show resolved Hide resolved
lib/types/array.js Outdated Show resolved Hide resolved
lib/types/array.js Outdated Show resolved Hide resolved
@hueniverse hueniverse self-assigned this Feb 7, 2021
@hueniverse hueniverse added the feature New functionality or improvement label Feb 8, 2021
@hueniverse hueniverse added this to the 17.3.1 milestone Feb 8, 2021
@hueniverse hueniverse merged commit 7ad4a2e into hapijs:master Feb 8, 2021
@hueniverse hueniverse added the bug Bug or defect label Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

defaults in ordered array are not filled
3 participants