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

[AutoMapper] New component to automatically map a source object to a target object #30248

Open
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
@joelwurtz
Copy link
Contributor

commented Feb 14, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17516 #22051
License MIT
Doc PR todo

This PR brings a new component to the Symfony. It's a follow up on the AST Generator Component that was done on #17516 which was lacking of real buisness value and only adding technical one. (It can also certainly replace #22051, but both can be merged without conflicts)

In this component code generation is only an implementation detail, however by its nature it achieves the goal of having ultra fast normalizers in Symfony.

Description

Taken from https://github.com/AutoMapper/AutoMapper

AutoMapper is a simple little library built to solve a deceptively complex problem - getting rid of code that mapped one object to another. This type of code is rather dreary and boring to write, so why not invent a tool to do it for us?

In PHP libraries and application mapping from one object to another is fairly common:

  • ObjectNormalizer / GetSetMethodNormalizer in symfony/serializer
  • Mapping request data to object in symfony/form
  • Hydrate object from sql results in doctrine
  • Migrating legacy data to new model
  • Mapping from database model to dto objects (API / CQRS / ...)
  • ...

The goal of this component is to offer an abstraction on top of this subject. For that goal it provides an unique interface (other code is only implementation detail):

interface AutoMapperInterface
{
    /**
     * Map data from to target.
     *
     * @param array|object        $source  Any data object, which may be an object or an array
     * @param string|array|object $target  To which type of data, or data, the source should be mapped
     * @param Context             $context Options mappers have access to
     *
     * @return array|object The mapped object
     */
    public function map($source, $target, Context $context = null);
}

The source is from where the data comes from, it can be either an array or an object.
The target is where the data should be mapped to, it can be either a string (representing a type: array or class name) or directly an array or object (in that case construction of the object is avoided).

Current implementation handle all of those possiblities at the exception of the mapping from a dynamic object (array / stdClass) to another dynamic object.

Usage

Someone who wants to map an object will only have to do this:

// With class name
$target = $automapper->map($source, Foo::class);
// With existing object
$target = new Foo();
$target = $automapper->map($source, $target);
// To an array
$target = $automapper->map($source, 'array');
// From an array
$source = ['a' => 'b'];
$target = $automapper->map($source, Foo::class);

Context

Context object allow to pass options for the mapping:

// Using context
$context = new Context();
$target = $automapper->map($source, Foo::class, $context);

// Groups (serializer annotation), will only map value that match those group in source and target
$context = new Context(['groupA', 'groupB']);
// Allowed attributes, will only map specific properties (exclude others), allow nesting for sub mapping like the serializer component
$context = new Context(null, ['propertyA', 'propertyB', 'foo' => ['fooPropertyA']]);
// Ignored attributes, exclude thos propreties include others
$context = new Context(null, null, ['propertyA', 'propertyB', 'foo' => ['fooPropertyA']]);
// Set circular reference limit
$context->setCircularReferenceLimit(2);
// Set circular reference handler
$context->setCircularReferenceHandler(function () { ... });

Other features

Private properties

This component map private properties (However this can deactivated).

Nested Mapping

This component map nested class when it's possible.

Circular Reference

Default circular reference implementation is to keep them during mapping, which means somethings like:

$foo = new Foo();
$foo->setFoo($foo);

$target = $this->automapper->map($foo, 'array');

will produce an array where the foo property will be a reference to the parent.

Having that allow using this component as a DeepCloning service by mapping to the same object:

$foo = new Foo();
$foo->setFoo($foo);

$deepClonedFoo = $this->automapper->map($foo, Foo::class);

Max Depth

This component understand the Max Depth Annotation of the Serializer component and will not map after it's reached.

Name Converter

Default implementation allows you to pass a Name Converter when converting to or from an array to change the property name used.

Discriminator Mapping

This component understand the Discriminiator Mapping Annotation of the Serializer component and should correctly handle construction of object when having inheritance.

Type casting

This component will try to correctly map scalar values (going from int to string, etc ...).

History

Initial code of this component was done in an expiremental library here: https://github.com/janephp/automapper

After some works and talks with @dunglas and @nicolas-grekas i propose this library as a new component for Symfony, which can be used by other components and could help in resolving some future bugs due to PHP 7.4 (more explanations below).

Implementation

Default implementation use code generation for mapping, it reads once the metadata needed to build the mapper then write PHP code, after this, no metadata reading or analysis is done, only the generated mapper is used.

This allow for very fast mapping, here is some benchmarks using the library where the code comes from (jane/automapper):

Benchmark on the component on serializer part only (code source here https://github.com/joelwurtz/ivory-serializer-benchmark/tree/symfony/automapper):

benchmark

Example of generated code

Normalizer Bridge

A normalizer Bridge is available where its goal is to be 100% feature compatible with the ObjectNormalizer of the symfony/serializer component. The goal of this bridge is not to replace the ObjectNormalizer but rather providing a very fast alternative.

As shown in the benchmark above, using this bridge leads up to more than 8x speed increase in normalization.

Existing PHP librairies on automapping

Has some nice features poke @mark-gerarts some API are based on your works and C# automapper, would be nice to have you here for your input.

Things that needs to be done

This PR should be mostly readly in terms of code and functionnalities for a first stage, however there is still some things to be done:

  • Tests, tests and tests: i did not copy the old tests as i want to rewrite them for this component (will be done in the next days)
  • Documentation: documentation explaining this new component (will be done once the main interface and usage is ok)

Future consideration

Things that could be done but not in this PR:

  • symfony/form bridge for mapping request data to object
  • symfony/framework-bundle integration
  • symfony/validator integration:

PHP 7.4 may give a problem to the symfony/validator component where typed properties can be problem, given a class like this:


class Foo {
    /** @Assert\NotNull() */
    public int $foo;
}

An user may send a null value (in a form by example or json), and PHP will raise an error before the validation, since the validation occurs on the mapped object.

This component can help resolving this case with the actual behavior:

  • Create a dummy class with the same properties as Foo but without type checking
  • Mapping user data to this dummy class (using the automapper component)
  • Validating this dummy class with the metadata from the Foo class
  • Mapping the dummy object to the foo class (using the automapper component)

Feel free to challenge as much as possible

@lyrixx

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@ostrolucky Could you elaborate about your vote? May be you miss-clicked :)

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

I just don't see any justification why creating new component instead of embracing existing one. Poster is aware there is plenty packages in this field already, why create new one? Core team can barely keep up even with issues and PRs in existing set of components.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Core-team can grow. But actually, that's an important part of adding a new component: who will maintain it in the long run? @joelwurtz we know you as a regular OSS contributor - not in Symfony itself but eg httpplug if I'm not wrong? What would be your stand on this topic?

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

On this subject, this library was developed under the jane organization which is sponsored by my company (jolicode.com) by giving me time for working on it.

It would be the same for this component (my company will give me time to contribute to this like it's already done for @lyrixx). I totally understand your concern and i'm really motivated to be a regular contributor on this component.

Poster is aware there is plenty packages in this field already, why create new one?

This is mainly because i think there is some values in using the AutoMapperInterface in other components of Symfony.

This implementation (compared to others) is really fast and also allow feature compatiblity with the ObjectNormalizer of Symfony and it would help us in some of our projects to boost their performances.

Also the proposed interface could be used by existing automapper librairies (that's why i ping author of automapper plus), so an user could switch its implementation.

@linaori

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

There's a bundle for automapping actually: https://github.com/mark-gerarts/automapper-plus-bundle

@zanbaldwin

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

I can see this being really useful for performance, especially in APIs which require the deserialization of request data and the serialization of responses (the main culprit for response times in our current project is denormalization).

The following may be completely unrelated and entirely out-of-scope, but after reading this PR my brain jumped to the following possibility and I would love to see some form of it come into Symfony 5:

Currently, the following code is pretty common:

// (ignoring possible exceptions thrown)
$requestData = $request->request->all();
$model = $serializer->deserialize($requestData, MyModel::class);
$validator->validate($model);

Whereas if the validator had the ability to generate constraint lists for the normalized version of objects, it would create a nice, performant data flow when combined with the AutoMapper. It would negate the need for the serializer altogether, and the form component could skip DTO's altogether while handling the strictly-typed class properties coming to PHP in 7.4.

$constraints = $validator->determineConstraintStructureForNormalizedObject(MyModel::class);
// Constraint list for this model can be cached during a custom warmup.
if ($validator->validate($requestData, $constraints)) {
    $object = $automapper->getMapper(MyModel::class)->map($requestData);
}

I've wanted this for a while and this seemed like the closest opportunity to rant about it 😉

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

I don't see how are reasons given valid, which were:

  • performance: Is it not possible to improve performance of existing components?
  • usage of interfaces in symfony components: I don't know about rule which forbids symfony components to implement 3rd party interfaces. IMO this is better solved with PSR. It's surprising there is still no attempt of Serializer PSR.
  • usage of interfaces in 3rd party components: As far as I see, people don't use SF interfaces in 3rd party components much even now

Also, this overlaps domain of Serializer. What makes this different from deserialization?

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@zanbaldwin Yes, it's also one of the reason i bring this component (see the last part about validation and 7.4 in my PR) to helps such case in validation. Not sure how this would be done in details but this component could definitely help.

@ostrolucky

  • performance: Is it not possible to improve performance of existing components?

Yes but it will always be limited by design, current normalizers need to check metadata (i do not speak about reading them, there is cache for that, speaking about code added to do that when metatdata X is equal to this).

Here metadata change the way the code is generated, so no need to even read them on subsequent requests.

Some prs where done to generated normalizers but they were never merged, as the ObjectNormalizer has a lot of features and it's really hard to keep BC compliance.

Here the proposed Bridge is offered as an alternative to the ObjectNormalizer, both could coexist, they have the same purpose but differents implementations and may have in the long term different features also. In my view i would see this Bridge as an normalizer focused on performance rather than features, where the object normalizer could be one focused on features and less on performance.

Speed of ObjectNormalizer is really fine when normalizing small to medium result set, however on large data set it's really one of the main blocker.

  • usage of interfaces in symfony components: I don't know about rule which forbids symfony components to implement 3rd party interfaces. IMO this is better solved with PSR. It's surprising there is still no attempt of Serializer PSR.

This is more about having the possibilites to implement needed thing if using an automapper in the symfony/form or symfony/validator component. As it if miss something then making a PR that modify both component allow for better workflow in contribution (syncing thing is hard).

  • usage of interfaces in 3rd party components: As far as I see, people don't use SF interfaces in 3rd party components much even now

I agree this is very rare, and IMO that's why https://github.com/symfony/symfony/tree/master/src/Symfony/Contracts was done. This interface could be part of it so it would allow to require it without having all code about this implementation.

Also, this overlaps domain of Serializer. What makes this different from deserialization?

Don't agree here, serialiazing and mapping are 2 differents problems.

It may overlap on the normalization, since both can do object to array, and array to object convertion.

But this component is not about encoding to json or other formats. Neither is serializer component about mapping between two objects (although this may be done by using an array as a proxy data).

If we want to be perfect, (but will not be since there is BC to keep in mind), serializer component should use this component for its default implementation.

@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

One question, why not name it symfony/mapper? What exactly is meant “Auto”-matic here?

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

"Auto" part is for automatically mapping properties without having to write a custom config.

This name also comes from the popular C# https://github.com/AutoMapper/AutoMapper which is where the original idea comes from.

But i don't mind having it named only "Symfony/Mapper" or whatever suits better.

Also updated PR with new benchmark on the component itself (8x performance compared to ObjectNormalizer)

@mark-gerarts

This comment has been minimized.

Copy link

commented Feb 15, 2019

Thank you for the mention @joelwurtz. I've been following the development of https://github.com/janephp/automapper for a while now. It's obvious that a lot of work went into this, and it shows (especially performance-wise). I'll list my thoughts here in no particular order.

  • I'm obviously a bit biased, but I would love to see this as a component. I tend to make use of (auto)mapping quite a lot, and having a standardised, high-quality library with great performance would be a nice thing.
  • While there are some overlaps with the serializer, I have to agree with @joelwurtz that they're dealing with two different problems.
  • I'd be more than happy to implement a common interface, even if this PR doesn't land. The ability to change implementations would be a good thing.
  • In addition to @OskarStark's question, I originally named my library as a homage to .NET's AutoMapper, since a great deal of my inspiration was taken from there.
@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

Just add an option to disallow attribute checking (it removes support for allowed and ignored attributes, something that IMO is not used everywhere, however it's really useful on normalizer when doing graphql by exemple).

When disabling this check it offers a 15% performance gain (compared to activating this option), and 11x gain compared to ObjectNormalizer

deepinscreenshot_select-area_20190215113512

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

@mark-gerarts Thanks you for coming in and for your view,

As for the common interface, it we want to go that way this is something that need to be added in symfony/contracts IMO, but waiting for core contributors view on this subject before going any further.

mark-gerarts added a commit to mark-gerarts/automapper-plus that referenced this pull request Mar 17, 2019

Update the AutoMapper `map` behaviour
This is a step in the direction of using a common interface:
symfony/symfony#30248
@lyrixx

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@joelwurtz Could add @experimental ?

@symfony/deciders What do you think? IMHO this would be nice to get this in 4.3

@pbowyer

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

Another for the "Existing Libraries" list is https://github.com/Seacommerce/php-mapper.

@smoelker

This comment has been minimized.

Copy link

commented Mar 24, 2019

I really like to see a mapper component being added to the Symfony framework.

We created the seacommerce/mapper library (thanks for the reference @pbowyer) in an attempt to combine the power of the jane/automapper library and the interface of mark-gerarts library plus one additional feature that I call "validation".

Validation is an early( AOT) warning system for invalid mapping configurations and should check at least for:

  • Existence of configured properties.
  • Completeness of the configuration. In other words, are all target properties mapped (either automatically or manually) or otherwise explicitly ignored.

And, additionally

  • Check for property type incompatibility paired with the option to register value converters that can be reused over configurations.

It's very important to me to have validation because our systems (like most systems) are under constant development (adding properties, removing properties, refactoring, new mappings, etc). In PHP, there is no language construct to reference properties (like C#'s nameof) and therefore we can only use string constants to configure mappings which is not refactor friendly. It's very easy to miss a property or make a typo and this can only be detected at runtime (whether or not using unit tests).

What are your thoughts on AOT validation?

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Hey @smoelker thanks for raising those questions:

Existence of configured properties.

Default implementation of the mapper rely on the symfony/property-info component to map properties between objects, so properties should exist by default, but really depends on the extractor implementation.

The goal here is to blindly trust extractor implementation, so someone can propose its own extractor and remove properties or add virtual one.

Completeness of the configuration. In other words, are all target properties mapped (either automatically or manually) or otherwise explicitly ignored.

It's not checked but could be added as an option or different implementation in a future PR, as you point out this can make sense in some configurations to ensure that each target property has been correctly handled.

Also this is only needed between two objects (as from / to array mapping always have all properties); and, this is something that should be not so hard, mapping properties between objects is done here: https://github.com/symfony/symfony/pull/30248/files#diff-5c94bcb9eaccdbd9585dcd26e7a924a0R42

But would need the possibility to add ignored property also (which should be done in the property info extractor, so other components can profit of it)

Check for property type incompatibility paired with the option to register value converters that can be reused over configurations.

ATM there is a lot of work to correctly map a specific type to another one, current implementation has been made internal as a start but can be added into the public API.

For each property we have:

  • A list of types for the input
  • A list of types for the output

Then there is the TransformerFactoryInterface https://github.com/symfony/symfony/pull/30248/files#diff-102cf9c93dd80aec0f645a4af77a750a which has the goal to give the correct TransformerInterface for this option

Some helpers are also present for the factory and transformer to only deal with a single input / output type.

So someone can add it's own pair of TransformerFactoryInterface and TransformerInterface in order to handle a specific value converter

@dunglas
Copy link
Member

left a comment

Good job, I would love to see this component in 4.3. It will help improving Serializer's performance.

Show resolved Hide resolved src/Symfony/Component/AutoMapper/AutoMapper.php Outdated
Show resolved Hide resolved src/Symfony/Component/AutoMapper/AutoMapper.php Outdated
Show resolved Hide resolved src/Symfony/Component/AutoMapper/AutoMapper.php Outdated
Show resolved Hide resolved src/Symfony/Component/AutoMapper/AutoMapper.php Outdated
/**
* {@inheritdoc}
*/
public function register(MapperGeneratorMetadataInterface $metadata): void

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 25, 2019

Member

Why not passing the list of metadata in the constructor, it will make this object immutable, then easier to maintain and debug.

This comment has been minimized.

Copy link
@joelwurtz

joelwurtz Mar 25, 2019

Author Contributor

For circular loop problem, as some mapping may be able to create sub mapping on the fly if needed, and this can only be done when extracting metadata / generating the code.

Show resolved Hide resolved src/Symfony/Component/AutoMapper/Loader/EvalLoader.php Outdated
Show resolved Hide resolved src/Symfony/Component/AutoMapper/Loader/EvalLoader.php Outdated
$hash = $mapperGeneratorMetadata->getHash();
$registry = $this->getRegistry();
if (!isset($registry[$className]) || $registry[$className] !== $hash || !file_exists($classPath)) {

This comment has been minimized.

Copy link
@dunglas

dunglas Mar 25, 2019

Member

Couldn't we use the existing Symfony file tracking infrastructure instead?

This comment has been minimized.

Copy link
@joelwurtz

joelwurtz Mar 25, 2019

Author Contributor

I'm not familiar with that system, can you give me some hint where i can find other code using this ?

Show resolved Hide resolved src/Symfony/Component/AutoMapper/MapperGeneratorMetadataFactory.php Outdated
Show resolved Hide resolved src/Symfony/Component/AutoMapper/MapperMetadata.php Outdated
@lsmith77

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

I also think that having some sort of mapper solution would be a useful addition.

A minor but potentially important thing: I am wondering about is the name. From my understanding "auto mapper" is mostly derived from the .Net/C# auto mapper lib, which is the source inspiration. Do we want to stick to the auto in there? Unless this has become a defacto lingo for such mappers, I wonder how many users benefit from this prefix and how much confusion it creates where people just expect it to be some automatic solution (while it is somewhat automatic, in many situations the mapping needs in-depth configuration)

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

A minor but potentially important thing: I am wondering about is the name. From my understanding "auto mapper" is mostly derived from the .Net/C# auto mapper lib, which is the source inspiration. Do we want to stick to the auto in there? Unless this has become a defacto lingo for such mappers, I wonder how many users benefit from this prefix and how much confusion it creates where people just expect it to be some automatic solution (while it is somewhat automatic, in many situations the mapping needs in-depth configuration)

I don't mind having it named symfony/mapper, but i like also the auto as the goal is to do the less configuration possible (and be as much automatic as we can).

@dunglas thanks for you review, about some classes that need to be moved into their own components, i see some options:

  • Do the change in this PR
  • Do the change in another PR before this get (potentially) merged
  • Do the change in another PR after this get (potentially) merged

Let me know which option do you prefer.

Also, This component is using nikic/php-parser for generating code. Although i prefer to use this library to generate code, i also understand this is mainly a subjective point of view (and objectively there is arguments for pro / con).

It is totally possible to not use this library and only use string as i know some people are not comfortable using this library for code generation, please let me know if we should keep this dependency or not.

joelwurtz added some commits Feb 14, 2019

joelwurtz and others added some commits Feb 15, 2019

Apply suggestions from @dunglas code review
Co-Authored-By: joelwurtz <jwurtz@jolicode.com>

@joelwurtz joelwurtz force-pushed the joelwurtz:symfony/automapper branch from 2217a71 to 595b67f Mar 25, 2019

joelwurtz added some commits Mar 25, 2019

@joelwurtz joelwurtz force-pushed the joelwurtz:symfony/automapper branch from b60df86 to 08407c0 Mar 26, 2019

joelwurtz added some commits Mar 26, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

about some classes that need to be moved into their own components, i see some options:

Moving existing classes should be discussed in another PR, but classes introduced here should be moved in this PR to avoid the need for deprecating them later.

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Since all the classes introduced here are either internal or expiremental do we really need to deprecate them later if this gets merged ?

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Just created #30704 and #30706 that extract some concepts in this pr into their rightful components

As for the NormalizerBridge i can remove it from this PR and create a new PR if this gets merged.
Will update this PR if / when relative PR are merged.

fabpot added a commit that referenced this pull request Mar 27, 2019

feature #30706 [PropertyInfo] Add possibility to extract private and …
…protected properties in reflection extractor (joelwurtz)

This PR was squashed before being merged into the 4.3-dev branch (closes #30706).

Discussion
----------

[PropertyInfo] Add possibility to extract private and protected properties in reflection extractor

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30248
| License       | MIT
| Doc PR        | TODO

This PR add the possibility to extract private and protected properties from a class by passing a new argument to the `ReflectionExtractor`

This new argument consist of flag that filters properties, so someone will also be able to use the `ReflectionExtractor` only for private property

```php
new ReflectionExtractor(null, null, null, true, ReflectionExtractor::ALLOW_PRIVATE | ReflectionExtractor::ALLOW_PROTECTED)
```

Flags method was prefered over a list of bool to avoid too many parameters and also be close to the reflection API of PHP

Commits
-------

05e487f [PropertyInfo] Add possibility to extract private and protected properties in reflection extractor
@jderusse

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Hello, I hope, I'm not too late on that topic. We've implemented a similar tool (that only denormalize) focused on performance, regarding the generated code (https://gist.github.com/joelwurtz/7ee48dd768f6d39ccc78d6ab7bdea22a), here are optimization I notices:

Each time you denormalize a UserDto you call context->isAllowedAttribute(...). Meaning, that if you denormalize an array with 1000 users (with the same context mostly initialized by the mappingGroups), you'll call the same method for each entity.

In our tool, we've use dump a cached code that vary on the contextOption. Even if the combinaison is huge, the users will probably reuse always the sames options. Without method (as less as possible) to focus on performances.

basically it do (here is a simplified view of the output. Many case are handled ($value could not be an array, constructor with argument, value null or key missing, ...):

// cache/user_XXXHashFromMappingGroupXXX.php
$result = new UserDto();

$result->id = $value['id'];

$result->address = (function ($value) {return include('cache/adress_XXXHashFromMappingGroupXXX.php');}($value['address']);

return $result;
@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Each time you denormalize a UserDto you call context->isAllowedAttribute(...). Meaning, that if you denormalize an array with 1000 users (with the same context mostly initialized by the mappingGroups), you'll call the same method for each entity.

Yes and also it's by property, so if you have 10 properties you will have 10000 calls to the allowedAttribute. FYI groups are not included in this, and a specific condition is created for that, but it's rouglhy the same.

I have think about this condition for a while, given it has a huge performant impact and i would like in the future do the following optimisations:

  • Add the ability to remove this feature from generated code, it's already done for allowed / ignored attributes and can also be done for groups, there is a lot of use case when you don't need groups attribute, like when using specific DTO, so an user will create a specific DTO for each of its view, and can use this as a proxy for other mapping: I really like this possibility, as the most performant code is the one that don't exist...

  • Using global condition / filtering: Instead of checking groups / allowed / ignored flags property by property, this could be done on a upper level and we could use the result of this condition to only set / get value from specific properties. And if this filtering has too much cost we can consider adding a cache for it which vary on the context.

However i'm not a fan of producing code depending on the context, as for me this component should be able to work in read only mode, and so be able to write all possible mapping paths when warming up the cache.

This means that, if we do code depending on the context, we will have to create a matrice for code generation, for each possible scenario and i fear that this is a O(n) (or even more) problem that can lead to serious issues when warming up the cache.

@torinaki

This comment has been minimized.

Copy link

commented May 21, 2019

@joelwurtz could you please update on the status of automapper component development?

@tsantos84

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Btw, the library TSantos listed on your screenshots uses the same technique to generate hydrators/extractors for PHP objects. In terms of performance, the doesn't make sense to "guess" type and accessor/mutator at runtime. Generated code is the way to follow.

Why not symfony/hydrator?

@joelwurtz

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@torinaki About the current status i still want to maintain this, got less time this last month beteween holidays and stuff. But definitely want to push this for 4.4 / 5.0. However there is dependent PR and work on serializer that need to be done before that (which i intend to work in the next weeks / months).

@tsantos84 Yes i see it use the same technique, and for the diff in benchmarks from what i have seen it comes mainly from date handling (format / parsing) that is not present in the tsantos library.

Why not symfony/hydrator?

Don't really mind about the name, they all represent the same thing, but i prefer functional name over technical one, and for me mapping is the right one, hydrating is only an implementation detail.

@tsantos84

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Don't really mind about the name, they all represent the same thing, but i prefer functional name over technical one, and for me mapping is the right one, hydrating is only an implementation detail.

I think that mappers are a way to instruct some tool how hydration/extraction should work. For example, Doctrine has a mapping that we describe how the data should be hydrated/extracted to/from objects. Mapping doesn't care about data and should be just a descriptive mechanism, IMHO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.