search

neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
267 stars 222 forks source link

FEATURE: Stricter server side node property validation (+ ajax endpoint?) #4631

Open mhsdesign opened 1 year ago

mhsdesign commented 1 year ago

related: https://github.com/neos/neos-development-collection/issues/968

i talked with @bwaidelich about this and the general ideas were:

to use the following syntax:

property:
  foo:  
    type: string
    required: true

the behavior would be, that you cannot set the property to null and the property must either be defined at creation time (via creation dialog) or must have an default value.

we thought at first to make required: true an alias to the not empty validator, but introducing flow's validators into the new cr is not what were are looking forward to. Validators are neither in 8.3 nor 9.0 run on the php side but only triggered by the neos ui, if they have a js-validator implementation.

A better approach to validators seem to be value objects like type: Neos\Neos\Types\Email which validate the constrains in the constructor. Those value objects could be created via ajax (maybe via an own requestHandler to not boot flow? and use plain php?) and the ui could then determine in near real time if the field is valid.

Even better would be to actually trigger the cr logic directly to also validate type conflicts and also handle required on the server.

@bwaidelich added:

Even better would be to actually trigger the cr logic directly

IMO we should do both. The UI could interpret that required flag and do that "not empty" logic that it does today already. And for other (non-simple) types it could use that AJAX validation, MH described above. In any case, required flag and type conversion should be tested at command handling time, before the NodePropertiesWereSet event is created

Note: We also briefly discussed support for special syntax ?string, !string (or string!) but IMO it's much more explicit (or rather self-explanatory) to use a separate flag for that

Originally posted by @mhsdesign in https://github.com/neos/neos-development-collection/issues/4304#issuecomment-1578965994

mhsdesign commented 1 year ago

The current state of property validation (Neos 8.3)

Neos <= 9.0 offers property validation via validators configured in properties.[$property].validation.

Those are sorely handled by the Neos Ui. Initially only their javascript implementation was executed inside the Neos Ui javascript Application.

But with https://github.com/neos/neos-ui/pull/2351 validators were also run on server side when properties were changed in the Neos Ui. This feature - as mentioned here - only works for native Neos.Neos validators. Custom validators will be skipped (see code) and also if no Flow validator can be found it will also be skipped.

Custom validators can only be introduced via a javascript plugin for the Neos Ui. Implementing them as php validator will have no effect. (Though it might be interesting to implement ajax handling validator that will run an arbitrary php validator)

A node type with validation would look like:

'Neos.TestNodeTypes:Content.Headline':
  properties:
    title:
      type: string
      # otherwise the node would be inconsistent as there is no node creation dialog configured
      defaultValue: '<h1>Enter headline here</h1>' 
      validation:
        'My.Custom/NeosUi/Javascript/Validator': {}
        'Neos.Neos/Validation/NotEmptyValidator': {}
        'Neos.Neos/Validation/StringLengthValidator':
          minimum: 1
          maximum: 255

Where Neos.Neos/Validation/StringLengthValidator is both a javascript validator provided by the neos ui as well as a flow server validator see Neos\Flow\Validation\Validator\StringLengthValidator.

Problems arise if the validator specifications differ and javascript allows a value where php doesnt.

The custom validator My.Custom/NeosUi/Javascript/Validator is not run on the server because it doesnt start with Neos.Neos.

The content repository package doesnt handle the validators at all. So its possible to set any value to the property regardless of validation. Infact previously to neos 9, the content repository doesnt validate if the node property exists in the schema and if the assigned type matches.

bwaidelich commented 1 year ago

Here's a suggestion that came up in todays Neos 9.0 Weekly:

TL;DR:

Step 1: Adjust the node type schema, deprecate (and eventually remove) validation and add constraints:

'Some.Node:Type':
  properties:
    'someProp':
      type: string
      required: true
      constraints:
        minLength: 3
        maxLength: 20
    'someOtherProp':
      type: array
      constraints:
        minItems: 3
        maxItems: 5

those constraints can be evaluated on the client and on the server side.

Step 2: Provide some API for custom validations that will be triggered via AJAX

Constraints

I'm convinced, that for the mast majority of cases a couple of constraints would be enough (inspired from JSON Schema and graphql constraints directive):

It should be relatively simple (tm) to map todays validation configuration to the above vice versa.

Extension

For custom validation that can't be implemented with the constraints mentioned above, we should not try to implement that on the server- and client-side (as we do today) but provide some API that can be used from the client-side via AJAX and on the server side at command handling time.

I could imagine a new interface like

interface PropertyValidationBla {
    public function validate(mixed $input, ValidationContext $context): ValidationResult
}

that could be registered via Settings for a specific type

Note: The ValidationResult could contain more details why a validation failed

Note: I added the ValidationContext as parameter, because we might need to add "parametrized" validation. For example "image of certain extension, filesize or dimensions" – for this we could consider some unstructured validationOptions in the node type schema

Note: In addition, we could execute the validation if the type itself implements that interface

Examples

Custom Value Object

final readonly class PhoneNumber {

    private function __construct(public string $value) {
        if (!self::isValid($value)) {
            throw new InvalidArgumentException(sprintf('"%s" is not a valid phone number', $value));
        }
    }

    public static function fromString(string $value): self {
        return new self($value);
    }

    public static function isValid(string $value): bool 
    {
        // todo implement
    }
}

final class PhoneNumberValidator implements PropertyValidator {
    public function validate(mixed $input, ValidationContext $context): ValidationResult {
        if (PhoneNumber::isValid($input)) {
            return ValidationResult::succeeded();            
        }
        return ValidationResult::failed('Not a valid phone number');
    }
}

Note: The PhoneNumberValidator would have to be registered as custom validator for the type on the CR in this case. Alternatively the PhoneNumber VO could implement the interface itself

mhsdesign commented 1 year ago

I discussed this matter again with @nezaniel and @grebaldi.   At first server side validation with the goal of producing well guiding error messages seemed a little misplaced as its usually seen in javascript. But the idea is that the VO itself stays clear of the logic to produce a good error message and only does the constraint check.

We also had the idea to not trigger the validation via the registered PropertyValidator, but structure the validation endpoint differently:

This contradicts with your proposal for the validationOptions, which seems more like the legacy validation option which we try to banish.

bwaidelich commented 1 year ago

Suggestion for a generic interface for the server-side part:

<?php

// In Cr Core:

final readonly class ConstraintValidationResult {
    private function __construct(
        public bool $succeeded,
        public ?string $errorMessage,
    ) {}

    public static function succeed(): self
    {
        return new self(true, null);
    }

    public static function fail(string $errorMessage, ...$args): self
    {
        return new self(false, vsprintf($errorMessage, ...$args));
    }
}

interface ConstraintFoo {
    public static function fromOptions(array $options): self;

    public function validate(mixed $value): ConstraintValidationResult;
}

enum StringBasedFormat {
    case email;
    case ipv4;
    // ...
}

final readonly class StringBasedFoo implements ConstraintFoo {

    private function __construct(
        public ?int $minLength,
        public ?int $maxLength,
        public ?string $pattern,
        public ?StringBasedFormat $format,
    ) {}

    public static function fromOptions(array $options): self
    {
        // TODO verify $options
        return new self(
            minLength: $options['minLength'] ?? null,
            maxLength: $options['maxLength'] ?? null,
            pattern: $options['pattern'] ?? null,
            format: $options['format'] ?? null,
        );
    }

    public function validate(mixed $value): ConstraintValidationResult
    {
        if (!is_string($value) && !$value instanceof \Stringable) {
            return ConstraintValidationResult::fail('%s is not a string', get_debug_type($value));
        }
        if ($this->minLength !== null && strlen($value) < $this->minLength) {
            return ConstraintValidationResult::fail('%s does not satisfy min length constraint of', $this->minLength);
        }
        // todo more checks
        return ConstraintValidationResult::succeed();
    }

}

// in Neos.Neos

final readonly class AssetConstraintFoo implements ConstraintFoo {

    private function __construct(
        public string $mediaTypePattern, // e.g. "image/*"
    ) {}

    public static function fromOptions(array $options): self
    {
        // TODO verify $options
        return new self(
            mediaTypePattern: $options['mediaTypePattern'] ?? null,
        );
    }

    public function validate(mixed $value): ConstraintValidationResult
    {
        if (!$value instanceof AssetInterface) {
            return ConstraintValidationResult::fail('%s is not an instance of %s', get_debug_type($value), AssetInterface::class);
        }
        // todo more checks
        return ConstraintValidationResult::succeed();
    }

}
mhsdesign commented 1 year ago

I think we can agree on this basic set on constraints for primitive types. Anything further can be implemented at a later point.

Right now i stumbled upon the fact that the javascript validators might never be async. Me and @Sebobo are thinking about fixing this and this seems to play well together with our neos 9 idea.

mhsdesign commented 5 months ago

As discussed with @kitsunet and @pKallert in todays weekly: