search

CuyZ / Valinor

PHP library that helps to map any input into a strongly-typed value object structure.
https://valinor.cuyz.io
MIT License
1.31k stars 74 forks source link

v1.8.0: LogicException due to unrecognized namespace #461

Closed eliashaeussler closed 10 months ago

eliashaeussler commented 10 months ago

Hi,

since v1.8.0 I get an exception due to an unrecognized namespace. This seems to be related to #457, because without this change everything works as expected.

Background: I'm trying to map an API response to a domain model which has a named constructor. The constructor has an array type annotated which contains another nested domain model:

Here's a workflow run that failed due to the change: https://github.com/CPS-IT/personio-jobs/actions/runs/7351388708/job/20014655808?pr=131

When running locally, I get another error:

Failed asserting that exception of type "AssertionError" matches expected exception "CPSIT\Typo3PersonioJobs\Exception\MalformedApiResponseException". Message was: "assert(!$type instanceof UnresolvableType)" at
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Shell.php:32
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Shell.php:42
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ObjectNodeBuilder.php:50
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ObjectNodeBuilder.php:23
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/NativeClassNodeBuilder.php:39
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/CasterNodeBuilder.php:24
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/UnionNodeBuilder.php:37
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/InterfaceNodeBuilder.php:51
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/CasterProxyNodeBuilder.php:24
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/IterableNodeBuilder.php:26
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/StrictNodeBuilder.php:36
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ErrorCatcherNodeBuilder.php:33
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/RootNodeBuilder.php:16
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ListNodeBuilder.php:58
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ListNodeBuilder.php:37
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/CasterNodeBuilder.php:24
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/UnionNodeBuilder.php:37
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/InterfaceNodeBuilder.php:37
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/CasterProxyNodeBuilder.php:24
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/IterableNodeBuilder.php:26
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/StrictNodeBuilder.php:36
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/ErrorCatcherNodeBuilder.php:33
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/Tree/Builder/RootNodeBuilder.php:16
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/TypeTreeMapper.php:44
~/Sites/cps-it/personio-jobs/.Build/vendor/cuyz/valinor/src/Mapper/TypeTreeMapper.php:25
~/Sites/cps-it/personio-jobs/Classes/Service/PersonioApiService.php:81
~/Sites/cps-it/personio-jobs/Tests/Unit/Service/PersonioApiServiceTest.php:102

Debugging showed me the following in Mapper/Tree/Shell.php:

object(CuyZ\Valinor\Type\Types\UnresolvableType)#648 (2) {
  ["rawType":"CuyZ\Valinor\Type\Types\UnresolvableType":private]=>
  string(43) "array{jobDescription: list<JobDescription>}"
  ["message":"CuyZ\Valinor\Type\Types\UnresolvableType":private]=>
  string(215) "The type `array{jobDescription: list<JobDescription>}` for parameter `CPSIT\Typo3PersonioJobs\Domain\Model\Job::fromApiResponse($jobDescriptions)` could not be resolved: Cannot parse unknown symbol `JobDescription`."
}

When changing the type array{jobDescription: list<JobDescription>} to use a FQCN array{jobDescription: list<\CPSIT\Typo3PersonioJobs\Domain\Model\JobDescription>}, everything works as expected.

wolfy-j commented 10 months ago

It breaks simpler flow as well. We have a custom mapper that unmarshal UUIDs.

$mapper = $this->builder
            ->infer(UuidInterface::class, fn() => Uuid::class)
            ->registerConstructor(
                Uuid::fromString(...)
            )
            ->mapper();

Works with proper UUID mapping before 1.8.0, and gives an error after 1.8.0 update:

The type UuidInterface for return type of method Ramsey\Uuid\Uuid::fromString() could not be resolved: Cannot parse unknown symbol UuidInterface.

Rolling back to 1.7.0 fixes the issue.

boesing commented 10 months ago

This change leads to the BC break:

https://github.com/CuyZ/Valinor/commit/a462fe1539a6553af26583fc99f09dfb33b49959

I have no clue what was fixed with the commit as there seems to be no corresponding issues which needed to be resolved in the first place (at least I am unable to see that from the commit history which did not introduce a test-case for the fixed issue).

I guess we are conflicting with valinor >= 1.8.0 for now until we now how @romm figured out a solution which does not break new stuff implemented in 1.8.0.

romm commented 10 months ago

Hi there, my apologies for quite a big regression! 😬

FYI commit a462fe1539a6553af26583fc99f09dfb33b49959 was introduced because of https://github.com/CuyZ/Valinor/issues/394#issuecomment-1746722996 — unfortunately tests would not cover the usecase you described in this issue.

All these cases should be fixed with #463, but before I merge it and release it would you mind checking this branch out (or apply the patch) in your applications and see if everything is fine?

Thanks!

eliashaeussler commented 10 months ago

Hi @romm, thanks for taking care of the issue! I can confirm that the fix works on my end, tests are green again. :tada:

romm commented 10 months ago

Alright let's go for it, thanks!

romm commented 10 months ago

Released as 1.8.1, enjoy!

eliashaeussler commented 10 months ago

Thanks, @romm! ❤️

boesing commented 10 months ago

Hi there, my apologies for quite a big regression! 😬

Happens to the best. Thanks for the immediate fix.

boesing commented 10 months ago

@romm Not sure if this is related but I do now get the following exception:

CuyZ\Valinor\Definition\Repository\Cache\Compiler\Exception\TypeCannotBeCompiled with

The type CuyZ\Valinor\Type\Types\CallableType cannot be compiled.

That issue did not appear in 1.8.0 (maybe due to all tests failing regarding the issue fixed here) and did not appear in 1.7.0 either. I will try to dive deeper into this and provide a dedicated issue for that problem.

romm commented 10 months ago

Hi @boesing, definitely not the same issue. I'll take a look right now, could you open a separate issue for the tracking?

Thanks for the report!

romm commented 10 months ago

Done in #465. Could you give it a try?