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

Annotation cache does not honor naming strategy #1244

Closed
boesing opened this issue Aug 19, 2020 · 4 comments
Closed

Annotation cache does not honor naming strategy #1244

boesing opened this issue Aug 19, 2020 · 4 comments

Comments

@boesing
Copy link
Contributor

boesing commented Aug 19, 2020

Q A
Bug report? yes

Steps required to reproduce the problem

  1. Create two objects with camelCase annotations where one extends the other
  2. Enable caching
  3. serialize the parent object with the serializer and use underscore naming strategy (this will put parsed annotations to cache with the naming strategy applied)
  4. serialize the extending object with the serializer and use camelCase naming strategy (this will only camel case the extending objects properties)
<?php

use C24\Mobilfunk\Serializer\Naming\UnderscoreNamingStrategy;
use JMS\Serializer\Metadata\PropertyMetadata;
use JMS\Serializer\Naming\PropertyNamingStrategyInterface;
use JMS\Serializer\Naming\SerializedNameAnnotationStrategy;
use JMS\Serializer\SerializerBuilder;

class Foo
{
    public $camelCase1 = '';
    public $camelCase2 = '';
}

class Bar extends Foo
{
    public $camelCase3 = '';
}

$serializerWithUnderscoreStrategy = SerializerBuilder::create()->setCacheDir(sys_get_temp_dir() . '/serializer-test/')->setPropertyNamingStrategy(
    new SerializedNameAnnotationStrategy(new UnderscoreNamingStrategy())
)->build();

$serializerWithCamelCaseStrategy = SerializerBuilder::create()->setCacheDir(sys_get_temp_dir() . '/serializer-test/')->setPropertyNamingStrategy(
    new SerializedNameAnnotationStrategy(new class implements PropertyNamingStrategyInterface {

        public function translateName(PropertyMetadata $property): string
        {
            $translated = $this->camelCase($property, '_');

            return lcfirst($translated);
        }

        private function camelCase(PropertyMetadata $property, string $seperator): string
        {
            $pregQuotedSeparator = preg_quote($seperator, '#');

            $patterns = [
                '#(' . $pregQuotedSeparator . ')([\S]{1})#',
                '#(^[\S]{1})#',
            ];
            $replacements = [
                static function ($matches) {
                    return strtoupper($matches[2]);
                },
                static function ($matches) {
                    return strtoupper($matches[1]);
                },
            ];

            $filtered = $property->name;
            foreach ($patterns as $index => $pattern) {
                $filtered = (string) preg_replace_callback($pattern, $replacements[$index], $filtered);
            }

            return $filtered;
        }
    })
)->build();

$foo = new Foo();
$bar = new Bar();


echo $serializerWithUnderscoreStrategy->serialize($foo, 'json') . PHP_EOL;
echo $serializerWithCamelCaseStrategy->serialize($foo, 'json') . PHP_EOL;
echo $serializerWithCamelCaseStrategy->serialize($bar, 'json') . PHP_EOL;

Expected Result

{"camel_case1":"","camel_case2":""}
{"camelCase1":"","camelCase2":""}
{"camelCase1":"","camelCase2":"","camelCase3":""}

Actual Result

{"camel_case1":"","camel_case2":""}
{"camel_case1":"","camel_case2":""}
{"camel_case1":"","camel_case2":"","camelCase3":""}
@boesing
Copy link
Contributor Author

boesing commented Aug 19, 2020

Fun fact: if you serialize $foo with the camelCase serializer first, camelCase will be cached and underscore would be ignored. Thus, depending on which is used first to create the cache, the naming strategy of that serializer is persisted for all other serializers which are used to serialize the same object.

@goetas
Copy link
Collaborator

goetas commented Aug 20, 2020

You are re using the same cache dir for all the test cases, so the frist one puts the data in cache for the others. You should specify different cache directories

@boesing
Copy link
Contributor Author

boesing commented Aug 20, 2020

What do you mean with different cache directories?
I have a huge API where I'd like to re-use my annotated JMS Objects.

You say, I have to specify a dedicated cache directory for each serializer which has a different naming strategy?

There are actually 3 Naming Strategies provided by the JMS serializer.
That means, I have to create 3 builders with different cache directories in case I want to use all strategies in my project?

Imho, that should be part of the cache.

There are three solutions to achieve what I expect:

  1. create a directory inside metadata cache-directory named by the naming strategy used
  2. create dedicated metadata for objects as a whole and not partially (in my case, there are 2 cache files, one for the Foo and one for the Bar class and these are getting "merged" during runtime)
  3. do not cache the metadata with the naming strategy already applied and apply naming strategy after reading from cache

Imho, the best solution would be having either number 3 or number 2.

I dont think that its common to switch naming strategies on a response object as a whole. We only have issues because the metadata of the Foo is shared with Bar. If the Bar metadata would contain all the informations of Foo (therefore duplicated on FS) would be the best solution. This would also mean that there is just a single unserialize happening for that Bar object instead of two (Foo and Bar cache).

What do you think about this?

@goetas
Copy link
Collaborator

goetas commented Aug 22, 2020

If you have multiple serializers with different configurations using the same cache directory, it is expected to have very strange behaviors (as example the name for each property is cached and not computed at runtime). Another example is that different serializers might have different metadata for some classes... And many other use cases.

Closing this as it is not a bug.

@goetas goetas closed this as completed Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants