-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[AutoMapper] New component to automatically map a source object to a target object #30248
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
Conversation
@ostrolucky Could you elaborate about your vote? May be you miss-clicked :) |
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. |
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? |
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.
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 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. |
There's a bundle for automapping actually: https://github.com/mark-gerarts/automapper-plus-bundle |
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 $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 😉 |
I don't see how are reasons given valid, which were:
Also, this overlaps domain of Serializer. What makes this different from deserialization? |
@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.
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.
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).
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.
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. |
One question, why not name it symfony/mapper? What exactly is meant “Auto”-matic here? |
"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) |
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.
|
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 |
@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. |
This is a step in the direction of using a common interface: symfony/symfony#30248
@joelwurtz Could add @symfony/deciders What do you think? IMHO this would be nice to get this in 4.3 |
Another for the "Existing Libraries" list is https://github.com/Seacommerce/php-mapper. |
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:
And, additionally
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 What are your thoughts on AOT validation? |
Hey @smoelker thanks for raising those questions:
Default implementation of the mapper rely on the The goal here is to blindly trust extractor implementation, so someone can propose its own extractor and remove properties or add virtual one.
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)
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:
Then there is the
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, I would love to see this component in 4.3. It will help improving Serializer's performance.
$hash = $mapperGeneratorMetadata->getHash(); | ||
$registry = $this->getRegistry(); | ||
|
||
if (!isset($registry[$className]) || $registry[$className] !== $hash || !file_exists($classPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we use the existing Symfony file tracking infrastructure instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with that system, can you give me some hint where i can find other code using this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @dunglas
src/Symfony/Component/AutoMapper/MapperGeneratorMetadataFactory.php
Outdated
Show resolved
Hide resolved
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 |
I don't mind having it named @dunglas thanks for you review, about some classes that need to be moved into their own components, i see some options:
Let me know which option do you prefer. Also, This component is using It is totally possible to not use this library and only use |
b60df86
to
08407c0
Compare
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. |
1a0cea3
to
5ba8171
Compare
We've just moved away from |
Personal note about this PR (ref #51741) I made this PR a while back, there was some interest but maybe not enough at this time ? Since there seems to be no interest for such component i let the PR died, for the record : it was fully ready it needed review / discussion with core contributors certainly but there was not such discussions so i did not go further and we went ahead having this as an external library. If such a component is needed (as said by some core contributors) why not contacting people that have already tried to do this ? a friendly ping ? a dm on slack ? |
@joelwurtz Sorry for the lack of communication. The new proposal was completely unplanned and emerged from discussions following @weaverryan's talk at API Platform Conference and the new SymfonyCasts micro-mapper library he revealed during it. I'll not speak for him, but I think that following these in-person discussions, @soyuka just created a Proof of Concept in the train (which has a totally different design than this PR) and opened a PR to discuss it, which is, in my opinion, a sane practice encouraging collaboration (everybody can review, discuss, improve, criticize...). Regarding this PR, it was closed 2 years ago and the need is still there. Personally, as we discussed again with @Korbeil at SymfonyLive Paris, I've always been in favor of having an automapper in Symfony, and, as demonstrated at API Platform Con, there is still a very strong need for such a component in Symfony. I didn't review thoroughly this PR (that's why I didn't formally "approve" it, even if I voted with a positive emoji), but I was for its merge when ready. I must admit that it's easier for me to find the time to review proposals of 500 lines of code than ones of thousands (it's also why I would love if #51718 could be more focused and... small :D). And it seems to be the same for most of the core contributors (including your colleagues). Now we have several competing options (and that's great):
In any case, we now have until 7.1 to confront, improve, and maybe merge proposals (if beneficial) and I'm sure that we'll finally get fast and easy-to-use new components in the end. |
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
In PHP libraries and application mapping from one object to another is fairly common:
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):
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:
Context
Context object allow to pass options for the mapping:
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:
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:
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):
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:
Future consideration
Things that could be done but not in this PR:
PHP 7.4 may give a problem to the symfony/validator component where typed properties can be problem, given a class like this:
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:
Feel free to challenge as much as possible