Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support for nullable array wrapping nullable property #492

Closed
GeLoLabs opened this issue Oct 3, 2024 · 5 comments
Closed

Support for nullable array wrapping nullable property #492

GeLoLabs opened this issue Oct 3, 2024 · 5 comments
Labels

Comments

@GeLoLabs
Copy link
Contributor

GeLoLabs commented Oct 3, 2024

Hello,

First thank you for your free time on this library.

Given the following PHP structure:

[
  "property" => null
]

or

[
  "property" => [
     "field" => "value"
  ]
]

or

[
  "property" => [
     "field" => null
  ]
]

If I try to validate them with the following pattern, I would expect it to match for all cases but it does not:

['property' => '@null@||@array@.match({"field": "@string@||@null@"})']

The issue comes from the "OrMatcher" which does a simple explode on the string using the "||" delimiter.

Not sure if it is supposed to be supported or not. Let's first discuss this before looking for a possible fix.

Unfortunately, for now, I was not able to find a way to cover my use case.

@GeLoLabs GeLoLabs added the bug label Oct 3, 2024
@norberttech
Copy link
Member

hey, @null@||@array@ should be totally supported, looks like you nailed that bug! Thanks for taking the initiative and fixing it, much appreciated!

@GeLoLabs
Copy link
Contributor Author

GeLoLabs commented Oct 4, 2024

I'm not sure how this issue should be fixed as I don't know what should be supported in the OrMatcher.

Currently, the matcher does a simple explode on the pattern using "||" as delimiter and so, in my example, it ends with:

[
    '@null@', 
    '@array@.match({"field": "@string@', 
    '@null@"})'
]

instead of

[
    '@null@', 
    '@array@.match({"field": "@string@||@null@"})'
]

Both, "canMatch" and "match" methods on the OrMatcher needs a fix.

We can try to use regex here and only match top level "||" and exclude those which are in a "match()" expander ? But is there other pattern / expanders which need to be supported ?

@norberttech
Copy link
Member

Hey,
I'm sorry I misunderstood your original issue, here is the correct answer, broken into steps.

Step 1: In general, we need to start from creating a pattern that will allow "property" to be an array or null type.

{"property": "@null@||@array@"}

Step 2 Now we should make sure that when "property" is an array it has "field" property

{"property": "@null@||@array@.hasProperty(\"field\")"}

Step 3 Finally we want to allow "field" to be one of ["value", null], so we can use oneOf expander with inArray expanders inside.

{"property": "@null@||@array@.hasProperty(\"field\").oneOf(inArray(\"value\"), inArray(null))"}

Here is a full code example:

<?php

use Coduo\PHPMatcher\PHPMatcher;

require_once __DIR__ . '/../vendor/autoload.php';

$matcher = new PHPMatcher();

$examples = [
    '{"property": null}', // true
    '{"property": {"field": "value"}}', // true
    '{"property": {"field": null}}', // true
    '{"property": {"field": "something else"}}',
];

$pattern = '{"property": "@null@||@array@.hasProperty(\"field\").oneOf(inArray(\"value\"), inArray(null))"}';

foreach ($examples as $example) {
    $result = $matcher->match($example, $pattern);

    echo ($result ? 'match' : 'no match') . ' - ' . $example . PHP_EOL;
}

And the output:

match - {"property": null}
match - {"property": {"field": "value"}}
match - {"property": {"field": null}}
no match - {"property": {"field": "something else"}}

I'm closing this issue since there is nothing really to fix here, in case of any additional questions feel free to comment in this one or open a new issue.

@GeLoLabs
Copy link
Contributor Author

GeLoLabs commented Oct 7, 2024

I understand your approach but IMHO, this is not very DX and becomes very verbose on complex substructure. It would be much more DX to support "||" in substructure but as I pointed out it's not so trivial to implement.

By the way, your example covers a simple property in a substructure what about if I have more than one field ? Can I chain "hasProperty" after the "oneOf" ?

@norberttech
Copy link
Member

I understand your approach but IMHO, this is not very DX and becomes very verbose on complex substructure.

That's a general problem with writing complicated nested patterns. I don't think there is any easy way to make it more user-friendly. I'm totally open to suggestions here, but using regex is not an option - matcher is using lexer because regexes would be too complicated to prepare and maintain later.

My approach is to match the value against multiple patterns.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants