From 4fe9cfa118b02fde1a42ffe330d80ebb2e4eba13 Mon Sep 17 00:00:00 2001 From: Alexander Rovner Date: Thu, 26 Sep 2024 13:42:56 +0200 Subject: [PATCH] fix bug where ccompat and core api would report different compatibility mode values --- .../rest/v7/impl/ConfigResourceImpl.java | 50 ++++++------ .../rest/CCompatCompatibilityConfigTest.java | 81 +++++++++++++++++++ .../rest/ForwardCompatModeProfile.java | 13 +++ .../ccompat/rest/CCompatTestConstants.java | 2 + 4 files changed, 121 insertions(+), 25 deletions(-) create mode 100644 app/src/test/java/io/apicurio/registry/ccompat/rest/CCompatCompatibilityConfigTest.java create mode 100644 app/src/test/java/io/apicurio/registry/ccompat/rest/ForwardCompatModeProfile.java diff --git a/app/src/main/java/io/apicurio/registry/ccompat/rest/v7/impl/ConfigResourceImpl.java b/app/src/main/java/io/apicurio/registry/ccompat/rest/v7/impl/ConfigResourceImpl.java index fc3f3bfb49..090a9c1fcd 100644 --- a/app/src/main/java/io/apicurio/registry/ccompat/rest/v7/impl/ConfigResourceImpl.java +++ b/app/src/main/java/io/apicurio/registry/ccompat/rest/v7/impl/ConfigResourceImpl.java @@ -28,10 +28,12 @@ import io.apicurio.registry.metrics.health.liveness.ResponseErrorLivenessCheck; import io.apicurio.registry.metrics.health.readiness.ResponseTimeoutReadinessCheck; import io.apicurio.registry.model.GA; +import io.apicurio.registry.rules.RulesProperties; import io.apicurio.registry.rules.compatibility.CompatibilityLevel; import io.apicurio.registry.storage.RuleNotFoundException; import io.apicurio.registry.storage.dto.RuleConfigurationDto; import io.apicurio.registry.types.RuleType; +import jakarta.inject.Inject; import jakarta.interceptor.Interceptors; import java.util.Optional; @@ -45,6 +47,9 @@ @Logged public class ConfigResourceImpl extends AbstractResource implements ConfigResource { + @Inject + RulesProperties rulesProperties; + private CompatibilityLevelParamDto getCompatibilityLevel(Supplier supplyLevel) { try { // We're assuming the configuration == compatibility level @@ -55,26 +60,27 @@ private CompatibilityLevelParamDto getCompatibilityLevel(Supplier supply ) ).get().name()); } catch (RuleNotFoundException ex) { - return new CompatibilityLevelParamDto(CompatibilityLevelDto.Level.NONE.name()); + var compatRuleDto = rulesProperties.getDefaultGlobalRuleConfiguration(RuleType.COMPATIBILITY); + if (compatRuleDto != null) { + // Fallback to the default global rule + return new CompatibilityLevelParamDto(compatRuleDto.getConfiguration()); + } else { + // Fallback to NONE if no default rule is found + return new CompatibilityLevelParamDto(CompatibilityLevelDto.Level.NONE.name()); + } } } private void updateCompatibilityLevel(CompatibilityLevelDto.Level level, - Consumer updater, - Runnable deleter) throws X { - if (level == CompatibilityLevelDto.Level.NONE) { - // delete the rule - deleter.run(); - } else { - String levelString = level.getStringValue(); - try { - CompatibilityLevel.valueOf(levelString); - } catch (IllegalArgumentException ex) { - throw new IllegalArgumentException("Illegal compatibility level: " + levelString); - } - updater.accept(RuleConfigurationDto.builder() - .configuration(levelString).build()); // TODO config should take CompatibilityLevel as param + Consumer updater) throws X { + String levelString = level.getStringValue(); + try { + CompatibilityLevel.valueOf(levelString); + } catch (IllegalArgumentException ex) { + throw new IllegalArgumentException("Illegal compatibility level: " + levelString); } + updater.accept(RuleConfigurationDto.builder() + .configuration(levelString).build()); // TODO config should take CompatibilityLevel as param } @Override @@ -94,8 +100,8 @@ public CompatibilityLevelDto updateGlobalCompatibilityLevel(CompatibilityLevelDt } else { storage.updateGlobalRule(RuleType.COMPATIBILITY, dto); } - }, - () -> storage.deleteGlobalRule(RuleType.COMPATIBILITY)); + } + ); return request; } @@ -112,14 +118,8 @@ public CompatibilityLevelDto updateSubjectCompatibilityLevel(String subject, Com } else { storage.updateArtifactRule(ga.getGroupId(), ga.getArtifactId(), RuleType.COMPATIBILITY, dto); } - }, - () -> { - try { - storage.deleteArtifactRule(ga.getGroupId(), ga.getArtifactId(), RuleType.COMPATIBILITY); - } catch (RuleNotFoundException e) { - //Ignore, fail only when the artifact is not found - } - }); + } + ); return request; } diff --git a/app/src/test/java/io/apicurio/registry/ccompat/rest/CCompatCompatibilityConfigTest.java b/app/src/test/java/io/apicurio/registry/ccompat/rest/CCompatCompatibilityConfigTest.java new file mode 100644 index 0000000000..8ce3e58822 --- /dev/null +++ b/app/src/test/java/io/apicurio/registry/ccompat/rest/CCompatCompatibilityConfigTest.java @@ -0,0 +1,81 @@ +package io.apicurio.registry.ccompat.rest; + +import io.apicurio.registry.AbstractResourceTestBase; +import io.apicurio.registry.ccompat.dto.CompatibilityLevelDto; +import io.apicurio.registry.ccompat.dto.CompatibilityLevelParamDto; +import io.apicurio.registry.rest.v2.beans.Rule; +import io.quarkus.test.junit.QuarkusTest; +import io.quarkus.test.junit.TestProfile; +import jakarta.enterprise.inject.Typed; +import org.jetbrains.annotations.NotNull; +import org.junit.jupiter.api.Test; + +import static io.apicurio.registry.noprofile.ccompat.rest.CCompatTestConstants.CONFIG_NONE; +import static io.restassured.RestAssured.given; +import static org.junit.jupiter.api.Assertions.assertEquals; + +@QuarkusTest +@Typed(CCompatCompatibilityConfigTest.class) +@TestProfile(ForwardCompatModeProfile.class) +public class CCompatCompatibilityConfigTest extends AbstractResourceTestBase { + + @NotNull + public String getCcompatBasePath() { + return "/ccompat/v7"; + } + + @NotNull + public String getCoreBasePath() { + return "/registry/v2"; + } + + CompatibilityLevelDto.Level getCompatibilityLevelFromCoreApi() { + var respCore = given() + .when() + .contentType(ContentTypes.JSON) + .get(getCoreBasePath() + "/admin/rules/COMPATIBILITY") + .then() + .statusCode(200) + .extract().as(Rule.class); + return CompatibilityLevelDto.Level.valueOf(respCore.getConfig()); + } + + CompatibilityLevelDto.Level getCompatibilityLevelFromCcompatApi() { + var respCcompat = given() + .when() + .contentType(ContentTypes.COMPAT_SCHEMA_REGISTRY_STABLE_LATEST) + .get(getCcompatBasePath() + "/config/") + .then() + .statusCode(200) + .extract().as(CompatibilityLevelParamDto.class); + return CompatibilityLevelDto.Level.valueOf(respCcompat.getCompatibilityLevel()); + } + + /** + * Test that the default global compatibility level is the same, no matter if we use + * the core or ccompat endpoint for retrieving its value. + */ + @Test + public void testConfigEndpoints() { + // make sure that the default global "FORWARD" compatibility level is returned by both endpoints + assertEquals(CompatibilityLevelDto.Level.FORWARD, getCompatibilityLevelFromCoreApi()); + assertEquals(getCompatibilityLevelFromCoreApi(), getCompatibilityLevelFromCcompatApi()); + + // change the compatibility level and check that it is the same in both endpoints + given() + .when() + .contentType(ContentTypes.COMPAT_SCHEMA_REGISTRY_STABLE_LATEST) + .body(CONFIG_NONE) + .put(getCcompatBasePath() + "/config") + .then() + .statusCode(200); + + assertEquals(CompatibilityLevelDto.Level.NONE, getCompatibilityLevelFromCoreApi()); + assertEquals(getCompatibilityLevelFromCoreApi(), getCompatibilityLevelFromCcompatApi()); + } + + @Override + protected void deleteGlobalRules(int expectedDefaultRulesCount) { + // Do nothing... (we want to keep the default global rules) + } +} diff --git a/app/src/test/java/io/apicurio/registry/ccompat/rest/ForwardCompatModeProfile.java b/app/src/test/java/io/apicurio/registry/ccompat/rest/ForwardCompatModeProfile.java new file mode 100644 index 0000000000..1b9576bc81 --- /dev/null +++ b/app/src/test/java/io/apicurio/registry/ccompat/rest/ForwardCompatModeProfile.java @@ -0,0 +1,13 @@ +package io.apicurio.registry.ccompat.rest; + +import io.quarkus.test.junit.QuarkusTestProfile; + +import java.util.Map; + +public class ForwardCompatModeProfile implements QuarkusTestProfile { + @Override + public Map getConfigOverrides() { + return Map.of("registry.rules.global.compatibility", "FORWARD", + "registry.auth.enabled", "false"); + } +} diff --git a/app/src/test/java/io/apicurio/registry/noprofile/ccompat/rest/CCompatTestConstants.java b/app/src/test/java/io/apicurio/registry/noprofile/ccompat/rest/CCompatTestConstants.java index 8f388d5a85..493e3f8063 100644 --- a/app/src/test/java/io/apicurio/registry/noprofile/ccompat/rest/CCompatTestConstants.java +++ b/app/src/test/java/io/apicurio/registry/noprofile/ccompat/rest/CCompatTestConstants.java @@ -48,6 +48,8 @@ public class CCompatTestConstants { public static final String CONFIG_BACKWARD = "{\"compatibility\": \"BACKWARD\"}"; + public static final String CONFIG_NONE = "{\"compatibility\": \"NONE\"}"; + public static final String VALID_AVRO_SCHEMA = "{\"schema\": \"{\\\"type\\\": \\\"record\\\",\\\"name\\\": \\\"myrecord1\\\",\\\"fields\\\": [{\\\"name\\\": \\\"foo1\\\",\\\"type\\\": \\\"string\\\"}]}\"}"; public static final String SCHEMA_INVALID_WRAPPED = "{\"schema\":\"{\\\"type\\\": \\\"bloop\\\"}\"}";