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

Mapping is not lazy! #10

Open
kamyar1979 opened this issue Jun 13, 2018 · 6 comments
Open

Mapping is not lazy! #10

kamyar1979 opened this issue Jun 13, 2018 · 6 comments

Comments

@kamyar1979
Copy link

The map function loads all the attribute values at first, although I expect when I give mappings as lambda, the attribute values get read lazily, that is, if some attribute has been set to null or other function, the getattr of source object must not be called. The reason is that I am using your library for mapping SqlAlchemy objects to DTO ones. Deferred loading feature of SqlAlchemy causes Select N+1 problem when using your library.

@marazt
Copy link
Owner

marazt commented Sep 29, 2018

Hi. Sorry for the very late answer - I have absolutely forgot to answer. Lazy evaluation is a good point. I'll look at it. But if you have a PR... :)

@renanvieira
Copy link
Contributor

renanvieira commented Feb 17, 2019

After a few minutes looking at thecode and found the problem when getmembers is called the code access all properties and this cause SQLAlchemy to fire all the relationship load in the model, even using the excluded argument will not avoid the problem.

I came up with one solution and would like to discuss:

From this:

from_obj_attributes = getmembers(from_obj, lambda a: not isroutine(a))
from_obj_dict = {k: v for k, v in from_obj_attributes
if not_private(k) and not_excluded(k)}
to_obj_attributes = getmembers(inst, lambda a: not isroutine(a))
to_obj_dict = {k: v for k, v in to_obj_attributes if not_private(k)}

To this:

from_obj_dict = {k: v for k, v in from_obj.__dict__.items() if not_private(k) and not_excluded(k)}

to_obj_dict = {k: v for k, v in inst.__dict__.items() if not_private(k)}

https://github.com/renanvieira/object-mapper/blob/264a50eb976bf9cd91b5ed850600975188ba58d2/mapper/object_mapper.py#L137-L139

I tested this over SQLAlchemy models in my company project and work like a charm. But this changes breaks the test test_mapping_creation_with_custom_dir because the way the test create the class properties doesn't show in __dict__.

I was thinking maybe we can include a flag like sqlalchemy=True|False in the mapper __init__ or maybe another mapper called something like SQLAlchemyObjectMapper?

@marazt
Copy link
Owner

marazt commented Feb 24, 2019

Hi. I will check it and let you know. I hope that tomorrow. Thanks for the post 👍.

@renanvieira
Copy link
Contributor

renanvieira commented Feb 26, 2019

Thanks, Marazt! I'm really glad to contribute, this lib is helping a lot.

I just stumbled upon a problem on this solution, __dict__ does not list @property attributes, only dir() and getmembers().

I think it'll be better to create a module of ObjectMapper coupled with SQLAlchemy where we can check if one of them is a model and only in this case use __dict__ instead of getmembers()

@marazt
Copy link
Owner

marazt commented Feb 26, 2019

Thanks, Marazt! I'm really glad to contribute, this lib is helping a lot.

I just stumbled upon a problem on this solution, __dict__ does not list @property attributes, only dir() and getmembers().

I think it'll be better to create a module of ObjectMapper coupled with SQLAlchemy where we can check if one of them is a model and only in this case use __dict__ instead of getmembers()

Yes,

Such composition of ObjectMapper and SqlAlchemy make sense. Do you have prototype that can be updated to PR? I can start to work on it probably tomorrow.

@renanvieira
Copy link
Contributor

I'll try to work on a Pull Request in the next couple of days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants