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

Consider sharing implementation with graphene-django #202

Open
thejcannon opened this issue Apr 5, 2019 · 7 comments
Open

Consider sharing implementation with graphene-django #202

thejcannon opened this issue Apr 5, 2019 · 7 comments

Comments

@thejcannon
Copy link

thejcannon commented Apr 5, 2019

Problem

I was surprised to see that graphene-django shares a considerable amount of code with this library. Almost line-for-line in some instances if you blur your eyes to the types. Unfortunately that means it also shares some common bugs, missing features, etc...

Worse-off for this library, the graphene "team" seems to prefer graphene-django by a 3-0 vote. Which might explain why that package has additional features over this one 😉. Not to mention, there also several graphene-django-* packages that tack on to grpahene-django[1]

Both libraries essentially represent the same thing: gluing your Python SQL ORM into graphene types. (Note that these aren't the only players on the field either [2])

Proposal

Find the middle ground for "here's how to take a declarative ORM and transform it into graphene types", and have graphene-django and graphene-sqlalchemy fill in the ORM-specific slots (how do I convert this ORM field type to a graphene field type?, etc...) and add additional features offered by that ORM.

This means that contributors who can bring valuable graphene expertise can contribute without heavy knowledge of an ORM, and aren't splintered across repos. Valuable features can be added to a single scaffold repo and then be filled by the implementation-specific repos much easier.
It would also mean core features/issues are located in one library.

I've outlined below (what I think) the key features that a shared library should attempt to solve, in addition to reducing the amount code copied between the two libraries.

Similar issues/features

Description django issue sqlalchemy issue
don't overfetch data 402 134
field/node level permissions 485 186
filtering supported 110, 16
N+1 graphene-django-optimizer 35

(there's more issues/features duplicated between the two, but those are the "big ones" IMO that stop these libraries form being true works of art)

drawbacks

With maintenance being scattered, it might be hard to find the right people to create/maintain such a library. Furthermore, maintenance/contributions would need to generic enough to satisfy several downstream libraries. However, from what I've seen scouring the issues and related libraries, there's no shortage of people willing to solve the hard problems. 👍

notable parties

@syrusakbary @jnak @Nabellaleen @Cito @patrick91 (plus whoever else y'all wanna tag)

[1] graphene-django-tools, graphene-django-optimizer, etc...
[2] @coleifer who authored Peewee showed interest in support (graphene#289) but recently said it wasn't being pursued (peewee#1790)

@thejcannon
Copy link
Author

I'd be happy to attempt a proof-of-concept that covers a good chunk of each library and shows how each would look leveraging the shared scaffolding, if y'all are interested.

@coleifer
Copy link

coleifer commented Apr 6, 2019

I'd like to see support for peewee, but I can't commit to doing the implementation or design myself right now :(

@ProjectCheshire
Copy link
Member

@thejcannon the graphql-python team expanded by quite a bit recently, but Django does have the larger community. (the docs are having an issue we're trying to wrangle)

As someone who uses aiohttp, and on neo4j as my ogm (graph db) I'd also like to see a generic interface / almost an abstract base class implementation. I started down a similar path of copy pasta coding from Django and here and converting to my specifuc graph needs in the same vein.

I don't think I can commit to being principle on this, because I don't have the traditional orm background, but I could certainly speak to / help around the abstracts

@jnak
Copy link
Collaborator

jnak commented Apr 6, 2019

@thejcannon I think it's definitely an interesting route to explore. I have 2 main concerns with that approach:

  1. It may prevent each library from building on the strengths or the specificities of each ORM. Obviously I'm biased here, but my understanding is that Django ORM is not nearly as flexible as SQLAlchemy. If we were to use a shared layer like you describe, I would want to make sure that we are not losing any of the power offered by SQLAlchemy.

  2. We're adding another abstraction layer that may result in making it harder to maintain and add new features. Unlike what you said, I think there is a risk that to update or refactor the shared library people will need intimate knowledge of both libraries.

My perspective on all this comes from the fact that we've already solved a lot of the problems you listed at my company. Btw we are committed to contributing most of those features back into this repo over the next few weeks / months.

That said, if you can somehow find a solution that addresses well my 2 concerns, I would love to see it and be convinced otherwise

@Nabellaleen
Copy link
Collaborator

I share both point of views, and the "right" solution is not clear for the moment, because of what @ProjectCheshire said about the recent team expansion.

There are similar discussion in the django repository, so a global analysis is needed. We had doubt about the quantity of code which is really shared between the library. You seems to have identified an important quantity of duplicated code, so it's going in the way of the https://github.com/graphql-python/graphql-server-core package.

We could try to start some new branches here and in graphql-server-core, to try new things, to really identify the patterns which we could factorize, and those who are linked to the "strengths" of django or sqlalchemy.
Django is a framework very very opinionated, which give it its strength, its success. So we should remain prudent as @jnak mentionned.

By the way, there are also a lot of moves into the graphql-core repository, which should have effects for all the other packages of the organization, so I think we should just "prospect", "understand" the pro and the cons, try things, and in some months, conclude and really build something answering to this problem :)

@ekampf
Copy link

ekampf commented Apr 10, 2019

IMHO I think graphql-server-core should be that place for shared code between integrations.

An even more radical idea I'm contemplating with is to actually move all integrations into graphql-server-core (under an integrations module?), manage dependencies via the setup.py extras, and maintain one release cycle and repo rather than a hoard of them... (especially as most of these are pretty small and share a lot of the code)

@Cito
Copy link
Member

Cito commented Apr 10, 2019

@ekampf - on the other hand it might also scare away people when there is too much going on in one project. If the project is focused on a single responsibility and easily testable (without needing to understand and install all the extra dependencies) the inhibition threshold for contributing is lower.

We're just trying to clean up graphql-server-core, document what exactly is the public API (actually only a few simple functions) and add tests for these. Then it's also much easier to push the next version that uses graphql-core-next instead of graphql-next.

I think it's also good to keep the integration with the backend/ORM separate from the integration with the webserver/WSGI. Django is somewhat special because it's a monolith, so it makes sense that GraphQL-Django contains both integrations, but most other web frameworks are more pluggable.

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

7 participants