feature: Add possibility to map nested attributes #29
Conversation
Saurus119
commented
Mar 5, 2025
Saurus119
commented
- Added possililbity to map chained object reference easily when we create map between source and dest.
|
Hi @Saurus119 |
README.md
Outdated
| mapper.add( | ||
| AdvancedUser, BasicUser, fields_mapping={ | ||
| "name": "AdvancedUser.user.name", | ||
| "city": "AdvancedUser.user.city", |
There was a problem hiding this comment.
The idea is good. Here are few things to consider:
-
If we know that the mapping is going to be from source class object, isn't name of the source class redundant?
For instance, you can just say:"name": ".user.name"or"name": "$.user.name"where.or$.would mean that the value is mapped from the subfield of a source class. -
The problem with string literal, is that there is no way to tell if
"AdvancedUser.user.name"is a reference path to a source subfield or it is just a string literal. Meaning, what if someone wants thenamefield to have string literal valueAdvancedUser.user.name, and not the subfield of a source object. This would be a unique scenario but not impossible.
There are couple of ways to solve the latter problem:
- (Preferred) Wrap the path in some sort of wrapper function to signify that this is a subfield path and not a string literal:
"name": mapper.path("user.name"), # where mapper.path would return an instance of MapPath wrapper class
# or just use MapPath class directly
"name": MapPath("user.name")- Have a different parameter for subfield mapping:
fields_mapping={
"name": "just a string literal"
}, src_path={
"city": "user.city" # path to source object subfield
}Solution (1) is preferrer because it will not break any existing usage of the library, where second approach modified the method signature and can break existing usage.
There was a problem hiding this comment.
I like the idea with MapPath another object that is specific for this scenario but at the first run I didn´t want to "overkill" the implementation but you are right with literal strings. I will try to adress till end of the weekend.
I was also considering this modifications:
- Change the register map from simple dict into MapperRegistry new class as this give opporunity to map one Src class to Multiple Dst if the field_mappings id different
- I didn´t see usecase where you are able map Iterables, so maybe add support for this as well as I code also in C# and I like the approach like:
users= _mapper.Map<List<User>>(userEntities)which give me opportinity to map from my "DB" users to my for example data transfer objects easily with one line. And I am able to access Src and Dst object and thier property by reference and not the strings, example:
CreateMap<SrcObject, DstObject>()
.ForMember(dest => dest.Duration, opt => opt.MapFrom(dest.DurationSeconds))
.ReverseMap();
Where ReverseMap means 1:1 mapping so object should have same fields. So after this function is called you just to same mapping as you have with map function,
But this two ideas will bring lot of code changes.
anikolaienko
changed the title
There was a problem hiding this comment.
Great work! Added some comments, would like to hear your opinion.
Please be aware I've release version 2.2.0, very few minor changes, should not affect you, but recommend get the latest changes from main branch.
Also approved Github Actions on your branch. Now you should see automatic code check results and pytest results. I'm sure you are using pre-commit but for some reason mypy is failing on CI/CD. If you don't have pre-commit locally enabled for this repo, I recommend: https://pre-commit.com/
| return self.attributes | ||
|
|
||
| def __repr__(self): | ||
| return f"MapPath({self.attributes})" |
There was a problem hiding this comment.
This looks a bit complex. I honestly thought this would be just:
@dataclass
class MapPath:
path: strand in the file named map_path.py
| T = TypeVar("T") | ||
| ClassifierFunction = Callable[[Type[T]], bool] | ||
| SpecFunction = Callable[[Type[T]], Iterable[str]] | ||
| FieldsMap = Optional[Dict[str | MapPath, Any]] |
There was a problem hiding this comment.
I have enabled Github Actions to run checks on your branch and it seems to be failing.
Maybe makes sense for now to put these back as these are only used in 1 file. Not very confident in these TypeVar types that I've created a while ago. Need to refresh my memory.
| f"Invalid path: '{path}'. Can´t reference to object attribute." | ||
| ) | ||
|
|
||
| self._obj_prefix: str | None = None |
There was a problem hiding this comment.
Is . really required? what if user just wants to map to source object property with different name?
E.g.
fields_mapping={
"name": MapPath("user_name") # where user_name is just a property in source object.
}| if not all(isinstance(map_path, MapPath) for map_path in map_paths): | ||
| raise MapPathMissMatchError( | ||
| "It is not allowed to mix MapPath mappings with string mappings." | ||
| ) |
| ): | ||
| common_fields_mapping[target_obj_field] = self._rgetter( | ||
| obj, source_field[len(obj_type_prefix) :] | ||
| ) |
There was a problem hiding this comment.
here I would just leave getattr, as it's simple and easy to understand.
| elif isinstance(source_field, MapPath): | ||
| common_fields_mapping[target_obj_field] = self._rgetter( | ||
| obj, source_field | ||
| ) |
There was a problem hiding this comment.
then here we know that we need to use recursive getter.
Could be also resolve_map_path or get_by_map_path or something similar, little bit more descriptive.
You'll know that you pass in a source_field of type MapPath and this function will get you a value using this MapPath.
If the value not possible to retrieve, I would suggest this function raises an InvalidMapPathError. So this is something user may expect from our package. Currently the reduce function in _rgetter would fail with unknown error.
