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

Validation #405

Merged
merged 2 commits into from Feb 17, 2022
Merged

Validation #405

merged 2 commits into from Feb 17, 2022

Conversation

Korbeil
Copy link
Member

@Korbeil Korbeil commented Sep 7, 2020

Fixes #10 (one of the first tickets on this repository !)
Fixes #451

This PR will bring Validation specification thanks to Symfony Validator, this will allow Jane to validate data before normalization or denormalization.
If some validation error is found, Jane will trigger a ValidationException (found in the Runtime directory of your Jane generated files with your choosed namespace) containing a violation list.


Steps:

  • Draft with enum field only
  • Bring needed files with Validator folder
  • Test a generated Normalizer
  • Cover all Validation spec
    • Numeric
    • String
    • Array
    • Object
    • Format
    • Others
  • Implement object Required fields
  • Clean code
  • Add an option to enable/disable validation (will be disabled by default for Client generation)

@er1z
Copy link
Contributor

er1z commented Sep 7, 2020

Last of the points — it'll be quite easy with validation groups. Others — I'd suggest to simply create something like tagged adapters loop with accept and fieldToConstraint methods. Simple and flexible. :)

@Korbeil
Copy link
Member Author

Korbeil commented Sep 8, 2020

Last of the points — it'll be quite easy with validation groups. Others — I'd suggest to simply create something like tagged adapters loop with accept and fieldToConstraint methods. Simple and flexible. :)

I don't see the point of using validation groups here ? Since we want to validate all the model everytime.

What this kind of complexity (tagged adapters, which won't really work like that since Jane is something that should work out of Symfony scope) will bring to Jane ? instead of this validator file that contains all the constraints ?

@Korbeil
Copy link
Member Author

Korbeil commented Oct 8, 2020

@joelwurtz gave me a quick review about how he thinks the validation should be done and gave me a really good example of how he see the generated validator:

namespace Jane\JsonSchema\Tests\Expected\Validator;

class ModelValidator implements Jane\JsonSchema\Tests\Expected\Validator\ValidatorInterface
{
    public function __construct(Symfony\Component\Validator\Validation $validator = null)
    {
        $this->validator = $validator ??  \Symfony\Component\Validator\Validation::createValidator();
    }
    public function validate($data): ConstraintViolationListInterface
    {
        return $this->validator->validate($data, self::getConstraint());
    }
    public function static getConstraint(): ConstraintInterface
    {
         // add some cache here ~
         return new \Symfony\Component\Validator\Constraints\Collection(array('enumString' => new \Symfony\Component\Validator\Constraints\Required(array(new \Symfony\Component\Validator\Constraints\Choice(array('choices' => array('foo', 'bar', 'baz'), 'message' => '"{{ value }}" is not part of the set of possible choices for this field: "{{ choices }}".')))), 'enumArrayString' => new \Symfony\Component\Validator\Constraints\Required(array(new \Symfony\Component\Validator\Constraints\Choice(array('choices' => array('foo', 'bar', 'baz'), 'message' => '"{{ value }}" is not part of the set of possible choices for this field: "{{ choices }}".')))), 'enumNoType' => new \Symfony\Component\Validator\Constraints\Required(array(new \Symfony\Component\Validator\Constraints\Choice(array('choices' => array('foo', 'bar', 'baz'), 'message' => '"{{ value }}" is not part of the set of possible choices for this field: "{{ choices }}".'))))));
    }
}

And was telling me to avoid using exception to get the violation list, he would more see the client returning violations directly when needed (this meant that I should move validation from Normalizers to the Client)

@Korbeil Korbeil force-pushed the next branch 15 times, most recently from 72b13a9 to 200cd7f Compare December 8, 2020 20:18
@Jrobles-Oniad
Copy link

Jrobles-Oniad commented Feb 24, 2021

Hi! What is the status of this issue?
I've considered other alternatives like using Opis JsonSchema before passing to the normalizer, but still prefer using Symfony validation that provides tighly integration.
Can you explain how is this planned to work? maybe generating the '@Assert::constraint` on property annotations will suffice?
Thanks!

@Korbeil
Copy link
Member Author

Korbeil commented Feb 24, 2021

Hi! What is the status of this issue?
I've considered other alternatives like using Opis JsonSchema before passing to the normalizer, but still prefer using Symfony validation that provides tighly integration.
Can you explain how is this planned to work? maybe generating the '@Assert::constraint` on property annotations will suffice?
Thanks!

Actually I did not plan to work on this topic anytime soon since I'm more focusing on v7 and AutoMapper features. Feel free to take over, I made a very detailed todo list in the PR description to what's missing but the big part that is missing is implementing each spec functionality one by one, the validation model is already working.
I'm also glad to take this back if you want to sponsor me, otherwise it will be planned later this year (mid/end of the year probably).

@Jrobles-Oniad
Copy link

Ok, thanks, I'll do some tests with the PR.

@Korbeil Korbeil force-pushed the feature/validator branch 2 times, most recently from 6ce044f to ade8284 Compare February 4, 2022 14:25
@Korbeil Korbeil marked this pull request as ready for review February 4, 2022 14:26
@Korbeil Korbeil force-pushed the feature/validator branch 8 times, most recently from e588e2e to f5fb063 Compare February 5, 2022 13:45
@Korbeil
Copy link
Member Author

Korbeil commented Feb 5, 2022

And it's done and open for reviews ! @Gounlaf maybe you could take a try there ?

@Gounlaf
Copy link
Contributor

Gounlaf commented Feb 5, 2022

And it's done and open for reviews ! @Gounlaf maybe you could take a try there ?

With pleasure. Will do tonight or tomorrow 😉

CHANGELOG.md Outdated Show resolved Hide resolved
documentation/components/JsonSchema.rst Outdated Show resolved Hide resolved
@Gounlaf
Copy link
Contributor

Gounlaf commented Feb 6, 2022

@Korbeil could you update options and split it in two?

As a user, I may want to validate my data before making call to the API I consume, but I don't want to validate that API is sending me "valid" data.
And vice versa; I could have have a validation process (on my own) before calling the API, so I don't need the built-in one, but I would like to validate APIs returns.

So it would be a double options; one for normalization, another one for de-normalization

@Korbeil Korbeil force-pushed the feature/validator branch 4 times, most recently from 5c57325 to 690103c Compare February 10, 2022 08:55
Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

Just some wording suggestions in the documentation

documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
documentation/guides/validation.rst Show resolved Hide resolved
@Korbeil Korbeil force-pushed the feature/validator branch 2 times, most recently from 90f8292 to d602653 Compare February 15, 2022 17:17
@Korbeil Korbeil merged commit 4660ecb into janephp:next Feb 17, 2022
@Korbeil Korbeil deleted the feature/validator branch February 17, 2022 12:09
@Korbeil Korbeil mentioned this pull request Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for simulating Symfony's AbstractNormalizer::ALLOW_EXTRA_ATTRIBUTES [Roadmap] Add Validation Support
5 participants