association_proxy support

[dpep]

support for sqlalchemy AssociationProxy properties (https://docs.sqlalchemy.org/en/13/orm/extensions/associationproxy.html)

Fixes #136

[coveralls]

Coverage increased (+0.07%) to 97.647% when pulling 8079d6c on dpep:association_proxy-support into 421f8e4 on graphql-python:master.

[dpep]

@jnak : any initial thoughts on adding AssociationProxy support?

[erikwrede]

Just checked your PR out. It's a feature I could use as well! I know it's been some time, but since checks are passing, I'm going to test this a bit in the upcoming days and give you a review. Confident we can get this merged soon.

[erikwrede]

Just saw that I accidentally posted these comments instead of adding them to the review. I'm still busy with that and expect to have everything I have in mind tested by the end of the week!

[erikwrede]

It took me longer than expected, but the rest looks fine for basic feature integration. We should add Hybrid Property support before this gets merged.
Thanks for the great work; this is helpful! If you @dpep have the time to implement the additional features, I'd highly appreciate that. Otherwise, I will work on it myself once I find the time.

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d0668cc) 96.39% compared to head (7fd8d29) 96.46%.
Report is 1 commits behind head on master.

❗ Current head 7fd8d29 differs from pull request most recent head 3f50a4c. Consider uploading reports for the commit 3f50a4c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   96.39%   96.46%   +0.06%     
==========================================
  Files           9        9              
  Lines         915      933      +18     
==========================================
+ Hits          882      900      +18     
  Misses         33       33              

Files	Coverage Δ
graphene_sqlalchemy/converter.py	96.05% <100.00%> (+0.22%)	⬆️
graphene_sqlalchemy/types.py	93.56% <100.00%> (+0.11%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[erikwrede]

Quick update: using this in dev, but there seem to be problems regarding batching/dataloader, especially if the association proxy "skips" an intermediate class in a relationship and maps to another relationship with backrefs (A.b->B.c->C => A.c->C). Will need to investigate further once I find the time.

I don't want to merge this unless it is clear which problems exist and what existing features this addition is incompatible with.

[erikwrede]

Fixed the review comment I've had. The issue with dataloaders should be solved in a follow-up PR. We could easily create a optional custom resolver for the proxies that first calls the dataloader for the specific relationship, so no implicit load takes place upon access.

[jendrikjoe]

Generally like the change as it probably saves a lot of code. One thing we have to be cautious about is that when variables are proxied they might be proxied between nodes. This will eventually conflict with the caching relay and apollo do internally. Imagine the following classes:

class A:
    id = str()
    b = relationship(B)
    proxied_c = association_proxy(B.c)
class B:
    id = str()
    c = str()

If we now run the mutation:

    query = """mutation alterB($c: str!) {
            alterB(c: $c) {
                id
                c
            }
        }"""

the cache for B is update to contain the new value of c but the cache for a, which might have been queried before wouldn’t be updated. We just have to make sure to document this possible conflict, as it probably goes a bit against the patter of just having on root per property. What do you think? 🙂

[erikwrede]

Good idea to mention that in the docs, probably best to scope that to an additional PR. It's more of a frontend task to take care of caching, but we cannot expect all of our users to know all about the best practices and patterns in GQL upfront. Let's address this after the release of 3.0 @jendrikjoe 👍

[jendrikjoe]

Sounds reasonable 👍 The rest of the code is fine by me 👍

[conao3]

I now find association_proxy and I want to use it with graphene-sqlalchemy.
What status on this PR? There're conflict, we cannot merge this?

[erikwrede]

@conao3 It's on my list to get it done for this month. If you have time to test it, I'd really appreciate a review! ☺️

[conao3]

This patch is already cherrypicked own fork and it's ready to ship :)
This seems super good but one point, I have to address required argument but I think we can fix it as a single issue after the merge.

[erikwrede]

@conao3 how would you addrss the required argument?

[conao3]

I want to use this patch now, I haven't investigated it.

The issue I face is, the association_proxy column is always required = True, but my association_proxy was a proxy for relationship_ship, so it had to be required=False. I decided to always specify required=False for the time being to prevent graphene from generating an error.

[erikwrede]

@conao3 I just checked, and I can't reproduce that behavior. For me, association proxies are always Nullable. That issue might be related to you local configuration.

[conao3]

OK, thanks for letting me know!