Serialization Context Naming Strategy

[dgafka]

This introduces possibility to set custom naming strategy for given serialization/deserialization using SerializationContext.

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT

[dgafka]

I can't find the cause of error in scrutinizer.
@scyzoryck can you point me to it? :)

[scyzoryck]

Hi! @dgafka - code looks good, thanks! Could you try to rebase your branch based on the newest master? I hope it might help :)

Best, scyzoryck.

[dgafka]

@scyzoryck @goetas can this be reviewed / merged? :)

[scyzoryck]

The rest of the code looks good, thanks for contribution! Could you add two missing test scenarios to make sure that we are not missing anything, please?
It would be nice to add new test case for PHPBench to check how it will impact performance of serialisation ;)

Best, Marcin.

[dgafka]

Hey @goetas @scyzoryck

I've covered changing context in next serialization runs and using context together with naming attributes.

Can you review? :)

[dgafka]

@scyzoryck can we push this forward? :)

[scyzoryck]

@dgafka Thanks for changes! I left one additional request. Could you also describe this feature in the docs, please?

[dgafka]

@scyzoryck docs updated.
Can review?

cc:
@goetas

[dgafka]

@scyzoryck thanks for reviewing :)

@goetas can we merge this?

[dgafka]

Will fixing the conflicts make it possible to merge?

[scyzoryck]

Hi! @dgafka - fine for me to merge it :)

[dgafka]

Hey @scyzoryck, PR rebased and green :)

[scyzoryck]

Thanks! Great coverage with tests, no impact on the performance. 👍
I left 2 small comments :)

[scyzoryck]

I'm afraid it can has some side effects if $this->context->getPropertyNamingStrategy() is null as we are not going to clone at all.

[dgafka]

Ah true. Should we clone always or leave this part code as it was before?

[scyzoryck]

I would left it as it used to be :)

[scyzoryck]

Seems to be not used anymore.

[scyzoryck]

Just in general I wonder about use case that it would solve - I do not know so many cases when it would require dynamic change during of the (de-)serialisation naming. On the other hand generating name of the function was one of the biggest performance updates when migration from 1.x to 2.x so using this feature might cause some performance issues. ;-)

[dgafka]

Just in general I wonder about use case that it would solve - I do not know so many cases when it would require dynamic change during of the (de-)serialisation naming. On the other hand generating name of the function was one of the biggest performance updates when migration from 1.x to 2.x so using this feature might cause some performance issues. ;-)

The feature is opt-in, so should not affect current usage if I am not mistaken?

And the use case is to use same JMS instance for two different purposes, like serialization of API responses (camel cases needed) and to do Database Mapping (snake cases needed).

[scyzoryck]

Just in general I wonder about use case that it would solve - I do not know so many cases when it would require dynamic change during of the (de-)serialisation naming. On the other hand generating name of the function was one of the biggest performance updates when migration from 1.x to 2.x so using this feature might cause some performance issues. ;-)

The feature is opt-in, so should not affect current usage if I am not mistaken?

Yes it is optin - so only performance impact will be with custom naming strategy.

And the use case is to use same JMS instance for two different purposes, like serialization of API responses (camel cases needed) and to do Database Mapping (snake cases needed).
Ok, but IMO especially for those operations we should care about performance - so having 2 instances that are cached seems to be still better option for me - specially that they will be in a different application layers and can be configure independently :) For the cases that you mentioned you might also need more options soon - like different config for enums or datetime handlers.
We can also make some call if'd be helpful for you :D

When I was taking another look I spotted another potential issue - when passing it into context it overrides all names - also the name defined in attribute/annotation. This behaviour is different than the default one - so it can potential source of the issues for other users. :(

[dgafka]

so having 2 instances that are cached seems to be still better option for me - specially that they will be in a different application layers and can be configure independently :) For the cases that you mentioned you might also need more options soon - like different config for enums or datetime handlers.

The configuration will stay the same, so that's fine. I've checked how much bootstrapping ($builder->build) takes and it around 5ms (with almost no config, like custom Handlers) for the simplest case and I think it may increase when there is more configuration involved.
Therefore having like three instances (identical property, camel case, underscores), seems like a bit of overkill, especially that only one of them will be used in most of the cases.

I may hack my way around, and try to bootstrap them conditionally depending on the usage. This is not ideal but at least there wouldn't be bootstraping penalty.
But do we know, how much using the solution from the PR would affect performance? As if the percentage is small, then I would still consider it preferable over multiple instances of JMS.

[scyzoryck]

With following bench added:

class JsonCamelCaseSerializationBench extends JsonSerializationBench
{
    protected function createContext(): SerializationContext
    {
        return parent::createContext()->setPropertyNamingStrategy(new CamelCaseNamingStrategy());
    }
}

Results on my local was:

\JMS\Serializer\Tests\Benchmark\Performance\JsonCamelCaseSerializationBench

    benchSerialization......................I2 - Mo973.740ms (±2.40%)

\JMS\Serializer\Tests\Benchmark\Performance\JsonSerializationBench

    benchSerialization......................I2 - Mo853.821ms (±2.72%)

So the difference seems to be around 15% ;-)