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

Allow object keys to be verified by schema #1499

Merged
merged 3 commits into from
Jun 6, 2018
Merged

Allow object keys to be verified by schema #1499

merged 3 commits into from
Jun 6, 2018

Conversation

rgoble4
Copy link
Contributor

@rgoble4 rgoble4 commented May 12, 2018

I saw this issue: #1382 and thought I might take a shot at it. It adds dynamicKeys to support validating object keys with schema instead of requiring a regex parameter. It also reworks pattern to just use dynamicKeys under the hood, but that might not be a great thing to stick with if there is a large performance gain in testing a single regex vs a schema that has a single regex rule.

As part of this, I also updated describe, but cannot for the life of me figure out how to make the describe test pass. Based on the output, the actual and expected describe results are identical, but the test still fails. edit: I just had to sleep on it, found and fixed.

Anyway, I hope this helps or at least gives some folks ideas on how they might want to make this feature happen. If this is even reasonably close to mergeable, I'm happy to keep working on it.

@Marsup
Copy link
Collaborator

Marsup commented May 21, 2018

I'd like to stay closer to what currently exists, integrating schemas into the current code should be fairly easy, no need for a new API or casting values, I'd just make pattern() accept schemas as 1st argument.

@rgoble4
Copy link
Contributor Author

rgoble4 commented May 21, 2018

Ah, I got it in my mind that pattern = regex, but it also makes sense that a schema would essentially still be a pattern for the keys. To be clear, should pattern accept standard regex as it does currently as well as a schema in the first arg to preserve backwards compatibility?

@Marsup
Copy link
Collaborator

Marsup commented May 21, 2018

That would be my intention yes, without casting it to a schema, it would be slower.

@rgoble4
Copy link
Contributor Author

rgoble4 commented May 25, 2018

Ok, updated as discussed. I also rebased and started clean so that there weren't a bunch of line diffs that were related to the direction I had previously gone in. Let me know what else you'd like to see.

@Marsup Marsup added this to the 13.4.0 milestone May 27, 2018
@Marsup Marsup self-assigned this May 27, 2018
@Marsup Marsup added the feature New functionality or improvement label May 27, 2018
API.md Outdated
@@ -1611,6 +1611,18 @@ const schema = Joi.object({
}).pattern(/\w\d/, Joi.boolean());
```

#### `object.pattern(keySchema, valueSchema)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not how we usually document methods with multiple signatures, you should try to make it work with current documentation.

@@ -637,7 +651,12 @@ internals.Object = class extends Any {

for (let i = 0; i < this._inner.patterns.length; ++i) {
const pattern = this._inner.patterns[i];
description.patterns.push({ regex: pattern.regex.toString(), rule: pattern.rule.describe() });
if (pattern.regex) {
description.patterns.push({ patternRule: pattern.patternRule.toString(), valueRule: pattern.valueRule.describe() });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not make this a breaking change please.

@rgoble4
Copy link
Contributor Author

rgoble4 commented May 28, 2018

Good points, updated!

@Marsup Marsup merged commit d97ca0d into hapijs:master Jun 6, 2018
Marsup added a commit that referenced this pull request Jun 6, 2018
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
@rgoble4 rgoble4 deleted the dynamic-keys branch January 15, 2020 02:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants