Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Support for multiple keys #12

Merged
merged 9 commits into from
Nov 2, 2019
Merged

Conversation

jonnii
Copy link
Contributor

@jonnii jonnii commented Oct 8, 2019

No description provided.

@jonnii jonnii mentioned this pull request Oct 8, 2019
@jonnii
Copy link
Contributor Author

jonnii commented Oct 16, 2019

One more thing we noticed was that the @Key is not converted from snake_case to camelCase which causes issues if you have a key like my_magic_key.

Copy link
Collaborator

@erebus1 erebus1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!
Nice feature!

Please check several comments

Feel free to let me know if you need any help with implementation

integration_tests/tests/tests/test_main.py Outdated Show resolved Hide resolved
integration_tests/service_b/src/schema.py Outdated Show resolved Hide resolved
@@ -20,9 +20,11 @@ def build_schema(query, mutation=None, **kwargs):
return graphene.Schema(query=_get_query(schema, query), mutation=mutation, **kwargs)


def key(fields: str):
def key(fields: str, *args: str):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the more explicit and consistent way would be to use new decorator for each primary key
as in federation docs
https://www.apollographql.com/docs/apollo-server/federation/advanced-features/#multiple-primary-keys

type Product @key(fields: "upc") @key(fields: "sku") {
  upc: String!
  sku: String!
  price: String
}

otherwise, we should make breaking changes and set fields as args:
def key(*fields: str)
Cause having *args together with fields is a bit confusing

integration_tests/service_a/src/schema.py Outdated Show resolved Hide resolved
integration_tests/service_a/src/schema.py Outdated Show resolved Hide resolved
integration_tests/service_b/src/schema.py Outdated Show resolved Hide resolved
integration_tests/service_b/src/schema.py Outdated Show resolved Hide resolved
integration_tests/service_b/src/schema.py Outdated Show resolved Hide resolved
@@ -12,11 +12,17 @@ class FunnyText(ObjectType):
id = external(Int(required=True))


@extend(fields='name')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add @extend(fields='id') to another node, to test multiple primary keys logic

integration_tests/service_a/src/schema.py Outdated Show resolved Hide resolved
@erebus1
Copy link
Collaborator

erebus1 commented Oct 16, 2019

One more thing we noticed was that the @Key is not converted from snake_case to camelCase which causes issues if you have a key like my_magic_key.

Nice catch!
I think it worth creating a new issue

BTW, as a solution, you can just specify fields in camelCase
so if you have my_magic_key
Just use @key(fields='myMagicKey')
Assuming the fact that fields of type _FieldSet it can be tricky to parse and reformat snake in camelCase. And I'm not sure that it worth to auto-convert it ...

Anyway we can discuss it in separate issue

@jonnii
Copy link
Contributor Author

jonnii commented Oct 17, 2019

Just use @key(fields='myMagicKey')

this didn't work for me because it's trying to call the resolver with this name and failing, will address feedback shortly.

@jonnii jonnii force-pushed the support-for-multiple-keys branch from 37bea5b to 75a3c43 Compare October 29, 2019 21:30
@jonnii
Copy link
Contributor Author

jonnii commented Oct 29, 2019

@erebus1 I think this all good to go now, let me know if you have any more feedback.

@erebus1
Copy link
Collaborator

erebus1 commented Nov 2, 2019

Cool!
I'll check it & merge today

@erebus1 erebus1 merged commit d40ba1d into preply:master Nov 2, 2019
@erebus1
Copy link
Collaborator

erebus1 commented Nov 20, 2019

@jonnii

One more thing we noticed was that the @key is not converted from snake_case to camelCase which causes issues if you have a key like my_magic_key.

Fixed: #19

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

Successfully merging this pull request may close these issues.

2 participants