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

ObjectMapperCodeGenerator doesn't recurse for serializeObject() #75

Open
Nadyita opened this issue Sep 6, 2024 · 14 comments
Open

ObjectMapperCodeGenerator doesn't recurse for serializeObject() #75

Nadyita opened this issue Sep 6, 2024 · 14 comments

Comments

@Nadyita
Copy link
Contributor

Nadyita commented Sep 6, 2024

I have a (shortened) class like this:

class ImplantConfig {
	/** psalm-param int<1,300> $ql */
	public function __construct(
		public int $ql=300,
		public ?SlotConfig $head=null,
	) {
	}
}

When I use the ObjectMapperCodeGenerator, it recurses into SlotConfig and further down, but only for hydration.
The generated code looks like this:

    /**
     * @template T of object
     * @param class-string<T> $className
     * @return T
     */
    public function hydrateObject(string $className, array $payload): object
    {
        return match($className) {
            'Nadybot\Modules\IMPLANT_MODULE\ImplantConfig' => $this->hydrateNadybot⚡️Modules⚡️IMPLANT_MODULE⚡️ImplantConfig($payload),
                'Nadybot\Modules\IMPLANT_MODULE\SlotConfig' => $this->hydrateNadybot⚡️Modules⚡️IMPLANT_MODULE⚡️SlotConfig($payload),
                'Nadybot\Modules\IMPLANT_MODULE\SymbiantSlot' => $this->hydrateNadybot⚡️Modules⚡️IMPLANT_MODULE⚡️SymbiantSlot($payload),
                'Nadybot\Modules\IMPLANT_MODULE\AbilityAmount' => $this->hydrateNadybot⚡️Modules⚡️IMPLANT_MODULE⚡️AbilityAmount($payload),
            default => throw UnableToHydrateObject::noHydrationDefined($className, $this->hydrationStack),
        };
    }

In comparison, the serialization code is this:

    /**
     * @template T
     *
     * @param T               $object
     * @param class-string<T> $className
     */
    public function serializeObjectOfType(object $object, string $className): mixed
    {
        try {
            return match($className) {
                'array' => $this->serializeValuearray($object),
            'Ramsey\Uuid\UuidInterface' => $this->serializeValueRamsey⚡️Uuid⚡️UuidInterface($object),
            'DateTime' => $this->serializeValueDateTime($object),
            'DateTimeImmutable' => $this->serializeValueDateTimeImmutable($object),
            'DateTimeInterface' => $this->serializeValueDateTimeInterface($object),
            'Nadybot\Modules\IMPLANT_MODULE\ImplantConfig' => $this->serializeObjectNadybot⚡️Modules⚡️IMPLANT_MODULE⚡️ImplantConfig($object),
                default => throw new \LogicException("No serialization defined for $className"),
            };
        } catch (\Throwable $exception) {
            throw UnableToSerializeObject::dueToError($className, $exception);
        }
    }

As you can see, it doesn't recurse, and to me, that is a bug. I'm using the latest 1.5.0 release. Am I mistaken?

frankdejonge added a commit that referenced this issue Sep 6, 2024
@frankdejonge
Copy link
Member

Hi, I'm trying to reproduce your issue, but I'm failing to do so. I did uncover another issue, which is resolved in the PR. Could you see if this represents your case? And if not, can you provide a case to reproduce your issue?

@Nadyita
Copy link
Contributor Author

Nadyita commented Sep 6, 2024

Sadly, it doesn't fix my issue. I'll try to create a minimum working example, because my current codebase is 760k LOC

@Nadyita
Copy link
Contributor Author

Nadyita commented Sep 6, 2024

Here we go:

<?php

declare(strict_types=1);

require_once './vendor/autoload.php';

use EventSauce\ObjectHydrator\ObjectMapperCodeGenerator;

class A
{
    public function __construct(
        public string $x,
    ) {
    }
}

class B
{
    public function __construct(
        public ?A $a,
    ) {
    }
}

$b = new B(a: new A(x: "here"));

$mapper = new ObjectMapperCodeGenerator();
$code = $mapper->dump([$b::class], 'Seri\\Tester');
$fileName = tempnam('/tmp/', 'seri_');
file_put_contents($fileName, $code);
require_once $fileName;
$serializer = new \Seri\Tester();
echo(json_encode($serializer->serializeObject($b)) . PHP_EOL);

This throws the error Call to undefined method Seri\Tester::serializeObjectA(), because it doesn't recurse into it. hydration works perfectly fine, though.

@frankdejonge
Copy link
Member

Right, the issue seems to be in the class expanding mechanism, If you add A::class into the dump function it's all fine.

@frankdejonge
Copy link
Member

I think I've got it. Can you checkout the PR I linked earlier?

@frankdejonge
Copy link
Member

Checking out an unintended side-effect of the fix now.

@frankdejonge
Copy link
Member

Your code should work in the PR branch now.

@Nadyita
Copy link
Contributor Author

Nadyita commented Sep 6, 2024

Yes, but the original one doesn't, because it's generating duplications. Minimum working proof:

<?php

declare(strict_types=1);

require_once './vendor/autoload.php';

use EventSauce\ObjectHydrator\ObjectMapperCodeGenerator;

class A
{
    public function __construct(
        public string $x,
    ) {
    }
}

class B
{
    public function __construct(
        public A $a,
    ) {
    }
}

class C
{
    public function __construct(
        public A $a,
        public B $b,
    ) {
    }
}

$c = new C(a: new A(x: "haha!"), b: new B(a: new A(x: "here")));

$mapper = new ObjectMapperCodeGenerator();
$code = $mapper->dump([$c::class], 'Seri\\Tester');
$fileName = tempnam('/tmp/', 'seri_');
file_put_contents($fileName, $code);
require_once $fileName;
$serializer = new \Seri\Tester();
echo(json_encode($serializer->serializeObject($b)) . PHP_EOL);

This throws Cannot redeclare Seri\Tester::serializeObjectA()

@frankdejonge
Copy link
Member

A quick array_unique resolved that.

@Nadyita
Copy link
Contributor Author

Nadyita commented Sep 6, 2024

Okay, now I'm getting other errors. Might take a break for today, but it seems it happens when I have this:

<?php declare(strict_types=1);

namespace Nadybot\Modules\IMPLANT_MODULE;

class SymbiantSlot {
	/**
	 * @param AbilityAmount[] $reqs
	 * @param AbilityAmount[] $mods
	 *
	 * @psalm-param list<AbilityAmount> $reqs
	 * @psalm-param list<AbilityAmount> $mods
	 */
	public function __construct(
		public string $name,
		public int $treatment,
		public int $level,
		public array $reqs,
		public array $mods,
	) {
	}
}

For this case, it reports No serialization defined for Nadybot\\Modules\\IMPLANT_MODULE\\AbilityAmount, so my guess is that annotations are ignored. I gotta go to bed, but maybe you can modify the previous example to reproduce the error. Sorry for being such a pain.

@frankdejonge
Copy link
Member

The class expansion mechanism is trying to do its best to resolve everything, but the more things are defined in relation to custom tooling, like psalm or via docblocks, the more its guesswork and not exact science. My recommendation is always to use league/construct-finder (which I also built) to resolve all classes and not rely on the class expansion mechanism that's built into this.

@Nadyita
Copy link
Contributor Author

Nadyita commented Sep 6, 2024

Well, I don't have much choice with array not being typed in PHP 🤷 and I thought that the algorithm for serialization and hydration was working the same way.
Sadly, I can't use tools that scan directories, because the code allows 3rd party plugins and who knows which of them are serializing which object. That is why I wrote a wrapper that compiles hydrators and serializers on the fly, so I don't have to know where all this is. And whenever the serializer throws exceptions, it uses the reflection-based one. Plus, I'm also using objects from other libraries, and they tend to move around from time to time 🙁

@frankdejonge
Copy link
Member

You can also take a stab at trying to solve it yourself. I'm doing my best to help you, providing support for a free tool that I literally do not benefit from in any shape or capacity.

@Nadyita
Copy link
Contributor Author

Nadyita commented Sep 6, 2024

Yes, I could, but it might take some time, before I'm familiar enough with the code to fix this kind of thing. Sadly, time doesn't grow on trees, and I was in the middle of preparing a new release when I noticed performance impacts when using the reflection version. So yes, I'm gonna try and fix it, but it will take a bit of time 😞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants