Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![apollo-solutions-reviewer[bot]](https://avatars.githubusercontent.com/u/84934393?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid Day-2 guidance; the structure and cross-linking to the existing technotes are good, and it satisfies the AS-323 acceptance criteria (seven patterns, rationales, examples, references). One technical inaccuracy should be corrected before this publishes as authoritative federation guidance.
The nullability section states that non-null @key fields are mandatory and that entity resolution fails immediately on a null key. Federation does not require @key fields to be declared non-null in SDL; composition accepts nullable key fields, and a @key can reference nullable fields. The sound underlying advice is that key values should be present and stable, not that the SDL type must be non-null. Reword so the doc does not assert a spec requirement that does not exist; see inline comment.
Minor: the @override bullet states it is "always paired with deletion of the field from the old subgraph in a follow-up release." Progressive @override (percentage-based rollout) is a supported intermediate step; consider softening "always" to avoid implying immediate deletion is the only path.
The test plan checkboxes (renders cleanly, links resolve) are still unchecked.
|
|
||
| Federation increases the surface area where nullable becomes load-bearing. The two rules that prevent the most pain: | ||
|
|
||
| 1. **Non-null `@key` fields** are mandatory — entity resolution fails immediately on a null key. |
There was a problem hiding this comment.
This overstates the spec. Federation does not require @key fields to be declared non-null in SDL, and it composes subgraphs whose key fields are nullable. The real guidance is that key values must be present and stable for entity resolution to work, which is a data/runtime concern, not an SDL non-null requirement. Suggest rewording to something like: "Key field values must be present and stable; a missing key breaks entity resolution. Prefer non-null key fields where the domain allows it."
There was a problem hiding this comment.
Verified at 31ef8c9: the nullability section now frames this as a runtime concern. It states that key values must be present at runtime, that Federation composes subgraphs whose key fields are SDL-nullable, that the spec does not require @key fields to be declared non-null, and that a missing key at runtime is treated as entity not found rather than a hard error; the non-null SDL recommendation is kept as advice rather than a spec requirement. This thread can be resolved.
Address docs-reviewer feedback. The prior wording asserted that non-null `@key` fields are mandatory and that entity resolution "fails immediately on a null key." Federation does not require @key fields to be declared non-null in SDL — it composes subgraphs whose key fields are SDL-nullable. The real invariant is a runtime one: key *values* must be present and stable for entity resolution to work, and a missing key resolves as "entity not found" rather than a hard error. Reword to state the actual constraint, while still recommending non-null SDL declarations as a discipline (documents the invariant, catches mistakes at compose time). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Re-reviewed at 31ef8c9. The blocking nullability inaccuracy is fixed: the @key section now correctly frames key presence as a runtime concern, states the spec does not require non-null SDL key fields, and replaces the immediate-failure claim with entity not found. The seven Day-2 patterns, rationales, examples, and technote references satisfy the AS-323 acceptance criteria. One previously flagged minor item remains, noted inline; the @OverRide bullet still asserts deletion is always the follow-up, which omits progressive override. Not blocking. The test plan checkboxes are also still unchecked.
|
|
||
| The right answer is almost always "don't" — federate around evolving the existing schema with `@deprecated`. The wrong answer that keeps coming back: `Query.userV2`, `Query.userV3`, `Query.user_new`. Every one of these makes deprecation harder and bloats the schema indefinitely. | ||
|
|
||
| The legitimate use cases for an explicit version split: |
There was a problem hiding this comment.
This still reads as an absolute. @OverRide is not always followed by deleting the field from the old subgraph; progressive @OverRide (percentage-based label rollout) is a supported intermediate state where both subgraphs keep the field during a gradual cutover. Suggest softening always to typically, and mentioning progressive override as the staged path. Non-blocking.
…ride "Always paired with deletion" is inaccurate — progressive @OverRide (percentage-based traffic rollout) is a supported intermediate step before full cutover. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
Adds
docs/advanced-schema-design.md. Day-2 schema design guidance covering the failure modes that show up after Federation is in production: avoiding the second-root-type anti-pattern, single-owner mutations, nullability discipline, pagination at federation scale, errors-as-data vs error throws, the @shareable/@override/@inaccessible trio, and the versioning trap.Cross-links to existing technotes (TN0012, TN0023, TN0024, TN0027, TN0029, TN0041) instead of duplicating their content.
Tracks AS-323.
Test plan
🤖 Generated with Claude Code