-
Notifications
You must be signed in to change notification settings - Fork 32
Add a mechanism to avoid mixing versions for a client #1167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c5336fa
0558dc9
a632ec0
2bba888
fc4d2e1
9a07ab1
5500854
e7dc082
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import org.gradle.api.DefaultTask | ||
| import org.gradle.api.tasks.Input | ||
| import org.gradle.api.tasks.OutputDirectory | ||
| import org.gradle.api.tasks.TaskAction | ||
| import java.io.File | ||
|
|
||
| /** | ||
| * Gradle task that generates a version marker resource for a module. | ||
| * | ||
| * Each module gets a {@code META-INF/smithy-java/versions.properties} file | ||
| * containing the module name and version. At test time, all such files are | ||
| * discovered via {@code ClassLoader.getResources()} to validate version consistency. | ||
| */ | ||
| abstract class GenerateVersionProviderTask : DefaultTask() { | ||
|
|
||
| @get:Input | ||
| var moduleName: String = "" | ||
|
|
||
| @get:Input | ||
| var moduleVersion: String = "" | ||
|
|
||
| @get:OutputDirectory | ||
| var outputDir: File = project.layout.buildDirectory.dir("generated/version-provider").get().asFile | ||
|
|
||
| @TaskAction | ||
| fun generate() { | ||
| val dir = File(outputDir, "resources/META-INF/smithy-java") | ||
| dir.mkdirs() | ||
| File(dir, "versions.properties").writeText( | ||
| "module=$moduleName\nversion=$moduleVersion\n" | ||
| ) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| import software.amazon.smithy.java.codegen.JavaCodegenSettings; | ||
| import software.amazon.smithy.java.codegen.generators.SchemaFieldOrder.SchemaField; | ||
| import software.amazon.smithy.java.codegen.writer.JavaWriter; | ||
| import software.amazon.smithy.java.core.Version; | ||
| import software.amazon.smithy.java.core.VersionCheck; | ||
| import software.amazon.smithy.java.core.schema.Schema; | ||
| import software.amazon.smithy.java.core.schema.SchemaBuilder; | ||
| import software.amazon.smithy.model.Model; | ||
|
|
@@ -51,9 +53,12 @@ public final class SchemasGenerator | |
| public void accept(CustomizeDirective<CodeGenerationContext, JavaCodegenSettings> directive) { | ||
|
|
||
| var order = directive.context().schemaFieldOrder(); | ||
| boolean first = true; | ||
| for (var shapeOrder : order.partitions()) { | ||
| var className = shapeOrder.getFirst().classRef().className(); | ||
| var fileName = CodegenUtils.getJavaFilePath(directive.settings(), "model", className); | ||
| boolean isFirst = first; | ||
| first = false; | ||
| directive.context() | ||
| .writerDelegator() | ||
| .useFileWriter(fileName, CodegenUtils.getModelNamespace(directive.settings()), writer -> { | ||
|
|
@@ -63,7 +68,10 @@ public void accept(CustomizeDirective<CodeGenerationContext, JavaCodegenSettings | |
| * Defines schemas for shapes in the model package. | ||
| */ | ||
| final class ${className:L} { | ||
| ${#builders}${value:C|} | ||
| ${?versionCheck}static { | ||
| ${versionCheck:C|} | ||
| } | ||
| ${/versionCheck}${#builders}${value:C|} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t love doing this during class initialization of code generated classes.
My preference would be to add this to the ClientBuilder instead. |
||
| ${/builders}${schemas:C|} | ||
| ${#resolvers} | ||
| ${value:C|} | ||
|
|
@@ -97,6 +105,12 @@ final class ${className:L} { | |
| writer.putContext("schemas", schemas); | ||
| writer.putContext("builders", builders); | ||
| writer.putContext("resolvers", resolvers); | ||
| if (isFirst) { | ||
| writer.putContext("versionCheck", | ||
| (Runnable) () -> writer.write("$T.check($S);", | ||
| VersionCheck.class, | ||
| Version.VERSION)); | ||
| } | ||
| writer.write(template); | ||
| writer.popState(); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package software.amazon.smithy.java.core; | ||
|
|
||
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
| import org.openjdk.jmh.annotations.Benchmark; | ||
| import org.openjdk.jmh.annotations.BenchmarkMode; | ||
| import org.openjdk.jmh.annotations.Fork; | ||
| import org.openjdk.jmh.annotations.Measurement; | ||
| import org.openjdk.jmh.annotations.Mode; | ||
| import org.openjdk.jmh.annotations.OutputTimeUnit; | ||
| import org.openjdk.jmh.annotations.Scope; | ||
| import org.openjdk.jmh.annotations.Setup; | ||
| import org.openjdk.jmh.annotations.State; | ||
| import org.openjdk.jmh.annotations.Warmup; | ||
|
|
||
| /** | ||
| * Measures the startup cost of the version compatibility check. | ||
| * | ||
| * <p>In production, {@code VersionCheck.check()} runs exactly once during class | ||
| * initialization. This benchmark measures the per-invocation cost to quantify | ||
| * the one-time startup impact. | ||
| */ | ||
| @State(Scope.Benchmark) | ||
| @OutputTimeUnit(TimeUnit.MICROSECONDS) | ||
| @BenchmarkMode(Mode.AverageTime) | ||
| @Warmup(iterations = 3, time = 1) | ||
| @Measurement(iterations = 5, time = 1) | ||
| @Fork(1) | ||
| public class VersionCheckBench { | ||
|
|
||
| private static final String VERSION = Version.VERSION; | ||
|
|
||
| @Setup | ||
| @SuppressFBWarnings(value = "LG_LOST_LOGGER_DUE_TO_WEAK_REFERENCE", justification = "Intentional for benchmark") | ||
| public void setup() { | ||
| Logger.getLogger(VersionCheck.class.getName()).setLevel(Level.OFF); | ||
| } | ||
|
|
||
| @Benchmark | ||
| public void versionCheckEnabled() { | ||
| VersionCheck.check(VERSION); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is truly measuring what we want it to measure which is how much does this add to the cold start cost. |
||
| } | ||
|
|
||
| @Benchmark | ||
| public void versionCheckSkipped() { | ||
| System.setProperty("smithy.java.skipVersionCheck", "true"); | ||
| try { | ||
| VersionCheck.check(VERSION); | ||
| } finally { | ||
| System.clearProperty("smithy.java.skipVersionCheck"); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package software.amazon.smithy.java.core; | ||
|
|
||
| /** | ||
| * Thrown when incompatible versions of Smithy Java modules are detected on the classpath. | ||
| */ | ||
| public final class IncompatibleVersionException extends RuntimeException { | ||
| IncompatibleVersionException(String message) { | ||
| super(message); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package software.amazon.smithy.java.core; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Properties; | ||
| import software.amazon.smithy.java.logging.InternalLogger; | ||
|
|
||
| /** | ||
| * Validates that all Smithy Java modules on the classpath have compatible versions. | ||
| * | ||
| * <p>Mixing different versions of Smithy Java modules in the same application can cause | ||
| * subtle runtime errors such as missing methods, class not found exceptions, or unexpected | ||
| * behavior that are difficult to diagnose. This commonly happens when different dependencies | ||
| * pull in different versions of the same module transitively. This check detects such | ||
| * mismatches early, at class-load time, before any operation is executed. | ||
| * | ||
| * <p>This check runs once during class initialization of generated code. It discovers | ||
| * all {@code META-INF/smithy-java/versions.properties} resources on the classpath and | ||
| * verifies that all modules report the same version and that all versions are at least | ||
| * as new as the version the code was generated against. | ||
| * | ||
| * <p>The check can be disabled by setting the system property | ||
| * {@code smithy.java.skipVersionCheck} to {@code true}. | ||
| */ | ||
| public final class VersionCheck { | ||
| private static final InternalLogger LOGGER = InternalLogger.getLogger(VersionCheck.class); | ||
| private static final String VERSIONS_RESOURCE = "META-INF/smithy-java/versions.properties"; | ||
| private static final String SKIP_PROPERTY = "smithy.java.skipVersionCheck"; | ||
|
|
||
| private VersionCheck() {} | ||
|
|
||
| /** | ||
| * Validates version compatibility of all Smithy Java modules on the classpath. | ||
| * | ||
| * @param codegenVersion the version the code was generated against | ||
| * @throws IncompatibleVersionException if a version mismatch is detected | ||
| */ | ||
| public static void check(String codegenVersion) { | ||
| if (Boolean.getBoolean(SKIP_PROPERTY)) { | ||
| LOGGER.warn("Smithy Java version compatibility check is disabled via '{}'. " | ||
| + "This is not recommended and should only be used as a temporary workaround. " | ||
| + "Running with mismatched module versions may cause unexpected runtime errors.", | ||
| SKIP_PROPERTY); | ||
| return; | ||
| } | ||
|
|
||
| var modules = new ArrayList<String[]>(); | ||
| try { | ||
| var urls = Thread.currentThread() | ||
| .getContextClassLoader() | ||
| .getResources(VERSIONS_RESOURCE); | ||
| while (urls.hasMoreElements()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can do a much cheaper check before scanning the classpath by comparing this version to the core's Version.VERSION, since in most cases if core doesn't match with the codegen version others won't match either. |
||
| var props = new Properties(); | ||
| try (var is = urls.nextElement().openStream()) { | ||
| props.load(is); | ||
| } | ||
| modules.add(new String[] { | ||
| props.getProperty("module", "unknown"), | ||
| props.getProperty("version", "unknown") | ||
| }); | ||
| } | ||
| } catch (IOException e) { | ||
| // Don't fail startup if we can't read version resources. | ||
| return; | ||
| } | ||
|
|
||
| if (modules.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| var errors = new ArrayList<String>(); | ||
|
|
||
| // All modules must report the same version. | ||
| var firstVersion = modules.get(0)[1]; | ||
| for (var module : modules) { | ||
| if (!module[1].equals(firstVersion)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a little hesitant of strict version equality specially because we follow semver and this would complicate customers deploying security patches. Like if there is a security issue in one of the modules, we force people to upgrade all of them. Not saying that might be a bad thing, just letting you know the ramifications of this decision. |
||
| errors.add("Version mismatch: module '" + modules.get(0)[0] + "' has version " | ||
| + firstVersion + " but module '" + module[0] + "' has version " + module[1]); | ||
| } | ||
| } | ||
|
|
||
| // All module versions must be >= the codegen version. | ||
| for (var module : modules) { | ||
| if (compareVersions(module[1], codegenVersion) < 0) { | ||
| errors.add("Module '" + module[0] + "' version " + module[1] | ||
| + " is older than the codegen version " + codegenVersion); | ||
| } | ||
| } | ||
|
|
||
| if (!errors.isEmpty()) { | ||
| // Build a nice error message to give the end-user all the details needed | ||
| // to fix the issue. | ||
| var sb = new StringBuilder("Smithy Java version compatibility check failed:\n"); | ||
| sb.append(" Generated with version: ").append(codegenVersion).append("\n"); | ||
| sb.append(" Modules on classpath:\n"); | ||
| for (var module : modules) { | ||
| sb.append(" - ").append(module[0]).append(" = ").append(module[1]).append("\n"); | ||
| } | ||
| sb.append(" Issues:\n"); | ||
| for (var error : errors) { | ||
| sb.append(" - ").append(error).append("\n"); | ||
| } | ||
| sb.append(" Fix: Align all smithy-java dependencies to the same version. ") | ||
| .append("If using Gradle, consider importing the BOM: ") | ||
| .append("platform('software.amazon.smithy.java:bom:") | ||
| .append(codegenVersion) | ||
| .append("')"); | ||
| throw new IncompatibleVersionException(sb.toString()); | ||
| } | ||
| } | ||
|
|
||
| private static int compareVersions(String v1, String v2) { | ||
| var parts1 = v1.split("[.\\-]"); | ||
| var parts2 = v2.split("[.\\-]"); | ||
| var len = Math.max(parts1.length, parts2.length); | ||
| for (int i = 0; i < len; i++) { | ||
| var p1 = i < parts1.length ? parsePart(parts1[i]) : 0; | ||
| var p2 = i < parts2.length ? parsePart(parts2[i]) : 0; | ||
| if (p1 != p2) { | ||
| return Integer.compare(p1, p2); | ||
| } | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| private static int parsePart(String part) { | ||
| try { | ||
| return Integer.parseInt(part); | ||
| } catch (NumberFormatException e) { | ||
| return 0; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought: I think SPIs would be a better fit than a versions.properties file, especially for shaded/fat-jar cases.
With a fat jar, all the versions.properties files can get collapsed into a single file, and then you’re in “last writer wins” territory. That’s pretty fragile and also hard to notice when it breaks.
An SPI avoids that class of problem. Each dependency contributes its own service entry, and most common shading tools already know how to merge service files correctly, including Gradle Shadow and maven-shade-plugin. So instead of relying on resource overwrite behavior, we get a model that composes much more naturally under shading.
An SPI also allows us to do fancy stuff like declare methods like
lastCompatibleVersionso that we can have more complex checks than version equality which would be a hedge against https://github.com/smithy-lang/smithy-java/pull/1167/changes#r3211780970