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

Read and use the connectionParams from operation extensions #4781

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

enisdenjo
Copy link
Collaborator

@enisdenjo enisdenjo commented Oct 25, 2022

Continuation task of ardatan/graphql-mesh#4724

Uses the connectionParams from the operation extensions to extend the WebSocket connection parameters.

Are there any security implications with doing this?

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2022

🦋 Changeset detected

Latest commit: 1495e11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-tools/executor-graphql-ws Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2022

🚀 Snapshot Release (alpha)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package Version Info
@graphql-tools/executor-graphql-ws 1.1.0-alpha-20230704112753-1495e11d npm ↗︎ unpkg ↗︎

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2022

✅ Benchmark Results

     ✓ no_errors
     ✓ expected_result

     checks.........................: 100.00% ✓ 228       ✗ 0  
     data_received..................: 27 MB   2.6 MB/s
     data_sent......................: 98 kB   9.7 kB/s
     http_req_blocked...............: avg=17.73µs  min=2.7µs   med=3.3µs   max=1.58ms   p(90)=4.07µs   p(95)=4.5µs   
     http_req_connecting............: avg=13.35µs  min=0s      med=0s      max=1.52ms   p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=82.91ms  min=67.01ms med=78.46ms max=212.72ms p(90)=113.05ms p(95)=121.61ms
       { expected_response:true }...: avg=82.91ms  min=67.01ms med=78.46ms max=212.72ms p(90)=113.05ms p(95)=121.61ms
     http_req_failed................: 0.00%   ✓ 0         ✗ 114
     http_req_receiving.............: avg=173.89µs min=119.6µs med=141.1µs max=715.53µs p(90)=251.39µs p(95)=270.81µs
     http_req_sending...............: avg=149.3µs  min=17.1µs  med=23.65µs max=3.78ms   p(90)=36.7µs   p(95)=857.78µs
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s      max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=82.59ms  min=66.87ms med=78.07ms max=212.38ms p(90)=112.8ms  p(95)=121.34ms
     http_reqs......................: 114     11.331394/s
     iteration_duration.............: avg=88.22ms  min=71.65ms med=83.15ms max=219ms    p(90)=118.02ms p(95)=126.29ms
     iterations.....................: 114     11.331394/s
     vus............................: 1       min=1       max=1
     vus_max........................: 1       min=1       max=1

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2022

💻 Website Preview

The latest changes are available as preview in: https://83536ef3.graphql-tools.pages.dev

@enisdenjo enisdenjo self-assigned this Oct 25, 2022
// additional connection params can be supplied through the "webSocketConnectionParams" field in extensions.
// TODO: connection params only from the FIRST operation in lazy mode will be used (detect connectionParams changes and reconnect, too implicit?)
if (extensions?.['webSocketConnectionParams'] && typeof extensions?.['webSocketConnectionParams'] === 'object') {
executorConnectionParams = Object.assign(executorConnectionParams, extensions['webSocketConnectionParams']);
Copy link
Owner

Choose a reason for hiding this comment

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

This looks like leaking because everytime you call executor, it manipulates the "global" object.

Suggested change
executorConnectionParams = Object.assign(executorConnectionParams, extensions['webSocketConnectionParams']);
const requestConnectionParams = Object.assign({}, executorConnectionParams, extensions['webSocketConnectionParams']);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But it should manipulate the global object, the executorConnectionParams are used by the connectionParams option in graphql-ws createClient above, see here:

connectionParams: () => {
const optionsConnectionParams =
(typeof connectionParams === 'function' ? connectionParams() : connectionParams) || {};
return Object.assign(optionsConnectionParams, executorConnectionParams);
},

@theguild-bot theguild-bot mentioned this pull request Apr 23, 2023
@ardatan ardatan changed the title Read and use the webSocketConnectionParams from operation extensions Read and use the connectionParams from operation extensions Jul 4, 2023
@ardatan ardatan force-pushed the websocketconnectionparams branch from 7acc717 to 1495e11 Compare July 4, 2023 11:26
@ardatan ardatan merged commit 104921f into master Jul 4, 2023
@ardatan ardatan deleted the websocketconnectionparams branch July 4, 2023 11:30
@theguild-bot theguild-bot mentioned this pull request Oct 26, 2023
@theguild-bot theguild-bot mentioned this pull request May 23, 2024
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.

2 participants