Mapper maps parameter names of inner objects

[TimWolla]

Unfortunately I was not able to think of a better title. Please consider the following failing test case:

<?php

declare(strict_types=1);

namespace CuyZ\Valinor\Tests\Integration\Mapping\Object;

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\MapperBuilder;
use CuyZ\Valinor\Tests\Integration\IntegrationTest;

final class ParameterNamesTest extends IntegrationTest
{
    /**
     * @dataProvider timezoneTargets
     */
    public function test_wrong_parameter_names_throws_exception(string $class): void
    {
        $source = [
            'timezone' => 'UTC',
        ];

        $this->expectException(MappingError::class);

        (new MapperBuilder())->mapper()->map($class, $source);
    }

    /**
     * @dataProvider timezoneTargets
     */
    public function test_right_parameter_names_work(string $class): void
    {
        $source = [
            'tz1' => 'UTC',
        ];

        self::assertSame('UTC', (new MapperBuilder())->mapper()->map($class, $source)->tz1->getName());
    }

    public function timezoneTargets()
    {
        return [
            [Target1::class],
            [Target2::class],
        ];
    }
}

final class Target1
{
    public function __construct(
        public readonly \DateTimeZone $tz1,
    ) {
    }
}

final class Target2
{
    public function __construct(
        public readonly \DateTimeZone $tz1,
        public readonly ?\DateTimeZone $tz2 = null,
    ) {
    }
}

Now consider the following situation:

1. In the initial version of my application the target class is Target1. This class expects a single tz1 parameter containing a value timezone string.
2. The user erroneously sends timezone=UTC (i.e. accidentally uses timezone instead of tz1 for the parameter name).
3. This succeeds, I believe because the first parameter of DateTimeZone::__construct() is called $timezone.
4. In a later version of my application I add a new optional parameter $tz2.
5. Now the client in (2) receives an error. The input was invalid before the new parameter was added, but only the addition of the new optional parameter exposed the error.

[romm]

Hi @TimWolla thanks for the report, I think this issue should be fixed with 72cba32 which is a part of the version 1.0.0 release.

Feel free to re-open if the issue persists. 😉

[TimWolla]

This issue now exists again with 1.2.0. With two new tests for behavior which I understand to be the reason for the revert my expected behavior should now look like this:

<?php

declare(strict_types=1);

namespace CuyZ\Valinor\Tests\Integration\Mapping\Object;

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\MapperBuilder;
use CuyZ\Valinor\Tests\Integration\IntegrationTest;

final class ParameterNamesTest extends IntegrationTest
{
    /**
     * @dataProvider timezoneTargets
     */
    public function test_wrong_parameter_names_throws_exception(string $class): void
    {
        $source = [
            'timezone' => 'UTC',
        ];

        $this->expectException(MappingError::class);

        (new MapperBuilder())->mapper()->map($class, $source);
    }

    /**
     * @dataProvider timezoneTargets
     */
    public function test_right_parameter_names_work(string $class): void
    {
        $source = [
            'tz1' => 'UTC',
        ];

        self::assertSame('UTC', (new MapperBuilder())->mapper()->map($class, $source)->tz1->getName());
    }

    /**
     * @dataProvider timezoneTargets
     */
    public function test_right_parameter_names_with_additional_array_work(string $class): void
    {
        $source = [
            'tz1' => [
                'timezone' => 'UTC',
            ],
        ];

        self::assertSame('UTC', (new MapperBuilder())->mapper()->map($class, $source)->tz1->getName());
    }

    /**
     * @dataProvider timezoneTargets
     */
    public function test_right_parameter_names_with_additional_array_with_incorrect_keys_throws_exception(string $class): void
    {
        $source = [
            'tz1' => [
                'tz' => 'UTC',
            ],
        ];

        $this->expectException(MappingError::class);

        (new MapperBuilder())->mapper()->map($class, $source)->tz1->getName();
    }

    public function timezoneTargets()
    {
        return [
            [Target1::class],
            [Target2::class],
        ];
    }
}

final class Target1
{
    public function __construct(
        public readonly \DateTimeZone $tz1,
    ) {
    }
}

final class Target2
{
    public function __construct(
        public readonly \DateTimeZone $tz1,
        public readonly ?\DateTimeZone $tz2 = null,
    ) {
    }
}

[romm]

Hi @TimWolla, what a fast post-release feedback. 🙂

I honestly forgot about this usecase, I'm sorry it troubles your application. I'll have to give it some thoughts, maybe a flag should be added in the mapper builder to switch the feature on/off. If you see any better way, please let me know.

In the meantime, I re-open this issue.

[TimWolla]

If you see any better way, please let me know.

My understanding is that for single-parameter classes the following should work:

• Directly providing the single parameter with the appropriate key for the "outer object": { "tz1": "Europe/Berlin" }
• Alternatively providing the single parameter within a nested object: { "tz1": { "timezone": "Europe/Berlin" } } (this is the reason for the revert).

But what should not work:

• Using the single parameter with the key of the "nested object": { "timezone": "Europe/Berlin" } # There is no "timezone" key in Target1.

So basically if my updated test passes, the behavior should be correct, it supports the use case in #269, but correctly rejects garbage.

[TimWolla]

What might be reasonable as well is:

• Directly providing the single parameter: "Europe/Berlin" # both Target1 and the inner DateTimeZone only have a single parameter.

[TimWolla]

Oh, now that I'm reading through my comments, I believe I understand the problem: Because we're dealing with two nested single-parameter objects, it's not clear to which of the two a single-value-object applies. The {"timezone": "Europe/Berlin"} input might either be passed as-is to the inner DateTimeZone or interpreted by the Target1 and then only the string passed to the inner DateTimeZone.