Support different arguments for different aliases #482

kryops · 2022-05-17T15:41:38Z

Hi there,

after experimenting with different workarounds in #481, I believe I may have found a proper fix for #126.

Please let me know what you think ☺️

I published the changes to npm as @kryops/join-monster@3.1.1-1 if anyone wants to try it out with their schema.

Description

The general strategy is as follows:

join-monster tries to detect conflicting aliases to the same field
If a conflict is detected, it will set multiple properties on the output object instead of overwriting the fieldName:

The query

{
  users {
    following { fullName }
    matts: following(name: "matt") { fullName }
  }
}

generates per user:

{
  following$: [
    { fullName: 'andrew carlson' },
    { fullName: 'matt elder' }
  ],
  following$matts: [
    { fullName: 'matt elder' }
  ],
  following: (args, context, info) => { /* ... */ }
}

GraphQL's default resolver supports functions as properties, and will call them with the information necessary to return the correct property.

This is a breaking change when using custom resolvers: Instead of always accessing source[fieldName], GraphQL's defaultFieldResolver has to be used to get the raw value in order to support multiple conflicting aliases:

import { defaultFieldResolver } from 'graphql'

// ...

resolve: (source, args, context, info) => {
  const rawValue = defaultFieldResolver(source, args, context, info)
  return processUsers(rawValue)
}

Note that this should be only actually breaking in situations that likely used to return wrong data. For normal fields and non-conflicting aliases the same format is used as before.

References

Fixes #126

Fixes #146

Checklist

I have added documentation for new/changed functionality in this PR via comments and by updating the change log
I'm not entirely sure when to consider aliases as "conflicting". For now I have implemented the following logic:
- Some types of nodes are never conflicting because join-monster does not use the args, thus they should point to the same data anyway (e.g. normal columns)
- Some types of nodes are always conflicting because even using the same args, they can have nested conflicting aliases inside them (e.g. tables, unions)
- For other types of nodes (e.g. sqlExpr), the args may be used by join-monster, so aliases are conflicting if they use different args
Are the generated property keys "unique enough"? What if a schema contains both comments and comments$?
We should probably add a separate test for aliases with paginated schemas. Maybe Post.comments or User.comments?
We should probably add a separate test for aliases with unions and interfaces. Any ideas how to best do it in the test schema? (should we add args to writtenMaterial1 or writtenMaterial2?)
I wasn't sure where to add the new behavior in the documentation. For now, I added it to the warnings page.

DanielSWolf · 2022-05-18T06:32:21Z

Are the generated property keys "unique enough"? What if a schema contains both comments and comments$?

According to the GraphQL spec, the $ character cannot be used in a GraphQL identifier. So it is my understanding that the keys you generate are always unique.

test/aliases.js

kryops · 2023-12-05T07:27:27Z

Just rebased on v3.3.2.

Any chance of moving this forward?

nicoabie · 2024-04-30T20:52:44Z

@bueche can you take a look into this one?

nicoabie · 2024-05-04T18:54:22Z

@kryops This week I become maintainer of this project and I would love helping you merge this PR if you are still interested.

Let me know, have a great weekend!

bueche · 2024-05-05T23:11:22Z

docs/CHANGELOG.md

+    following: {
+      // ...
+      resolve: (source, args, context, info) => {
+        const rawValue = defaultFieldResolver(source, args, context, info)


assuming this is only required for "non-trivial" field usage. Please confirm. Also, perhaps it's useful to explain in more detail how this helps. Isn't this defaultFieldResolver() called under-the-hood?

Yes, defaultFieldResolver is only required for non-trivial fields.

The problem is that GraphQL only uses its defaultFieldResolver if you do not specify a custom resolve function. As soon as you do, you need to handle this yourself.

I extended the section on custom resolvers in the docs and added a link to it from the changelog, please let me know if this covers it better now 😅

bueche · 2024-05-05T23:20:29Z

docs/CHANGELOG.md

+      // ...
+      resolve: (source, args, context, info) => {
+        const rawValue = defaultFieldResolver(source, args, context, info)
+        return processUsers(rawValue)


and what do you mean by "rawValue" here? raw vs. what??

Good point, it's just the value of the field - changed it to value

bueche · 2024-05-06T00:01:00Z

docs/warnings.md

+
+The `source.following` property is now a function that will return the correct value. GraphQL's default resolver will detect the function and do the right thing.
+
+Therefore, we recommend getting the raw value through GraphQL's default resolver first when using a custom resolver on a non-trivial field:


"non-trivial field" ... this should have a more precise definition at least as best as you understand it.

"recommend" ? ... perhaps too softly worded. What happens if you don't use the default resolver ... and (again) why are we calling the default resolver explicitly when it was previously called implicitly?

True, it might throw an error or do weird things if you don't use the default resolver and a query uses multiple aliases, because source[fieldName] would contain a function instead of the field value.

I changed the wording to "need to", and added a section explaining "non-trivial" values in more detail.

bueche · 2024-05-06T00:03:58Z

test-api/schema-basic/User.js

@@ -275,6 +275,9 @@ const User = new GraphQLObjectType({
    },
    writtenMaterial1: {
      type: new GraphQLList(AuthoredUnion),
+      args: {
+        search: { type: GraphQLString },


Excellent to add to the tests ... can you explain what is being added to test this?

This was done to be able to test multiple aliases with different args on union and interface types. I added a comment explaining this.

test/aliases.js

kryops · 2024-05-06T05:52:11Z

Hi all, yes I'm still interested in getting this merged.

Thanks for the feedback, I'll try to find some time this week to address it 😅

nicoabie · 2024-05-27T12:43:39Z

I'm planning to do a v3.3.5 with the small fixes currently in master and then create a v3.4.0 release with this.
@bueche what are your thoughts?

kryops · 2024-05-27T13:06:54Z

Should this rather be a v4.0.0 as it is a breaking change for custom resolvers?

nicoabie · 2024-05-27T13:16:13Z

I understood it is only a breaking change for users that are using broken aliases but yeah we can for sure make it v4.0.0

bueche · 2024-05-28T00:43:05Z

hi Nico will try to look at it soon. cheers Ed

…

On Sat, May 4, 2024 at 11:54 AM Nico Gallinal ***@***.***> wrote: @kryops <https://github.com/kryops> This week I become maintainer of this project and I would love helping you merge this PR if you are still interested. Let me know, have a great weekend! — Reply to this email directly, view it on GitHub <#482 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJTEHDK3Q5RLRS5KHKWLZ2LZAUVHHAVCNFSM5WFMAHH2U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBZGQZTINRUGEZA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

kryops mentioned this pull request May 17, 2022

Problem with aliases on paginated fields #126

Open

DanielSWolf reviewed May 18, 2022

View reviewed changes

test/aliases.js Outdated Show resolved Hide resolved

kryops force-pushed the alias-pr branch 2 times, most recently from 0010f49 to e41f974 Compare December 5, 2023 07:24

bueche reviewed May 5, 2024

View reviewed changes

bueche reviewed May 6, 2024

View reviewed changes

test/aliases.js Show resolved Hide resolved

kryops and others added 2 commits May 6, 2024 10:29

Support different arguments for different aliases

bdd6d44

Address PR feedback

10f203c

kryops force-pushed the alias-pr branch from e41f974 to 10f203c Compare May 7, 2024 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support different arguments for different aliases #482

Support different arguments for different aliases #482

kryops commented May 17, 2022 •

edited

DanielSWolf commented May 18, 2022

kryops commented Dec 5, 2023

nicoabie commented Apr 30, 2024

nicoabie commented May 4, 2024

bueche May 5, 2024 •

edited

kryops May 7, 2024

bueche May 5, 2024

kryops May 7, 2024

bueche May 6, 2024

kryops May 7, 2024

bueche May 6, 2024

kryops May 7, 2024

kryops commented May 6, 2024

nicoabie commented May 27, 2024

kryops commented May 27, 2024

nicoabie commented May 27, 2024

bueche commented May 28, 2024 via email


		The `source.following` property is now a function that will return the correct value. GraphQL's default resolver will detect the function and do the right thing.

		Therefore, we recommend getting the raw value through GraphQL's default resolver first when using a custom resolver on a non-trivial field:

Support different arguments for different aliases #482

Are you sure you want to change the base?

Support different arguments for different aliases #482

Conversation

kryops commented May 17, 2022 • edited

Description

References

Checklist

DanielSWolf commented May 18, 2022

kryops commented Dec 5, 2023

nicoabie commented Apr 30, 2024

nicoabie commented May 4, 2024

bueche May 5, 2024 • edited

Choose a reason for hiding this comment

kryops May 7, 2024

Choose a reason for hiding this comment

bueche May 5, 2024

Choose a reason for hiding this comment

kryops May 7, 2024

Choose a reason for hiding this comment

bueche May 6, 2024

Choose a reason for hiding this comment

kryops May 7, 2024

Choose a reason for hiding this comment

bueche May 6, 2024

Choose a reason for hiding this comment

kryops May 7, 2024

Choose a reason for hiding this comment

kryops commented May 6, 2024

nicoabie commented May 27, 2024

kryops commented May 27, 2024

nicoabie commented May 27, 2024

bueche commented May 28, 2024 via email

kryops commented May 17, 2022 •

edited

bueche May 5, 2024 •

edited