Connectors by lennyburdette · Pull Request #5108 · apollographql/router

lennyburdette · 2024-05-07T15:55:21Z

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

github-actions · 2024-05-07T15:55:33Z

@lennyburdette, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

router-perf · 2024-05-07T15:55:54Z

…deration's Connector (#5423)

abernix · 2024-07-30T13:17:33Z

saw two things that can likely land in dev right away too

This is really good to call out – do we inventory this somewhere?

Geal

I have only reviewed the parts where the connectors interact with existing router functionality, I'll dig into the connectors plugin itself later.
It's good overall, there's a few things that will need attention, but they're not blockers

Cargo.lock

Geal · 2024-07-30T07:26:49Z

apollo-router/Cargo.toml

 fred = { version = "7.1.2", features = ["enable-rustls", "mocks"] }
 futures-test = "0.3.30"
 insta.workspace = true
+json_value_merge = "2.0.0"


we already have internal code for this https://github.com/apollographql/router/blob/dev/apollo-router/src/json_ext.rs#L68-L96
At this point we should be careful about adding any new dependencies, because compilations is already too heavy

Geal · 2024-07-30T07:29:47Z

apollo-router/Cargo.toml


 # See note above in this file about `^tracing` packages which also applies to
 # these dev dependencies.
+tracing-fluent-assertions = "0.3.0"


we already use at least 2 other crates to test tracing and spans, is this one really needed?

Geal · 2024-07-30T07:32:59Z

apollo-router/src/plugin/mod.rs

        };
    };
+
+    ($group: literal, $name: ident, $plugin_type: ident) => {


NIT: instead of 2 different declarations with ident and literal, we should have only one with expr

Geal · 2024-07-30T09:42:53Z

apollo-router/src/query_planner/execution.rs

+                                    .supergraph_request(parameters.supergraph_request.clone())
+                                    .variables(variables)
+                                    .current_dir(current_dir.clone())
+                                    .deferred_fetches(parameters.deferred_fetches.clone())


this clone will cause performance issues. The ExecutionParameters structure that uses references everywhere was introduced to avoid the clones during execution, which became expensive.
The only reason deferred_fetches is used in FetchNode::fetch_node is this:

router/apollo-router/src/query_planner/fetch.rs

Lines 522 to 529 in d940c3f

if let Some(id) = &self.id {

if let Some(sender) = parameters.deferred_fetches.get(id.as_str()) {

tracing::info!(monotonic_counter.apollo.router.operations.defer.fetch = 1u64);

if let Err(e) = sender.clone().send((value.clone(), errors.clone())) {

tracing::error!("error sending fetch result at path {} and id {:?} for deferred response building: {}", current_dir, self.id, e);

}

}

}

That should be moved right here in execution.rs

Geal · 2024-07-30T09:52:35Z

apollo-router/src/query_planner/fetch.rs

+    pub(crate) fn response_at_path<'a>(
        schema: &Schema,
        current_dir: &'a Path,
        inverted_paths: Vec<Vec<Path>>,
        response: graphql::Response,
+        requires: &[Selection],
+        output_rewrites: &Option<Vec<DataRewrite>>,
+        service_name: &str,


why was self removed?

Geal · 2024-07-30T09:53:49Z

apollo-router/src/query_planner/subscription.rs

            .and_connection_closed_signal(parameters.subscription_handle.as_ref().map(|s| s.closed_signal.resubscribe()))
            .build();

+        // TODO[igni]: refactor so it uses fetchservice


please open an issue to follow up on this?

i've filed issues for all comments on this PR, thank you!

Geal · 2024-07-30T09:54:10Z

apollo-router/src/query_planner/subscription.rs

-            .create(service_name)
-            .expect("we already checked that the service exists during planning; qed");
+            .subgraph_service_for_subscriptions(service_name)
+            .unwrap();


this should be an expect

Geal · 2024-07-30T10:06:24Z

apollo-router/src/services/fetch.rs

@@ -0,0 +1,54 @@
+#![allow(missing_docs)] // FIXME


Geal · 2024-07-30T10:21:51Z

apollo-router/src/spec/schema.rs

+                let join_directive = value
+                    .directives
+                    .iter()
+                    .find(|directive| directive.name.eq_ignore_ascii_case("join__graph"))?;


does the directive name need to be case insensitive?

lennyburdette · 2024-07-30T13:49:59Z

saw two things that can likely land in dev right away too

done in #5720

dylan-apollo

I'm ready when you are

apollo-bot2 assigned lennyburdette May 7, 2024

Squashed all of the connectors branch

2858152

lennyburdette force-pushed the connectors branch from fa13cd4 to 2858152 Compare June 12, 2024 00:30

lennyburdette and others added 25 commits June 12, 2024 09:22

make_requests() using models::Connector

7d38ee4

send already created Variables to the FetchService

374cfc0

handle_responses() using models::Connector

5cf4eb8

update snapshot

e1cf4bc

More @connect validations (#5427)

6070506

squash code review

71b677a

new versions of make_requests and handle_responses that use apollo-fe…

ea9c6b1

…deration's Connector (#5423)

Merge branch 'dev' into connectors

e49c80c

Add license check for connectors (#5436)

c229410

fix variables/value in http transport make_request (#5441)

d20e37a

Add validation of connectors HTTP methods (#5461)

c62c2bb

connector: initial expand subgraphs (#5456)

9fb00b5

Adding validity condition for entity arg to only be used with root Query

f413615

changeset

7930ecb

deleting changelog

25a94df

rebased so fixing merge conflicts & updating tests

3dcb4ec

fixing indentation format

238799e

use expand_connectors to get new supergraph and connectors map (#5465)

c79f13a

Add validation check for entity arg on root Query field (#5468)

1880c8c

Merge branch 'dev' into connectors

1b3db9e

update to RouterBody

e204ba0

fix snapshot after dev update

a616ec3

use connect::Request/Response (#5482)

d48bbfd

Fix validation test data and snapshot paths (#5498)

e34d3bf

adding validation for entity types with entity:true

2df21b1

lennyburdette marked this pull request as ready for review July 30, 2024 01:23

lennyburdette requested a review from a team July 30, 2024 01:23

lennyburdette requested review from a team, SimonSapin, TylerBloom, benjamn, dariuszkuc, duckki, goto-bus-stop and sachindshinde as code owners July 30, 2024 01:23

lennyburdette requested review from dylan-apollo, nicholascioli, pubmodmatt and tayrrible July 30, 2024 01:23

lennyburdette changed the title ~~DO NOT MERGE Connectors~~ Connectors Jul 30, 2024

abernix approved these changes Jul 30, 2024

View reviewed changes

Geal reviewed Jul 30, 2024

View reviewed changes

lennyburdette and others added 3 commits July 30, 2024 09:37

reset dependencies in cargo.lock

a9c9a28

Replace json_value_merge with router's custom implementation (#5749)

ce6544f

Simplify register_plugin! (#5750)

c4eb0ca

dylan-apollo approved these changes Jul 30, 2024

View reviewed changes

test the quickstart example (#5751)

8ce4894

lennyburdette merged commit 9fa9cf0 into next Jul 30, 2024

lennyburdette deleted the connectors branch July 30, 2024 19:08

lennyburdette restored the connectors branch July 30, 2024 19:42

lennyburdette deleted the connectors branch July 30, 2024 19:42

This was referenced Jul 30, 2024

Remove tracing-fluent-assertions dependency #5752

Merged

Minor connectors fixes from code review #5774

Merged

	if let Some(id) = &self.id {
	if let Some(sender) = parameters.deferred_fetches.get(id.as_str()) {
	tracing::info!(monotonic_counter.apollo.router.operations.defer.fetch = 1u64);
	if let Err(e) = sender.clone().send((value.clone(), errors.clone())) {
	tracing::error!("error sending fetch result at path {} and id {:?} for deferred response building: {}", current_dir, self.id, e);
	}
	}
	}

Conversation

lennyburdette commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

github-actions bot commented May 7, 2024

Uh oh!

router-perf bot commented May 7, 2024

Uh oh!

abernix commented Jul 30, 2024

Uh oh!

Geal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lennyburdette commented Jul 30, 2024

Uh oh!

dylan-apollo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

lennyburdette commented May 7, 2024 •

edited

Loading