Skip to content

Commit c9ac9de

Browse files
dariuszkucclaude
andcommitted
fix(federation): resolve spec versions like JS getMinimumRequiredVersion
`SpecDefinitions` resolved the spec version to use for a federation version with `Version::satisfies`, which requires matching majors. That only worked while fed 1 and fed 2 were the only majors: for federation v3.0 every spec whose minimum federation version is v2.x (link, inaccessible, tag, cost, context, ...) resolved to nothing, which is what the `Version::satisfies_federation` carveout papered over. The JS `FeatureDefinitions.getMinimumRequiredVersion` instead returns the latest version whose minimum federation version is `<=` the given one, clamped to the spec's latest major. Port it as `get_maximum_allowed_version`, which is now the only lookup: - Drop `get_minimum_required_version` (oldest compatible version with a matching major); for every fed 2.x version it resolves to the same spec versions as the ported lookup. - Rename `get_dyn_minimum_required_version` to `get_dyn_maximum_allowed_version`. - Drop `Version::satisfies_federation`. JS: https://github.com/apollographql/federation/blob/7c0a8ba4cf061cf0e2a6537803ed828c36234bed/internals-js/src/specs/coreSpec.ts#L660-L675 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 184efd1 commit c9ac9de

12 files changed

Lines changed: 91 additions & 61 deletions

‎apollo-federation/src/connectors/spec/type_and_directive_specifications.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ fn connect_directive_spec() -> DirectiveSpecification {
461461
None,
462462
// Some(DirectiveCompositionOptions {
463463
// supergraph_specification: &|v| {
464-
// CONNECT_VERSIONS.get_dyn_minimum_required_version(v)
464+
// CONNECT_VERSIONS.get_dyn_maximum_allowed_version(v)
465465
// },
466466
// static_argument_transform: None,
467467
// use_join_directive: true,

‎apollo-federation/src/link/authenticated_spec_definition.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ impl AuthenticatedSpecDefinition {
4747
],
4848
Some(DirectiveCompositionOptions {
4949
supergraph_specification: &|v| {
50-
AUTHENTICATED_VERSIONS.get_dyn_minimum_required_version(v)
50+
AUTHENTICATED_VERSIONS.get_dyn_maximum_allowed_version(v)
5151
},
5252
static_argument_transform: None,
5353
use_join_directive: false,

‎apollo-federation/src/link/context_spec_definition.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ impl SpecDefinition for ContextSpecDefinition {
136136
DirectiveLocation::Union,
137137
],
138138
Some(DirectiveCompositionOptions {
139-
supergraph_specification: &|v| CONTEXT_VERSIONS.get_dyn_minimum_required_version(v),
139+
supergraph_specification: &|v| CONTEXT_VERSIONS.get_dyn_maximum_allowed_version(v),
140140
static_argument_transform: Some(Rc::new(Self::static_argument_transform)),
141141
use_join_directive: false,
142142
}),

‎apollo-federation/src/link/cost_spec_definition.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,7 @@ impl CostSpecDefinition {
316316
DirectiveLocation::Scalar,
317317
],
318318
Some(DirectiveCompositionOptions {
319-
supergraph_specification: &|v| COST_VERSIONS.get_dyn_minimum_required_version(v),
319+
supergraph_specification: &|v| COST_VERSIONS.get_dyn_maximum_allowed_version(v),
320320
static_argument_transform: None,
321321
use_join_directive: false,
322322
}),
@@ -363,7 +363,7 @@ impl CostSpecDefinition {
363363
false,
364364
&[DirectiveLocation::FieldDefinition],
365365
Some(DirectiveCompositionOptions {
366-
supergraph_specification: &|v| COST_VERSIONS.get_dyn_minimum_required_version(v),
366+
supergraph_specification: &|v| COST_VERSIONS.get_dyn_maximum_allowed_version(v),
367367
static_argument_transform: None,
368368
use_join_directive: false,
369369
}),

‎apollo-federation/src/link/federation_spec_definition.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -956,7 +956,7 @@ impl FederationSpecDefinition {
956956
],
957957
Some(DirectiveCompositionOptions {
958958
supergraph_specification: &|v| {
959-
CACHE_TAG_VERSIONS.get_dyn_minimum_required_version(v)
959+
CACHE_TAG_VERSIONS.get_dyn_maximum_allowed_version(v)
960960
},
961961
static_argument_transform: None,
962962
use_join_directive: true,
@@ -1004,7 +1004,7 @@ impl SpecDefinition for FederationSpecDefinition {
10041004
specs.push(Box::new(self.shareable_directive_specification()));
10051005

10061006
if let Some(inaccessible_spec) =
1007-
INACCESSIBLE_VERSIONS.get_dyn_minimum_required_version(self.version())
1007+
INACCESSIBLE_VERSIONS.get_dyn_maximum_allowed_version(self.version())
10081008
{
10091009
specs.extend(inaccessible_spec.directive_specs());
10101010
}

‎apollo-federation/src/link/inaccessible_spec_definition.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ impl InaccessibleSpecDefinition {
128128
locations,
129129
Some(DirectiveCompositionOptions {
130130
supergraph_specification: &|v| {
131-
INACCESSIBLE_VERSIONS.get_dyn_minimum_required_version(v)
131+
INACCESSIBLE_VERSIONS.get_dyn_maximum_allowed_version(v)
132132
},
133133
static_argument_transform: None,
134134
use_join_directive: false,

‎apollo-federation/src/link/policy_spec_definition.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ impl PolicySpecDefinition {
6868
DirectiveLocation::Enum,
6969
],
7070
Some(DirectiveCompositionOptions {
71-
supergraph_specification: &|v| POLICY_VERSIONS.get_dyn_minimum_required_version(v),
71+
supergraph_specification: &|v| POLICY_VERSIONS.get_dyn_maximum_allowed_version(v),
7272
static_argument_transform: None,
7373
use_join_directive: false,
7474
}),

‎apollo-federation/src/link/requires_scopes_spec_definition.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ impl RequiresScopesSpecDefinition {
7171
],
7272
Some(DirectiveCompositionOptions {
7373
supergraph_specification: &|v| {
74-
REQUIRES_SCOPES_VERSIONS.get_dyn_minimum_required_version(v)
74+
REQUIRES_SCOPES_VERSIONS.get_dyn_maximum_allowed_version(v)
7575
},
7676
static_argument_transform: None,
7777
use_join_directive: false,

‎apollo-federation/src/link/spec.rs‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -120,19 +120,6 @@ impl Version {
120120

121121
self.major == min.major && self.minor >= min.minor && self.minor <= max.minor
122122
}
123-
124-
/// Whether this federation version satisfies the provided minimum federation version.
125-
///
126-
/// Unlike [`Version::satisfies`], federation versions are cumulative across majors from v2.0
127-
/// onwards: federation v3.0 supports everything federation v2.x does. Federation v1 remains a
128-
/// separate line, so a fed 1 requirement is only satisfied by a fed 1 version.
129-
pub(crate) fn satisfies_federation(&self, required: &Version) -> bool {
130-
if required.major < 2 {
131-
self.satisfies(required)
132-
} else {
133-
self >= required
134-
}
135-
}
136123
}
137124

138125
/// A `@link` specification url, which identifies a specific version of a specification.
@@ -254,29 +241,6 @@ mod tests {
254241
);
255242
}
256243

257-
#[test]
258-
fn federation_versions_satisfy_across_majors() {
259-
let fed_1_0 = Version { major: 1, minor: 0 };
260-
let fed_2_0 = Version { major: 2, minor: 0 };
261-
let fed_2_12 = Version {
262-
major: 2,
263-
minor: 12,
264-
};
265-
let fed_3_0 = Version { major: 3, minor: 0 };
266-
267-
assert!(fed_3_0.satisfies_federation(&fed_2_0));
268-
assert!(fed_3_0.satisfies_federation(&fed_2_12));
269-
assert!(fed_3_0.satisfies_federation(&fed_3_0));
270-
assert!(fed_2_12.satisfies_federation(&fed_2_0));
271-
assert!(!fed_2_12.satisfies_federation(&fed_3_0));
272-
assert!(!fed_1_0.satisfies_federation(&fed_2_0));
273-
274-
// Fed 1 requirements are only met by fed 1 versions.
275-
assert!(fed_1_0.satisfies_federation(&fed_1_0));
276-
assert!(!fed_2_0.satisfies_federation(&fed_1_0));
277-
assert!(!fed_3_0.satisfies_federation(&fed_1_0));
278-
}
279-
280244
#[test]
281245
fn valid_versions_can_be_parsed() {
282246
assert_eq!(

‎apollo-federation/src/link/spec_definition.rs‎

Lines changed: 79 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -268,30 +268,96 @@ impl<T: SpecDefinition> SpecDefinitions<T> {
268268
self.definitions.iter()
269269
}
270270

271+
/// Returns the latest spec version usable with the given federation version, i.e. the latest
272+
/// one whose minimum federation version is at most `federation_version`.
273+
///
274+
/// Only versions with the same major as the latest spec version are returned: if the matching
275+
/// version has an older major, the oldest version with the latest major is returned instead.
276+
// PORT_NOTE: This corresponds to `FeatureDefinitions.getMinimumRequiredVersion` in JS.
271277
pub(crate) fn get_maximum_allowed_version(
272278
&'static self,
273279
federation_version: &Version,
274280
) -> Option<&'static T> {
275-
self.definitions
281+
let spec = self
282+
.definitions
276283
.values()
277284
.rev()
278-
.find(|spec| federation_version.satisfies_federation(spec.minimum_federation_version()))
279-
}
280-
281-
pub(crate) fn get_minimum_required_version(
282-
&'static self,
283-
federation_version: &Version,
284-
) -> Option<&'static T> {
285-
self.definitions
286-
.values()
287-
.find(|spec| federation_version.satisfies_federation(spec.minimum_federation_version()))
285+
.find(|spec| federation_version >= spec.minimum_federation_version())?;
286+
let latest_major = self.latest().version().major;
287+
if spec.version().major != latest_major {
288+
return self
289+
.definitions
290+
.values()
291+
.find(|spec| spec.version().major == latest_major);
292+
}
293+
Some(spec)
288294
}
289295

290-
pub(crate) fn get_dyn_minimum_required_version(
296+
pub(crate) fn get_dyn_maximum_allowed_version(
291297
&'static self,
292298
federation_version: &Version,
293299
) -> Option<&'static dyn SpecDefinition> {
294-
self.get_minimum_required_version(federation_version)
300+
self.get_maximum_allowed_version(federation_version)
295301
.map(|spec| spec as &dyn SpecDefinition)
296302
}
297303
}
304+
305+
#[cfg(test)]
306+
mod tests {
307+
use super::*;
308+
use crate::link::cost_spec_definition::COST_VERSIONS;
309+
use crate::link::inaccessible_spec_definition::INACCESSIBLE_VERSIONS;
310+
use crate::link::join_spec_definition::JOIN_VERSIONS;
311+
use crate::link::link_spec_definition::LINK_VERSIONS;
312+
use crate::link::tag_spec_definition::TAG_VERSIONS;
313+
314+
fn v(major: u32, minor: u32) -> Version {
315+
Version { major, minor }
316+
}
317+
318+
fn maximum_allowed<T: SpecDefinition>(
319+
definitions: &'static SpecDefinitions<T>,
320+
federation_version: Version,
321+
) -> Option<Version> {
322+
definitions
323+
.get_maximum_allowed_version(&federation_version)
324+
.map(|spec| spec.version().clone())
325+
}
326+
327+
#[test]
328+
fn maximum_allowed_version_is_latest_compatible_version() {
329+
assert_eq!(maximum_allowed(&JOIN_VERSIONS, v(2, 0)), Some(v(0, 3)));
330+
assert_eq!(maximum_allowed(&JOIN_VERSIONS, v(2, 7)), Some(v(0, 4)));
331+
assert_eq!(maximum_allowed(&JOIN_VERSIONS, v(2, 15)), Some(v(0, 5)));
332+
assert_eq!(maximum_allowed(&JOIN_VERSIONS, v(3, 0)), Some(v(0, 6)));
333+
}
334+
335+
#[test]
336+
fn maximum_allowed_version_skips_fed_1_versions_for_fed_2() {
337+
// Tag v0.1/v0.2 and inaccessible v0.1 only require federation v1.0, but a later version is
338+
// compatible with fed 2 and takes precedence.
339+
assert_eq!(maximum_allowed(&TAG_VERSIONS, v(2, 0)), Some(v(0, 3)));
340+
assert_eq!(
341+
maximum_allowed(&INACCESSIBLE_VERSIONS, v(2, 0)),
342+
Some(v(0, 2))
343+
);
344+
assert_eq!(maximum_allowed(&LINK_VERSIONS, v(2, 0)), Some(v(1, 0)));
345+
}
346+
347+
#[test]
348+
fn maximum_allowed_version_accepts_fed_2_versions_for_fed_3() {
349+
assert_eq!(maximum_allowed(&TAG_VERSIONS, v(3, 0)), Some(v(0, 3)));
350+
assert_eq!(
351+
maximum_allowed(&INACCESSIBLE_VERSIONS, v(3, 0)),
352+
Some(v(0, 2))
353+
);
354+
assert_eq!(maximum_allowed(&LINK_VERSIONS, v(3, 0)), Some(v(1, 0)));
355+
assert_eq!(maximum_allowed(&COST_VERSIONS, v(3, 0)), Some(v(0, 1)));
356+
}
357+
358+
#[test]
359+
fn maximum_allowed_version_is_none_below_the_minimum_federation_version() {
360+
assert_eq!(maximum_allowed(&COST_VERSIONS, v(2, 8)), None);
361+
assert_eq!(maximum_allowed(&LINK_VERSIONS, v(1, 0)), None);
362+
}
363+
}

0 commit comments

Comments
 (0)