Pass relationship and registry objects to connection_field_factory

[jnak]

Hello,

The createConnectionField currently only expects the type of the connection. I found this to be limiting since there are valid cases where connection factories depend on other attributes of the relationship object.

For example, in my case I need the connection_factory to inspect the relationship object to extract the JoinCondition object from SqlAlchemy. This allows us to auto-generate performant sql-based connections for SqlAlchamy relationships that only fetch the relevant page of data from the database ¹.

Given convert_sqlalchemy_relationship takes both a relationship and a registry objects, I think it makes sense to have the factory take the same parameters. That said, I think it would be more future proof to also pass the model object passed in construct_fields.

Thank you!
J

_
¹ I will send a PR in the next few weeks to see if there is any interest to share this here.

[jnak]

The current pattern to register a factory is to use a global module variable. I followed it in this PR but I think we should move away from it because:

• unless every module that defines a subclass of SQLAlchemyObjectType explicitly calls register_connection_field_factory, you may not get the expected connection when you import a SQLAlchemyObjectType subclass.
• it is not possible to use different factories within the same schema.

Instead I would prefer to explicitly define the factory on the Meta class of SQLAlchemyObjectType:

class MyType(SQLAlchemyObjectType):
     class Meta:
         connection_field_factory = ...
         ...

This behavior could easily be shared within an app by putting this in an abstract meta class. Let me know if you agree with this change and I'll update this PR.

[Nabellaleen]

Clearly it seems more "standard" to implement it as meta parameter

But think also to retro-compatibility if you change the behavior (with deprecated warnings as you wrote for register_connection_field_factory)
Maybe you could do it in another PR, because this is a different subject

[jnak]

My worry is that if we merge this as is now we'll have to support 2 deprecated methods (register_connection_field_factory and registerConnectionFieldFactory) with different interfaces. In my opinion it would easier / cleaner to fix everything at once if you also think that 'meta' is the way to go. What do you think?

[Nabellaleen]

Clearly, I think the "meta" approach is the right way.

My point is just : if you implement this with the "meta" way, do you still keep a "retro-compatibility" on the old registerConnectionFieldFactory ? I think it's important

(@Cito seems also agree with the meta way, with his 👍 ;))

[Nabellaleen]

Clearly it seems more "standard" to implement it as meta parameter

But think also to retro-compatibility if you change the behavior (with deprecated warnings as you wrote for register_connection_field_factory)
Maybe you could do it in another PR, because this is a different subject

[coveralls]

Coverage increased (+1.02%) to 92.916% when pulling b97bbfb on jnak:connection-factory into c9af40c on graphql-python:master.

[jnak]

@Nabellaleen I made the changes we discussed. Would you mind taking a look at it? Thanks