feat(composition): port updateInaccessibleErrorsWithLinkToSubgraphs f…

[briannafugate408]

…unction from JS

federation/composition-js/src/merging/merge.ts at f3ab499eaf62b1a1c0f08b838d2cbde5accb303a · apollographql/federation

---

Checklist

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

• PR description explains the motivation for the change and relevant context for reviewing
• PR description links appropriate GitHub/Jira tickets (creating when necessary)
• Changeset is included for user-facing changes
• Changes are compatible¹
• Documentation² completed
• Performance impact assessed and acceptable
• Metrics and logs are added³ and documented
• Tests added and passing⁴
  • Unit tests
  • Integration tests
  • Manual tests, as necessary

Exceptions

Note any exceptions here

Notes

Footnotes

1. 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. ↩

2. Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩

3. A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices. ↩

4. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

[apollo-librarian]

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: d9bd47b0b0b60256a9423490
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.

[github-actions]

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

[duckki]

Did you use Claude Sonnet (not Opus)?
Here's the review by Claude Opus:

Here's my review of the PR below. I still need to independently verify this. But let's think about this together.

---

PR Review: Port updateInaccessibleErrorsWithLinkToSubgraphs

Bug: subgraph_marks_inaccessible_element checks the wrong schema

supergraph_coordinate.rs:56-69 — This is the most significant issue. The method receives _subgraph (unused, note the underscore) and schema: &FederationSchema, but the caller at line 413-416 passes the supergraph as schema:

let Ok(marked) = parsed.subgraph_marks_inaccessible_element(
    subgraph,          // unused (_subgraph)!
    &inaccessible_name,
    supergraph,        // <-- should be subgraph.schema()
) else {

The JS reference checks whether the element is marked @inaccessible in each individual subgraph:

const subgraphElement = subgraphSchema.elementByCoordinate(coordinate);
if (subgraphElement) {
    const inaccessibleDirective = federationMetadata(subgraphSchema)!.inaccessibleDirective();
    if (subgraphElement.hasAppliedDirective(inaccessibleDirective)) {
        subgraphHasInaccessibleElements[subgraphIndex] = true;

The Rust code instead checks the supergraph, which means:

• If any subgraph marks an element @inaccessible, the merged supergraph will have it, so the check will return true for all subgraphs that contain the element — not just the ones that actually marked it @inaccessible.
• The subgraph_has_inaccessible per-subgraph tracking becomes meaningless since it'll be true for every subgraph that has the element.
• This affects DISALLOWED_INACCESSIBLE and ONLY_INACCESSIBLE_CHILDREN error handling downstream, which rely on the per-subgraph flag.

Fix: The method should use subgraph.schema() instead of the supergraph. The _subgraph parameter should be used, and the schema parameter removed (or renamed to clarify intent).

---

Performance: self.merged.clone() at merger.rs:561

match Self::validate_supergraph_schema(self.merged.clone(), &self.subgraphs) {

The original code moved self.merged into validate_supergraph_schema. Now it's cloned to avoid a partial-move conflict with &self.subgraphs. Cloning an entire FederationSchema may be expensive. Consider destructuring self to allow moving merged while borrowing subgraphs:

let Merger { merged, subgraphs, .. } = self;
match Self::validate_supergraph_schema(merged, &subgraphs) {

(Or use std::mem::take if destructuring is awkward.)

---

Minor issues

1. No tests — The new supergraph_coordinate module has no unit tests. At minimum, parse_supergraph_coordinate deserves test coverage for each coordinate form (Type, Type.field, Type.field(arg:), @directive, @directive(arg:)), plus edge cases (empty string, malformed input). The end-to-end behavior of update_inaccessible_errors_with_link_to_subgraphs should also have at least one integration test demonstrating that subgraph locations are correctly attached.

2. inaccessible_name vs supergraph directive name — At line 407, inaccessible_name is obtained from the subgraph (subgraph.inaccessible_directive_name()), which returns the potentially-aliased directive name for that subgraph. This name is then used to check against the supergraph (due to the bug above). Even after fixing the schema bug, ensure that directive_target_is_inaccessible and directive_uses_inaccessible use the correct directive name for the schema being queried.

3. InaccessibleCompositionLinks naming — The struct name is somewhat opaque. Consider InaccessibleCoordinates or InaccessibleErrorContext to better convey that it carries supergraph coordinate strings for error enrichment.

4. is_relevant_subgraph_referencer uses supergraph for referencer_base_type_name (supergraph_coordinate.rs:347) — The JS checks the base type of the subgraph referencer's field type. The Rust checks against the supergraph. This works in practice (base type names don't differ across subgraphs), but for correctness parity with JS, the subgraph schema should be used.

---

What looks good

• The coordinate parsing in parse_supergraph_coordinate handles all the forms comprehensively and correctly.
• The InaccessibleCompositionLinks struct on error variants is a clean Rust-idiomatic replacement for JS's extensions['inaccessible_elements']/extensions['inaccessible_referencers'].
• The is_relevant_subgraph_referencer logic matches the JS semantics for all 6 error codes.
• Adding locations: Locations to CompositionError::MergeError and wiring it into add_locations/locations is clean.
• Making directive_uses_inaccessible pub(crate) for reuse is appropriate.