diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/AbstractSchemaProvider.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/AbstractSchemaProvider.java index da96789f269..3152257173e 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/AbstractSchemaProvider.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/AbstractSchemaProvider.java @@ -16,13 +16,13 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.common.base.MoreObjects; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.Set; import java.util.TreeMap; import java.util.function.BiFunction; -import java.util.stream.Stream; import tech.pegasys.teku.spec.SpecMilestone; import tech.pegasys.teku.spec.config.SpecConfig; import tech.pegasys.teku.spec.schemas.registry.SchemaTypes.SchemaId; @@ -36,38 +36,57 @@ abstract class AbstractSchemaProvider implements SchemaProvider { protected AbstractSchemaProvider( final SchemaId schemaId, final SchemaProviderCreator... schemaProviderCreators) { this.schemaId = schemaId; - final List> creatorsList = Arrays.asList(schemaProviderCreators); - checkArgument(!creatorsList.isEmpty(), "There should be at least 1 creator"); - - Arrays.stream(schemaProviderCreators) - .forEach( - creator -> - creator - .streamSupporterMilestones() - .forEach(milestone -> putCreatorAtMilestone(milestone, creator))); - } - - private void putCreatorAtMilestone( - final SpecMilestone milestone, final SchemaProviderCreator creator) { - if (!milestoneToSchemaCreator.isEmpty()) { - final SpecMilestone lastMilestone = milestoneToSchemaCreator.lastKey(); - if (lastMilestone.isGreaterThanOrEqualTo(milestone)) { - throw new IllegalArgumentException( - "Milestones ascending ordering error: from " + lastMilestone + " to " + milestone); + validateCreators(schemaProviderCreators); + final List> creatorsList = + new ArrayList<>(Arrays.asList(schemaProviderCreators)); + + SchemaProviderCreator lastCreator = null; + for (final SpecMilestone milestone : SpecMilestone.values()) { + if (!creatorsList.isEmpty() && creatorsList.getFirst().milestone == milestone) { + lastCreator = creatorsList.removeFirst(); } - if (lastMilestone != milestone.getPreviousMilestone()) { - throw new IllegalArgumentException( - "Milestones gap detected: from " - + lastMilestone - + " to " - + milestone.getPreviousMilestone()); + if (lastCreator != null) { + milestoneToSchemaCreator.put(milestone, lastCreator); + + final boolean untilSpecReached = + lastCreator + .untilMilestone + .map(untilMilestone -> untilMilestone == milestone) + .orElse(false); + + if (untilSpecReached) { + break; + } } } + } - if (milestoneToSchemaCreator.put(milestone, creator) != null) { - // this can't happen due to preceded checks - throw new IllegalArgumentException("Overlapping creators detected"); + @SafeVarargs + private void validateCreators(final SchemaProviderCreator... schemaProviderCreators) { + checkArgument( + schemaProviderCreators.length > 0, "There should be at least 1 creator for %s", schemaId); + for (int i = 0; i < schemaProviderCreators.length; i++) { + final SchemaProviderCreator currentCreator = schemaProviderCreators[i]; + if (i > 0) { + checkArgument( + currentCreator.milestone.isGreaterThan(schemaProviderCreators[i - 1].milestone), + "Creator's milestones must be in order for %s", + schemaId); + } + final boolean isLast = i == schemaProviderCreators.length - 1; + if (isLast) { + checkArgument( + currentCreator.untilMilestone.isEmpty() + || currentCreator.untilMilestone.get().isGreaterThan(currentCreator.milestone), + "Last creator untilMilestone must be greater than milestone in %s", + schemaId); + } else { + checkArgument( + currentCreator.untilMilestone.isEmpty(), + "Only last creator is allowed to use until for %s", + schemaId); + } } } @@ -97,8 +116,6 @@ static SchemaProviderCreator schemaCreator( final SpecMilestone milestone, final SpecMilestone untilMilestone, final BiFunction creationSchema) { - checkArgument( - untilMilestone.isGreaterThan(milestone), "untilMilestone should be greater than milestone"); return new SchemaProviderCreator<>(milestone, Optional.of(untilMilestone), creationSchema); } @@ -127,14 +144,6 @@ protected record SchemaProviderCreator( SpecMilestone milestone, Optional untilMilestone, BiFunction creator) { - Stream streamSupporterMilestones() { - if (untilMilestone.isEmpty()) { - return Stream.of(milestone); - } - return SpecMilestone.getMilestonesUpTo(untilMilestone.get()).stream() - .filter(m -> m.isGreaterThanOrEqualTo(milestone)); - } - @Override public String toString() { return MoreObjects.toStringHelper(this) diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/SchemaProvider.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/SchemaProvider.java index 7c6efcafd5a..75516b08fe5 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/SchemaProvider.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/schemas/registry/SchemaProvider.java @@ -13,35 +13,11 @@ package tech.pegasys.teku.spec.schemas.registry; -import static tech.pegasys.teku.spec.SpecMilestone.BELLATRIX; -import static tech.pegasys.teku.spec.SpecMilestone.CAPELLA; -import static tech.pegasys.teku.spec.SpecMilestone.DENEB; -import static tech.pegasys.teku.spec.SpecMilestone.ELECTRA; - -import java.util.EnumSet; import java.util.Set; import tech.pegasys.teku.spec.SpecMilestone; import tech.pegasys.teku.spec.schemas.registry.SchemaTypes.SchemaId; interface SchemaProvider { - Set ALL_MILESTONES = EnumSet.allOf(SpecMilestone.class); - Set FROM_BELLATRIX = from(BELLATRIX); - Set FROM_CAPELLA = from(CAPELLA); - Set FROM_DENEB = from(DENEB); - Set FROM_ELECTRA = from(ELECTRA); - - static Set from(final SpecMilestone milestone) { - return EnumSet.copyOf(SpecMilestone.getAllMilestonesFrom(milestone)); - } - - static Set fromTo( - final SpecMilestone fromMilestone, final SpecMilestone toMilestone) { - return EnumSet.copyOf( - SpecMilestone.getAllMilestonesFrom(fromMilestone).stream() - .filter(toMilestone::isLessThanOrEqualTo) - .toList()); - } - T getSchema(SchemaRegistry registry); Set getSupportedMilestones(); diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/schemas/registry/BaseSchemaProviderTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/schemas/registry/BaseSchemaProviderTest.java index c634eefa40f..caf47a764ef 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/schemas/registry/BaseSchemaProviderTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/schemas/registry/BaseSchemaProviderTest.java @@ -26,6 +26,7 @@ import static tech.pegasys.teku.spec.schemas.registry.AbstractSchemaProvider.schemaCreator; import org.junit.jupiter.api.Test; +import tech.pegasys.teku.spec.SpecMilestone; import tech.pegasys.teku.spec.schemas.registry.SchemaTypes.SchemaId; class BaseSchemaProviderTest { @@ -49,14 +50,15 @@ void shouldSupportContinuousWithoutUntil() { when(mockRegistry.getMilestone()).thenReturn(BELLATRIX); assertEquals(provider.getSchema(mockRegistry), "TestSchemaBellatrix"); - assertThat(provider.getSupportedMilestones()).containsExactly(ALTAIR, BELLATRIX); + assertThat(provider.getSupportedMilestones()) + .containsAll(SpecMilestone.getAllMilestonesFrom(ALTAIR)); } @Test void shouldSupportContinuousWithUntil() { final TestSchemaProvider provider = new TestSchemaProvider( - schemaCreator(PHASE0, ALTAIR, (r, c) -> "TestSchemaPhase0"), + schemaCreator(PHASE0, (r, c) -> "TestSchemaPhase0"), schemaCreator(BELLATRIX, CAPELLA, (r, c) -> "TestSchemaBellatrix")); assertEquals(PHASE0, provider.getEffectiveMilestone(PHASE0)); @@ -84,7 +86,7 @@ void shouldSupportContinuousWithUntil() { void shouldThrowWhenNoCreators() { assertThatThrownBy(TestSchemaProvider::new) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("There should be at least 1 creator"); + .hasMessageStartingWith("There should be at least 1 creator"); } @Test @@ -92,18 +94,10 @@ void shouldThrowWhenNotAscendingMilestonesWithUntil() { assertThatThrownBy( () -> new TestSchemaProvider( - schemaCreator(PHASE0, ALTAIR, (r, c) -> "TestSchema"), - schemaCreator(ALTAIR, (r, c) -> "TestSchema"))) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Milestones ascending ordering error: from ALTAIR to ALTAIR"); - - assertThatThrownBy( - () -> - new TestSchemaProvider( - schemaCreator(ALTAIR, BELLATRIX, (r, c) -> "TestSchema"), - schemaCreator(PHASE0, (r, c) -> "TestSchema"))) + schemaCreator(ALTAIR, (r, c) -> "TestSchema"), + schemaCreator(PHASE0, BELLATRIX, (r, c) -> "TestSchema"))) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Milestones ascending ordering error: from BELLATRIX to PHASE0"); + .hasMessageStartingWith("Creator's milestones must be in order"); } @Test @@ -114,7 +108,7 @@ void shouldThrowWhenNotAscendingMilestonesWithoutUntil() { schemaCreator(PHASE0, (r, c) -> "TestSchema"), schemaCreator(PHASE0, (r, c) -> "TestSchema"))) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Milestones ascending ordering error: from PHASE0 to PHASE0"); + .hasMessageStartingWith("Creator's milestones must be in order"); assertThatThrownBy( () -> @@ -122,18 +116,32 @@ void shouldThrowWhenNotAscendingMilestonesWithoutUntil() { schemaCreator(DENEB, (r, c) -> "TestSchema"), schemaCreator(ALTAIR, (r, c) -> "TestSchema"))) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Milestones ascending ordering error: from DENEB to ALTAIR"); + .hasMessageStartingWith("Creator's milestones must be in order"); } @Test - void shouldThrowWhenDeclaringGaps() { + void shouldThrowWhenWithUntilNotAsLast() { assertThatThrownBy( () -> new TestSchemaProvider( - schemaCreator(PHASE0, (r, c) -> "TestSchema"), - schemaCreator(BELLATRIX, (r, c) -> "TestSchema"))) + schemaCreator(ALTAIR, BELLATRIX, (r, c) -> "TestSchema"), + schemaCreator(CAPELLA, (r, c) -> "TestSchema"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageStartingWith("Only last creator is allowed to use until"); + } + + @Test + void shouldThrowWhenWithUntilIsPriorToMilestone() { + assertThatThrownBy( + () -> + new TestSchemaProvider(schemaCreator(BELLATRIX, BELLATRIX, (r, c) -> "TestSchema"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageStartingWith("Last creator untilMilestone must be greater than milestone in"); + + assertThatThrownBy( + () -> new TestSchemaProvider(schemaCreator(CAPELLA, BELLATRIX, (r, c) -> "TestSchema"))) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Milestones gap detected: from PHASE0 to ALTAIR"); + .hasMessageStartingWith("Last creator untilMilestone must be greater than milestone in"); } private static class TestSchemaProvider extends AbstractSchemaProvider {