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

fix(router): send graphql closing boundary to fit Apollo client #1579

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

df-wg
Copy link
Contributor

@df-wg df-wg commented Feb 10, 2025

Motivation and Context

Users noted that our Multipart implementation, while it works and fits the spec (RFC1341), wasn't being correctly parsed by some clients. Specifically, the client wouldn't parse one message until the next message had been pushed, with it's --graphql prefix.

This stems from an issue in their implementation, but we want to ensure that our router is accessible by as many services as possible.

The response now has the --graphql of the next response being sent early, which doesn't seem to impact any other services that I've checked. If necessary, we can hide this behind a feature flag so it's specific for apollo

wget

wget --no-check-certificate --quiet \
--method POST \
--timeout=0 \
--header 'Accept: multipart/mixed;subscriptionSpec="1.0", application/json' \
--header 'Content-Type: application/json' \
--body-data '{"query":"subscription { countEmp2(max: 6, intervalMilliseconds: 1000) }"}' \
-O - 'http://localhost:3003/graphql'

demo

curl

curl -L 'http://localhost:3003/graphql' -H 'Accept: multipart/mixed;subscriptionSpec="1.0", application/json' -H 'Content-Type: application/json' -d '{"query":"subscription { countEmp2(max: 3, intervalMilliseconds: 500) }"}'

demo2

Apollo Client

const { ApolloClient, InMemoryCache, HttpLink} = require('@apollo/client');
const gql = require('graphql-tag');

// Define your subscription query
const SUBSCRIPTION_QUERY = `
  subscription Test {
    countEmp2(max: 4, intervalMilliseconds: 1000)
  }
`;

// HTTP link for queries and mutations
const httpLink = new HttpLink({
  uri: 'http://localhost:3003/graphql',
  headers: {
    Accept: 'multipart/mixed;subscriptionSpec="1.0", application/json',
  },
});

// Create the Apollo Client
const client = new ApolloClient({
  link: httpLink,
  cache: new InMemoryCache(),
});

// Execute the subscription
const testSubscription = () => {
  const observable = client.subscribe({
    query: gql(SUBSCRIPTION_QUERY),
  });

  // Listen for incoming data
  observable.subscribe({
    next(data) {
      console.log('Subscription data:', data);
    },
    error(err) {
      if (err?.message === 'internal system error') {
        console.log("system error, ignore")
      } else {
        console.error('Subscription error:', err);
      }
    },
    complete() {
      console.log('Subscription completed');
    },
  });
};

// Start the test
console.log("hello world")
testSubscription();

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

Copy link

github-actions bot commented Feb 10, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-b6aa8655a8f66759ce46089232b72d6408ab99ef

@Noroth
Copy link
Contributor

Noroth commented Feb 14, 2025

I was able to verify locally that this change fixes the error from the apollo client.

@alepane21 alepane21 self-assigned this Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants