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

Use the operationId as a key for ResponseFieldMapperFactory #2945

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

ansman
Copy link
Contributor

@ansman ansman commented Feb 21, 2021

This will prevent errors when wrapping subscriptions in a shared type.

This fixes #2944

@martinbonnin
Copy link
Contributor

Hi 👋 ! Thanks for opening this.

One issue is that operations "could" have the same id in two different services (different schemas). Granted this is an edge case but I've seen it happen in the integration tests already. Possible solutions:

  1. Wrapping each Subscription in a different class implementing ExtensibleSubscription but that doesn't scale at all (unless writing a KSP processor to generate it automatically, but I don't think we want to go that route 😆)
  2. Using a Pair<Class, String> as a key, feels a bit sub-optimal but that should work
  3. Removing the pool altogether.

I think 3. makes sense as the adapters are statically allocated anyways, what is cached is a thin wrapper around them that shouldn't cost much and doesn't require caching.

@ansman
Copy link
Contributor Author

ansman commented Feb 22, 2021

Ideally I'd want a way to pass data to the serializer built in but I'm waiting for 3.x. I removed the usage of the factory but I left if for binary compatibility reasons.

The class itself is in the internal package and can probably be removed but there are references to it in ApolloCall for example

This will prevent errors when wrapping subscriptions and operations
in a shared type.

This fixes apollographql#2944
@martinbonnin martinbonnin merged commit 87d6d47 into apollographql:main Feb 22, 2021
@martinbonnin
Copy link
Contributor

Thanks!

@ansman ansman deleted the fix/2944 branch February 22, 2021 16:16
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

Successfully merging this pull request may close these issues.

Exception while handling subscriptions
2 participants