Implement configuration migration system and update ABI checks#270
Implement configuration migration system and update ABI checks#270twisti-dev merged 6 commits intoversion/1.21.11from
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c137dde216
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| configFolder: Path, | ||
| configFileName: @JsonConfigFileNamePattern String, | ||
| ): SpongeConfigManager<C> = | ||
| createSpongeYmlConfigManager(configClass, configFolder, configFileName, ConfigMigrationBuilder()) |
There was a problem hiding this comment.
The new 3-argument createSpongeJsonConfigManager overload calls createSpongeYmlConfigManager(...) instead of the JSON variant. Any code path that uses the JSON convenience API without explicitly passing migrations (including createSpongeJsonConfig in the implementation) will construct a YAML-backed manager for a JSON config file, causing wrong parser/serializer behavior and unexpected file format changes. This overload should delegate to createSpongeJsonConfigManager(..., ConfigMigrationBuilder()).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Introduces a versioned migration system for Sponge/Configurate-backed configuration files, wiring migrations into the config manager APIs and adding an example usage in the Bukkit test plugin. Also updates CI workflows to use the newer Kotlin ABI check/update commands.
Changes:
- Added
ConfigMigration+ConfigMigrationBuilderand integrated migration application intoSpongeConfigManagerload/reload flows. - Extended
SurfConfigApi/SurfConfigApiImplandSponge*ConfigClasswrappers to support passing/declaring migrations. - Updated GitHub Actions workflows to run
checkKotlinAbi/updateKotlinAbi.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| surf-api-core/surf-api-core-server/src/main/kotlin/dev/slne/surf/surfapi/core/server/config/SurfConfigApiImpl.kt | Plumbs migration builder into YAML/JSON manager creation. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/migration/ConfigMigrationBuilder.kt | New builder for collecting/applying versioned migrations. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/migration/ConfigMigration.kt | New migration step abstraction. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/manager/SpongeConfigManager.kt | Applies migrations on initial load and on reload; adds runtime migration registration helpers. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/SurfConfigApi.kt | Adds migration-aware overloads for config manager creation. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/SpongeYmlConfigClass.kt | Lazily creates manager so companion-init migrations can be registered first. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/SpongeJsonConfigClass.kt | Same as YAML variant, for JSON. |
| surf-api-core/surf-api-core-api/src/main/kotlin/dev/slne/surf/surfapi/core/api/config/SpongeConfigClass.kt | Adds migration registration/version key helpers; ensures init() triggers manager initialization. |
| surf-api-core/surf-api-core-api/api/surf-api-core-api.api | Updates ABI dump for new public API surface. |
| surf-api-bukkit/surf-api-bukkit-plugin-test/src/main/kotlin/dev/slne/surf/surfapi/bukkit/test/config/MyPluginConfig.kt | Adds an example config showing migrations in a companion object. |
| surf-api-bukkit/surf-api-bukkit-plugin-test/src/main/kotlin/dev/slne/surf/surfapi/bukkit/test/BukkitPluginMain.kt | Initializes the new example config on plugin enable. |
| .github/workflows/publish.yml | Switches CI ABI check task to checkKotlinAbi. |
| .github/workflows/api-dump-version.yml | Switches ABI check/update guidance to checkKotlinAbi/updateKotlinAbi. |
| configFolder: Path, | ||
| configFileName: @JsonConfigFileNamePattern String, | ||
| ): SpongeConfigManager<C> = | ||
| createSpongeYmlConfigManager(configClass, configFolder, configFileName, ConfigMigrationBuilder()) |
There was a problem hiding this comment.
createSpongeJsonConfigManager's default implementation delegates to createSpongeYmlConfigManager(...), which will create a YAML manager for a JSON filename/pattern. This should delegate to createSpongeJsonConfigManager(...) instead (and pass the migrations builder).
| createSpongeYmlConfigManager(configClass, configFolder, configFileName, ConfigMigrationBuilder()) | |
| createSpongeJsonConfigManager(configClass, configFolder, configFileName, ConfigMigrationBuilder()) |
| * The version key defaults to `"_config_version"` at the root of the config file. | ||
| * This can be customized via [versionKey]. |
There was a problem hiding this comment.
This KDoc says the version key defaults to "_config_version", but the versionKey(...) method KDoc later says it defaults to "config-version". Align the documentation (and ensure it matches DEFAULT_VERSION_KEY).
| * @param node the root configuration node to migrate | ||
| * @return the version the node was migrated from, or `-1` if unversioned | ||
| * @throws ConfigurateException if a migration fails | ||
| */ | ||
| @Throws(ConfigurateException::class) | ||
| fun migrate(node: ConfigurationNode): MigrationResult { | ||
| val transformation = buildTransformation() | ||
| ?: return MigrationResult(fromVersion = -1, toVersion = -1, migrated = false) |
There was a problem hiding this comment.
The migrate(...) KDoc still describes returning an Int ("the version the node was migrated from"), but the method now returns MigrationResult. Update the @return documentation to match the current return type/semantics.
| import dev.slne.surf.surfapi.core.api.config.manager.SpongeConfigManager.Companion.json | ||
| import dev.slne.surf.surfapi.core.api.config.manager.SpongeConfigManager.Companion.yaml |
There was a problem hiding this comment.
These imports of SpongeConfigManager.Companion.json/yaml are unused (the only references are the imports themselves). Removing them avoids IDE/compiler warnings and keeps the import list clean.
| import dev.slne.surf.surfapi.core.api.config.manager.SpongeConfigManager.Companion.json | |
| import dev.slne.surf.surfapi.core.api.config.manager.SpongeConfigManager.Companion.yaml |
| migration(3) { node -> | ||
| // inline migration: rename maxPlayers default | ||
| val mp = node.node("max-players") | ||
| if (!mp.virtual() && mp.getInt(0) == 0) { | ||
| mp.set(100) |
There was a problem hiding this comment.
The example YAML/migration uses kebab-case keys (max-players), but Configurate object mapping will use the Kotlin property name (maxPlayers) unless annotated (e.g., @Setting("max-players")). As written, this example/migration likely won't match the serialized keys; update the keys/migration paths or add @Setting so the demo reflects actual behavior.
| fun init() { | ||
| manager | ||
| } |
There was a problem hiding this comment.
init() is documented as a no-op, but it now forces manager initialization (which can load/save the config and run migrations). Consider updating the KDoc to reflect the new behavior so callers understand the side effects.
| val reloadedNode: ConfigurationNode = loader.load() | ||
|
|
||
| // Apply migrations before deserializing | ||
| applyMigrations(reloadedNode) | ||
|
|
There was a problem hiding this comment.
reloadFromFile() applies migrations to reloadedNode, but the method later persists the manager's stored node instance (loaded at construction) rather than the migrated reloadedNode. If migrations run during reload (e.g., new migrations added at runtime), this can overwrite the migrated version key/structure with stale state. Consider keeping the internal node in sync with the freshly loaded node (e.g., make node mutable and replace it on reload, or save the migrated reloadedNode after setting the reloaded config).
| * Defaults to `"config-version"`. Call this in the `init` block before | ||
| * any migration registrations if you need a custom key. |
There was a problem hiding this comment.
The docs say the version key defaults to "config-version", but ConfigMigrationBuilder currently defaults to ConfigMigrationBuilder.DEFAULT_VERSION_KEY ("_config_version"). Update the KDoc here (or change the default) so consumers know which key is actually written/read.
| * Defaults to `"config-version"`. Call this in the `init` block before | |
| * any migration registrations if you need a custom key. | |
| * By default, this delegates to [ConfigMigrationBuilder.versionKey], which uses | |
| * [ConfigMigrationBuilder.DEFAULT_VERSION_KEY] (currently `"_config_version"`) as the key. | |
| * Call this in the `init` block before any migration registrations if you need a custom key. |
| * Sets the path in the config file where the version number is stored. | ||
| * | ||
| * Defaults to `"config-version"` (a single root-level key). | ||
| * | ||
| * @param path the path components to the version key | ||
| * @return this builder for chaining | ||
| */ | ||
| fun versionKey(vararg path: Any): ConfigMigrationBuilder { | ||
| versionKeyPath = path | ||
| return this |
There was a problem hiding this comment.
The versionKey(...) KDoc states the default is "config-version", but the implementation defaults to DEFAULT_VERSION_KEY ("_config_version"). Please update the documentation (or the default) so they match.
This pull request introduces a new versioned migration system for Sponge-based configuration classes, enabling safe and incremental updates to config file schemas. It adds a migration builder to the core config abstractions, updates the config manager APIs to support migrations, and demonstrates usage with a new example config class. Additionally, workflow scripts are updated to use the new ABI check and update commands.
Config migration system enhancements:
Introduced the
ConfigMigrationandConfigMigrationBuilderclasses, allowing registration and management of versioned migrations for configuration files. TheSpongeConfigClassbase class now supports migration registration via themigrationandversionKeymethods, and exposes themigrationBuilderproperty for advanced use. [1] [2] [3] [4] [5] [6]Updated the
SpongeConfigManagerandSurfConfigApiAPIs to accept migration builders and expose migration-related methods, ensuring that config managers can apply registered migrations when loading or upgrading configs. [1] [2] [3] [4]Documentation and usage examples:
Expanded documentation for
SpongeConfigClassandSpongeJsonConfigClassto explain migration registration and usage patterns, including code examples for registering migrations in companion objects. [1] [2] [3]Added a new example config class,
MyPluginConfig, in the test plugin, demonstrating how to define and migrate configuration schemas using the new migration system. [1] [2] [3]Workflow updates:
checkKotlinAbiandupdateKotlinAbiinstead of the legacy ABI commands, aligning workflow automation with the latest build tooling. [1] [2] [3] [4]