Add a mechanism to avoid mixing versions for a client#1167
Add a mechanism to avoid mixing versions for a client#1167sugmanue wants to merge 8 commits intosmithy-lang:mainfrom
Conversation
| ${?versionCheck}static { | ||
| ${versionCheck:C|} | ||
| } | ||
| ${/versionCheck}${#builders}${value:C|} |
There was a problem hiding this comment.
I don’t love doing this during class initialization of code generated classes.
- Failures during class init are painful to debug. The first access shows up as the dreaded ExceptionInInitializerError, and every access after that turns into a NoClassDefFoundError. At that point users have to go spelunking through older logs to find the original failure, which is usually nowhere near the code path they’re currently debugging.
- This also does not work cleanly for cases like Dynamic Client.
My preference would be to add this to the ClientBuilder instead.
| var urls = Thread.currentThread() | ||
| .getContextClassLoader() | ||
| .getResources(VERSIONS_RESOURCE); | ||
| while (urls.hasMoreElements()) { |
There was a problem hiding this comment.
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.
| fun generate() { | ||
| val dir = File(outputDir, "resources/META-INF/smithy-java") | ||
| dir.mkdirs() | ||
| File(dir, "versions.properties").writeText( |
There was a problem hiding this comment.
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 lastCompatibleVersion so 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
|
|
||
| @Benchmark | ||
| public void versionCheckEnabled() { | ||
| VersionCheck.check(VERSION); |
There was a problem hiding this comment.
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.
| // All modules must report the same version. | ||
| var firstVersion = modules.get(0)[1]; | ||
| for (var module : modules) { | ||
| if (!module[1].equals(firstVersion)) { |
There was a problem hiding this comment.
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.
3 participants
Issue #, if available:
Description of changes:
Other SDKs have historically suffered from issues where users inadvertently mix versions of runtime libraries in their final application. This typically happens when different dependencies pull in different versions of the same module, resulting in a classpath that contains, for example, core at version 1.1.0 alongside client-http at version 1.0.9. These mismatches cause subtle runtime errors, class not found exceptions, missing methods, or unexpected behavior, that are difficult to diagnose because nothing fails at compile time.
This change adds a version compatibility test that addresses this at runtime when the codegen schema classes are loaded: if the modules on the classpath don't all agree on the same version, or if any module is older than what the client was generated against, the test fails with a clear message identifying exactly which modules are mismatched, which prevents the classloader from succeeding.
The end-user can set the java property
smithy.java.skipVersionCheckto bypass this check if needed.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.