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.

Sign up for GitHub

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

Jump to bottom

Add specification property field's values restrictions based on validator setting #3329

Merged
merged 1 commit into from Feb 18, 2020
Merged

Add specification property field's values restrictions based on validator setting #3329

merged 1 commit into from Feb 18, 2020

Conversation

penja
Copy link
Contributor

@penja penja commented Dec 25, 2019 •
edited

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tickets fixes #3319
License MIT
Doc PR api-platform/docs#1015

Generate specification property restrictions based on Symfony’s built-in validator.

@penja penja changed the title Add specification property field's values restrictions based on validator setting Add specification property field's values restrictions based on validator setting (WIP) Dec 25, 2019
@penja penja mentioned this pull request Dec 28, 2019
Merged
@penja penja force-pushed the property_shema_validation_restrictions branch from d912241 to 75d8123 Compare December 28, 2019 15:37
@penja
Copy link
Contributor Author

penja commented Dec 28, 2019

@dunglas @soyuka I've added implementation. Could you please review the PR? Thanks in advance

@penja penja changed the title Add specification property field's values restrictions based on validator setting (WIP) Add specification property field's values restrictions based on validator setting Dec 28, 2019
soyuka
soyuka reviewed Jan 6, 2020
View reviewed changes
src/Bridge/Symfony/Validator/Metadata/Property/Restriction/PropertySchemaRegexRestriction.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php
}
}
}

return $propertyMetadata->withIri($iri)->withRequired($required ?? false);
$propertyMetadata = $propertyMetadata->withIri($iri)->withRequired($required ?? false);
Copy link
Member

Choose a reason for hiding this comment

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

We don't break the above loop anymore, and it looks like we can have multiple constraints, hence different iris? I'm not sure that this case is handled / implemented correctly.

Copy link
Contributor Author

@penja penja Jan 8, 2020 •
edited

Choose a reason for hiding this comment

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

@soyuka Like in previous implementation we continue to set IRI and required only if it was not set before.

src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php
$schema = [];
}

$schema += array_merge(...$restrictions);
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this spread operator / array_merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we must merge all restrictions into one schema

src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php Outdated Show resolved Hide resolved
tests/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactoryTest.php Outdated Show resolved Hide resolved
tests/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactoryTest.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Validator/Metadata/Property/Restriction/PropertySchemaLengthRestriction.php Outdated Show resolved Hide resolved
@penja penja requested a review from soyuka January 8, 2020 14:02
@soyuka soyuka requested a review from dunglas January 10, 2020 14:16
dunglas
dunglas requested changes Jan 10, 2020
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Great addition! Thanks for working on this.

src/Bridge/Symfony/Validator/Metadata/Property/Restriction/PropertySchemaFormat.php Outdated Show resolved Hide resolved
src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php
@@ -67,11 +68,19 @@ final class ValidatorPropertyMetadataFactory implements PropertyMetadataFactoryI

private $decorated;
private $validatorMetadataFactory;
/**
* @var iterable<PropertySchemaRestrictionMetadataInterface>
Copy link
Member

Choose a reason for hiding this comment

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

Can you move it to the constructor please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php Outdated
public function __construct(ValidatorMetadataFactoryInterface $validatorMetadataFactory, PropertyMetadataFactoryInterface $decorated)
{
public function __construct(
ValidatorMetadataFactoryInterface $validatorMetadataFactory,
Copy link
Member

Choose a reason for hiding this comment

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

We put every parameters on the same line (same everywhere).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php Outdated Show resolved Hide resolved
src/Metadata/Property/PropertyMetadata.php Outdated
public function __construct(Type $type = null, string $description = null, bool $readable = null, bool $writable = null, bool $readableLink = null, bool $writableLink = null, bool $required = null, bool $identifier = null, string $iri = null, $childInherited = null, array $attributes = null, SubresourceMetadata $subresource = null, bool $initializable = null, $default = null, $example = null)
{
/**
* @var array
Copy link
Member

Choose a reason for hiding this comment

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

to remove (and should be array|null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@penja
Copy link
Contributor Author

penja commented Jan 10, 2020

@dunglas @soyuka Thanks for the CR.

@penja penja requested a review from dunglas January 10, 2020 20:31
soyuka
soyuka reviewed Jan 20, 2020
View reviewed changes
src/Bridge/Symfony/Validator/Metadata/Property/ValidatorPropertyMetadataFactory.php Outdated
/**
* @param PropertySchemaRestrictionMetadataInterface[] $restrictionsMetadata
*/
public function __construct(ValidatorMetadataFactoryInterface $validatorMetadataFactory, PropertyMetadataFactoryInterface $decorated, iterable $restrictionsMetadata)
Copy link
Member

Choose a reason for hiding this comment

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

Can we default iterable $restrictionsMetadata to an array? I fear it may break some implementations if we leave this as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soyuka I've added default value

soyuka
soyuka approved these changes Jan 20, 2020
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

nice thanks, may you rebase/sqash? I'd like to merge this.

@penja penja force-pushed the property_shema_validation_restrictions branch from 0a7461a to e9bdc58 Compare January 20, 2020 14:24
@penja
Copy link
Contributor Author

penja commented Jan 20, 2020

@soyuka Thanks, done.

@soyuka soyuka force-pushed the property_shema_validation_restrictions branch from e9bdc58 to 38d6955 Compare February 18, 2020 14:48
@soyuka soyuka merged commit dee4c09 into api-platform:master Feb 18, 2020
@soyuka
Copy link
Member

soyuka commented Feb 18, 2020

Thanks @penja!

@penja penja deleted the property_shema_validation_restrictions branch February 20, 2020 13:57
guilliamxavier
guilliamxavier reviewed Mar 31, 2021
View reviewed changes
src/Bridge/Symfony/Validator/Metadata/Property/Restriction/PropertySchemaLengthRestriction.php
Comment on lines +47 to +55
case Type::BUILTIN_TYPE_INT:
case Type::BUILTIN_TYPE_FLOAT:
if (isset($constraint->min)) {
$restriction['minimum'] = (int) $constraint->min;
}

if (isset($constraint->max)) {
$restriction['maximum'] = (int) $constraint->max;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part looks wrong. Length is a string constraint. There seems to be confusion with Range, implemented in #4158

Copy link
Member

Choose a reason for hiding this comment

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

That's right. Could you provide a PR to fix it?

Copy link
Contributor

Choose a reason for hiding this comment

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

PR: #4177

Copy link
Contributor

Choose a reason for hiding this comment

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

PS: thanks @penja for implementing this feature!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guilliamxavier thanks for fix

@guilliamxavier guilliamxavier mentioned this pull request Mar 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guilliamxavier guilliamxavier guilliamxavier left review comments

@alanpoulain alanpoulain alanpoulain left review comments

@dunglas dunglas dunglas approved these changes

@soyuka soyuka soyuka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Result OpenAPI file doesn't contain field's values restrictions based on validator setting
5 participants
@penja @soyuka @dunglas @guilliamxavier @alanpoulain