Converter performance #526
Conversation
packages/Ecotone/src/Messaging/Conversion/SerializedToObject/DeserializingConverter.php
Outdated
Show resolved
Hide resolved
packages/Ecotone/src/Messaging/Conversion/AutoCollectionConversionService.php
Show resolved
Hide resolved
jlabedo
force-pushed
the
converter-performance-2.1
branch
from
81f3e67 to
0248416
Compare
| if ($conversionService === null) { | ||
| throw ConversionException::create("To convert serialized data to different type than original, you need to set conversion service in " . self::class); | ||
| } | ||
| return $conversionService->convert($phpVar, Type::createFromVariable($phpVar), MediaType::createApplicationXPHP(), $targetType, MediaType::createApplicationXPHP()); |
There was a problem hiding this comment.
The idea behind convert method was to be called, when we know that given Converter can handle conversion, however it feels now that it may be called regardless and require fallback.
Can you explain that design?
There was a problem hiding this comment.
It's hard for me to see the connection between this and static converters
There was a problem hiding this comment.
Do you remember the double conversion code ? It has been replaced by this snippet.
This is really only for the deserializing converter.
Imagine I have an array ["status" => "done"] that is serialized by the serializing converter.
Now, if I am asking for this to be deseriliazed to the Status enum, the deserializing converter would give me an array instead. This is why we need to denormalize further. Does it makes sense ?
There was a problem hiding this comment.
It's hard for me to see the connection between this and static converters
This has nothin to do with static converters, it is another PR :)
There was a problem hiding this comment.
Imagine I have an array ["status" => "done"] that is serialized by the serializing converter.
Now, if I am asking for this to be deseriliazed to the Status enum, the deserializing converter would give me an array instead. This is why we need to denormalize further. Does it makes sense ?
Yep, thanks for explaining :)
If we want to do it in Deserializing only, can we then inject ConversionService to this DeserializingConverter (as part of DI), instead of changing the API (interface)?
There was a problem hiding this comment.
If we want to do it in Deserializing only, can we then inject ConversionService to this DeserializingConverter (as part of DI), instead of changing the API (interface)?
We would have a circular dependency(ConversionService depends on DeserializingConverter which depends on ConversionService). Maybe it is possible now with lazy services features provided my modern dependency injection, but It requires at least some work in EcotoneLite container implementation.
| $converted = $converter->convert($source, $sourcePHPType, $sourceMediaType, $targetPHPType, $targetMediaType); | ||
| $convertedType = Type::createFromVariable($converted); | ||
| // Some converters (eg. DeserializingConverter) may return a value that is not compatible with the target type, | ||
| // so we try to convert it again, from the new media type and PHP type. | ||
| if (! $convertedType->isCompatibleWith($targetPHPType) && $targetMediaType->isCompatibleWith(MediaType::createApplicationXPHP())) { | ||
| $sourceMediaType = MediaType::createApplicationXPHPWithTypeParameter($convertedType->getTypeHint()); | ||
| $converter = $this->getConverter($convertedType, $sourceMediaType, $targetPHPType, $targetMediaType); | ||
| if (is_object($converter)) { | ||
| $converted = $converter->convert($converted, $convertedType, $sourceMediaType, $targetPHPType, $targetMediaType); |
There was a problem hiding this comment.
Here is the "double conversion" code, which was here only for deserializing converter
Why is this change proposed?
For conversion performance
Description of Changes
This PR will cache converter resolution
Pull Request Contribution Terms