fix(connectors): carry executable directives into synthetic connector subgraphs

[benjamn]

Summary

Composing connector subgraphs alongside an executable directive whose arguments reference a custom type (for example directive @ownership(owners: [Owner!]) on FIELD with enum Owner) failed satisfiability validation:

INTERNAL_ERROR: ... Cannot wrap an undefined/null type

Rust-native composition reports the same problem as Directive argument "@ownership(owners:)"'s inner type "Owner" does not refer to an existing input type.

Root cause

split_subgraph() builds each synthetic connector subgraph from an empty schema, and neither expander (legacy v0.1-v0.3 nor v0.4+) copied executable directive definitions into it. Two bugs followed:

1. Mixed supergraphs (the reported case). merge_subgraphs() emits an executable directive if any single subgraph defines it, so a non-connector subgraph kept the directive in the expanded supergraph. But the synthetic subgraphs never declared its argument types, so @join__type recorded them as absent. Subgraph extraction copies executable directives into every subgraph, which left the synthetic subgraphs referencing types they didn't have.
2. All-connector supergraphs. With no non-connector subgraph to supply it, the executable directive was dropped from the expanded supergraph entirely.

Fix

Per review, the fix now lives in split_subgraph() rather than in carryover. Both expanders gain process_executable_directives, which runs first in expand(): for each directive definition in the source subgraph that has an executable location (and isn't already in the new schema, which skips built-ins), it carries over the argument types using the existing process_inputs (scalars, enums, and input objects, recursively), then inserts the definition.

The synthetic subgraphs are now valid GraphQL on their own, and merging produces the correct @join__type / @join__enumValue / @join__field membership without any hand-written join logic. The earlier carryover/inputs.rs approach is gone.

Tests

Two regression tests in tests/composition/connectors.rs, each run against connect/v0.2 (legacy expander) and v0.4:

• executable_directive_arg_types_survive_connector_expansion: a connector subgraph plus a plain subgraph, both defining the directive. Composition fails before the fix and succeeds after.
• executable_directive_survives_all_connector_expansion: connector subgraphs only. Composes, runs expand_connectors, and asserts the expanded supergraph keeps the directive and its enum, scalar, and input types, and that subgraph extraction succeeds. Before the fix, the directive was missing.

Full apollo-federation suite passes with no snapshot changes.

Not in scope

merge_subgraphs() still emits an executable directive when any subgraph defines it, rather than requiring all of them like real composition. That's a behavior change for non-connector inputs, so it's left for a follow-up (and goes away once expansion moves to the modern merge code).

Ref: RH-1375

[apollo-librarian]

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: d507af89603b2af8236da390
Build Logs: View logs

---

✅ AI Style Review — No Changes Detected

No MDX files were changed in this pull request.

Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

[sachindshinde]

The issue here is the supergraph schema that expand_connectors() generates for satisfiability checking is invalid. That code is declaring (via @join__types in the supergraph schema) that certain enum types aren't present in the synthetic subgraphs when they really should be, and your PR change (instead of fixing the pathway that generates the supergraph schema for connectors) is saying that subgraph extraction logic should just ignore the invalid @join__types and try to patch up the synthetic subgraph schemas by looking at usages. That might be necessary if e.g. those invalid supergraph schemas were passing composition and we needed to support them in the router, but they're getting caught at composition time, and we should keep rejecting them there (and fix connectors code).

[sachindshinde]

Two general comments here.

The first is that while the approach in this PR does try to ensure a valid supergraph schema is generated, I think the way it goes about this isn't robust. Specifically, the current approach tries to patch up the generated supergraph schema, instead of modifying the code that generates the synthetic subgraph schemas from the extracted subgraphs (i.e. split_subgraph()).

I can understand why the supergraph-modification approach might make sense in the current codebase, since subgraph extraction currently doesn't extract all the features of the supergraph, and connectors code has historically worked around this via some manual carryover logic in carryover_directives() (logic that this approach modifies, specifically via copy_input_types()). However, the only reason why subgraph extraction hasn't extracted some features, is that nothing downstream of subgraph extraction needed those features prior to connectors. (Separate from that, the normalization that occurs as part of merging means there isn't a single canonical way to extract subgraphs, but we should be fine to just pick one. The assertion of normalization being correct is that it shouldn't matter to downstream logic, and if it did, the normalization shouldn't have been performed to start with.) In this particular case, subgraph extraction does add the executable directive and any referenced types during subgraph extraction, so AFAICT there's no reason this approach needs to patch the supergraph schema directly.

As part of cleaning up that connectors carryover logic in the future, I expect that we'll update subgraph extraction logic to not just ignore these features (ideally making extraction idempotent, in the sense that extracted subgraphs generate a semantically equivalent supergraph schema when recomposed), and we'll swap out the old merge logic POC with the modern merge logic we use today. This brings us to the first robustness issue, which is that modern merge logic expects the subgraph schemas to be valid GraphQL, so making the changes to carryover_directives() instead of split_subgraph() means we'll have to throw this PR's work away in the future, and make the fix instead to split_subgraph() anyway.

The second robustness issue, is that the code in the current approach tries to mimic/understand how merging and @join__* logic work when modifying the supergraph schema. Doing this is bug-prone because that logic is complex, and ideally we'd leave that to the merge logic itself instead of trying to mimic it.

This brings me to the second general comment, which is that the PR's current code does contain such a bug. When computing @join__enumValues and @join__fields to add, it looks at the @join__types on the desired nodes instead of @join__enumValues and @join__fields on the desired nodes (which means the patched supergraph schema may mistakenly declare an enum value/input field is in a subgraph when it isn't).

If we need to get this fix out fast, fixing the second general comment should be enough. However, if we've got the time, I'd prefer to make the fix to split_subgraph() instead (if there's a specific reason we shouldn't make the fix there, let me know).

[sachindshinde]

So something was bothering me about the expand_connectors() code. I had assumed since I didn't see any code in carryover_directives() for copying over executable directive definitions, that the executable directive definitions were being passed into merge_subgraphs() instead. I'd assumed merge_subgraphs() functioned similarly to actual merge code today when it came to executable directive definitions, and would only emit an executable directive definition into the supergraph schema when it was present in all subgraphs.

The thing is though, when I looked at split_subgraph() more closely, I didn't see any logic there that kept the executable directive definitions around (I'd assumed before I was just missing something). It turns out that merge_subgraphs() actually contains a bug, in that if any single subgraph contains an executable directive definition, that executable directive definition still gets emitted into the supergraph schema. This would explain the behavior we were seeing; a non-connectors subgraph had the executable directive definitions and the synthetic subgraphs did not, but the bug in merge_subgraphs() meant they were still appearing in the supergraph schema.

What this means is there's actually another expand_connectors() bug, where executable directive definitions are dropped entirely from the returned supergraph schema if all the subgraphs are connector subgraphs. The robust fix for this IMO is to ensure split_subgraph() places both executable directive definitions and their referenced types into the synthetic subgraphs (I'd assumed it was at least doing the former, but it's not). The alternative is to modify carryover_directives() to carryover executable directive definitions from the supergraph schema when absent (though again, we'd probably end up throwing this logic away in the future when we replace merge_subgraphs() with actual merge code, and modifying split_subgraph() anyway).

[benjamn]

Reworked per review: the fix now lives in split_subgraph(). Both expanders copy executable directive definitions from the source subgraph and carry over their argument types via process_inputs, and the carryover/@join__* patching is gone entirely. That also fixes the all-connector case you spotted, where the directive was dropped (new test covers it on v0.2 and v0.4). I left the merge_subgraphs() any-vs-all behavior alone since it also backs the public Supergraph::compose, and with every synthetic subgraph now carrying the directive it no longer affects expansion. Happy to do it as a follow-up.