Fix multiple JSON profiles issue in one model

[aragentum]

Fixed support for several JSON profiles in one model.

Done:

• fixed bug JSONProperties don't work correctly for multiple JSON profiles #693
• renamed __profile__ to __profiles__
• added tests

If you have any comments or suggestions, let me know :)

[aragentum]

@fantix I think we can't make the complexity any less.
Could you tell me how I can get into the authors of this project?

[wwwjfy]

Option 2: iterate every property of a model to find all JSON properties in get_profile.
Option 3: save all prop_name used during initialization and go through every one in get_profile.

The main difference is to keep __profile__ flat.

As for author of the project, you are the author contributing MR. If you mean to become a maintainer, I think we don't have a rule or something now, but I believe it can be made by regularly answering questions and fixing issues for some time.

[aragentum]

@wwwjfy thank you for your options and thank you for your work.

About your Option 2: I think it is not an optimal solution because get_profile called in many places and if we will search by all properties we can get performance problems then we will have a large number of objects/instances.

About your Option 3: Sorry, If I don't understand you correctly. We can add a separate place or we can add special property inside __profile__ which will be store information about initialized prop_names. But I think both these solutions make the code more complex.

[wwwjfy]

About your Option 2: I think it is not an optimal solution because get_profile called in many places and if we will search by all properties we can get performance problems then we will have a large number of objects/instances.

To clarify, with the current implementation, the search is done once, and __profile__ will be returned directly (well, it's also why we have this issue at the first place)

About your Option 3: Sorry, If I don't understand you correctly. We can add a separate place or we can add special property inside profile which will be store information about initialized prop_names. But I think both these solutions make the code more complex.

I mean when the table is initialized (in _init_table

gino/src/gino/declarative.py

Line 286 in c43b619

def _init_table(cls, sub_cls):

), we can know json properties and the corresponding columns. These info can be stored in a class attribute and here in get_profile we can use that information.

At this moment I lean to option 3.

Both should be relatively small changes. I could have a diff to show my idea if that's more clear.

[aragentum]

@wwwjfy thank you, I get your point - save to instance list of prop_names and in get_profile initialize all properties in one __profile__. Something like this (I added sub_cls._prop_names) (this is not final solution):

        sub_cls._prop_names = []
        rv = sa.Table(table_name, sub_cls.__metadata__, *args, **table_kw)
        for k, v in updates.items():
            setattr(sub_cls, k, v)
        for each_cls in sub_cls.__mro__[::-1]:
            for k, v in each_cls.__dict__.items():
                if isinstance(v, json_support.JSONProperty):
                    if v.prop_name not in sub_cls._prop_names:
                        sub_cls._prop_names.append(v.prop_name)
                    json_col = getattr(
                        sub_cls.__dict__.get(v.prop_name), "column", None
                    )
                    if not isinstance(json_col, sa.Column) or not isinstance(
                        json_col.type, sa.JSON
                    ):
                        raise AttributeError(
                            '{} "{}" requires a JSON[B] column "{}" '
                            "which is not found or has a wrong type.".format(
                                type(v).__name__, v.name, v.prop_name,
                            )
                        )
        return rv

[aragentum]

I will try to implement a new solution.

[aragentum]

@wwwjfy what do you think about this solution? Codacy said that complexity has increased :(

Could you tell me JSONProperties will have aliases in the future? Because in this case properties in different profiles can have the same names (in my option this is not problem).

[fantix]

Codacy said that complexity has increased :(

No worries, this is fine. :thisisfine:

[wwwjfy]

At least in major version 1, aliases are not a problem as the JSON properties are the same as "official" columns in the model classes. Same names can't happen.

Diff looks fine to me. @fantix please help to take a look too.

[fantix]

Looks good to me! Please also rebase to the latest master - sorry I think the conflict is caused by my recent fix.

[aragentum]

@fantix sorry, are you mean to make master rebase with removing the last merge commit Merge branch 'master' into bug/multiple-json-profiles?

[fantix]

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- src/gino/json_support.py  1
         

See the complete overview on Codacy

[fantix]

Thanks for working on this one!