Conversation
- Removed SQLibrary soft dependency - Implemented Jdbc in order to handle mysql - Currently saves serializable data classes FURTHER TESTING IS REQUIRED Next steps is to allow saving unserializable data classes on both SQL and CSV
- Colors, vectors, locations etc... are now save-able - Slight tweaks to the database schema - Fixed config.sk's database table field being completely ignored - More testing was done, confirmed suspicions and fixes applied - Some progress on inventory saving for sql Future plans prior to releasing this: - Saving inventories - Making unserializable data classes save in CSV (hopefully) - Polishing, refining and possible better reimplementation of this (probably wont)
🔍 Triage ChecklistType
Project
|
|
great that you are interested :)
|
|
I had a small conversation regarding inventories with some members of the team on Skript's discord, safe to say i will not be working on inventories for the time being. Yes the current architecture is not perfect, its just a placeholder rn used to make sure the result is achievable, the pull request was made to let others know that i am indeed working on this. Documentation and comments will be added in due time. The implementation is basic, all depends on how the user configures it, the config allows for the use of various tables so the same hosted database could be used by multiple servers I have not seen skNetwork. |
- Gave up and removed Inventory related efforts - Fixed yggdrasil superclass lookup issue, which caused some classes to not be serializable, most notibaly colors & some particles - Deleted most of the prior implemented function thus sql operations use the already implemented serializers - Removed the "short_value" field from the database and changed ordering - Added a serializer for rgb & skript colors so no warning pops up when trying to save a color - Cleared up some bloat - Added architecture documentation
|
Ok so i believe im done, i stepped back a bit and ended up only making mysql support & fixing a yggdrasil issue so now colors and probably(?) some other classes are also serialisable and saved in both csv & sql |
|
Were the previous MySQL implementations actually shipped? I don't necessarily think it's important to support variables saved by the old SQLibrary format since it hasn't been supported in years. I assume you marked AI assistance as |
|
I didnt use AI when creating the pull request, i did for the documentation (i got lazy) and i dont think i can change the original message And im sorry, what are you referring to with "old mysql implementation" as i could think of 3 things rn |
|
Missformed sentence, i meant to say "i didnt use ai PRIOR to creating the pull request" |
Okay :) Re.: MySQL, for practical purposes all three of them. We don't need migrations for database schemas that have only ever been used by you while making this pull request, and we also don't need migrations for the old SQLStorage schema that currently exists in mainline Skript either. This is because no Skript users actually have data with these schemas; you already migrated your own database, and Skript's old MySQL schema hasn't been supported since SQLibrary 7.1 broke in Spigot 1.17. Ideally, the javadocs for each class would contain extensive documentation about the purpose of that class and how it relates to the SQL storage system as a whole; it doesn't need to document every implementation detail, but it should be written so that a person who is reading the code gets an idea of the purpose of that class and—after reading enough javadocs—understands the SQL storage altogether. English is excellent, and it's okay to use AI to fix spelling mistakes and grammatical errors :) I recommend you write the content yourself though, as AI is very bad at writing prudent javadocs and often includes unnecessary detail about what had been wrong previously and omits crucial information about the design. |
I will get these done then get back to you |
- Removed accidentally left-in code that helped me with testing, and migration - Revisited & rewritten the architecture documentation
The SQL implementation uses Skript’s existing serialization system. Skript turns each variable into a name, type, and bytes before passing it to the backend. SQL doesn’t need to know how types work. The MySQL backend saves changes in batches on a background thread. Before writing a batch, it records the pending changes in a recovery file, named mysql-pending.bin. If the database becomes unavailable for whatever reason, those changes can be retried. It also attempts to flush pending changes during shutdown. The same file handles deletions of variables periodically. For colors, the main problem was that 'Color' covers two different representations: 'SkriptColor' and 'ColorRGB' which are different classes (enum & object). I registered them separately so Skript uses the correct serialization format for each. Named colors still go through Yggdrasil’s built-in enum handling, and ColorRGB stores its RGBA value. I also fixed a separate bug in Yggdrasil’s superclass lookup: it was checking the map entry’s class instead of the registered class. The color registrations fixes the enum/object mismatch, the lookup correction fixes superclass resolution generally. These changes apply to shared serialization, so they benefit both CSV and SQL. I hope this is what you wanted :> Also, i will be reworking the whole yggdrasil & colors handling in either a separate PR or continue working on this one, depending on the feedback for this, as sovde requested |
- Now using Paper's provided MySQL & SQLite drivers, reducing the plugin size from 19MB to 5MB - Removed private MySQL driver cleanup and update checks - Fixed class-ID resolution and villager profession serialization headers - Corrected banner test aliases and added the wind-charge entity alias - Improved MySQL startup and schema validation feedback - Expanded serialization and custom-table regression coverage - Added a manual persistence, restart, deletion, and outage test script
sovdeeth
left a comment
There was a problem hiding this comment.
didn't look at the sql details, since i have no experience there. Haven't checked the tests yet either.
Added some design concerns, though, and I would also say that #8420 seems like a more comprehensive and versatile update to variable storage, so you may want to pull from it instead of the current design that seems to have a lot of little carve-outs for sql behavior instead of a clean variablestorage interface.
| mysql: | ||
| # Optional utilisation of a MySQL database. | ||
| # Disabled means the existing storage behaves exactly as before (ie a CSV file). | ||
| # IF ENABLED, DO NOT DELETE ANY MYSQL/BIN FILES GENERATED BY THE PLUGIN (mysql-pending.bin)!!!!! | ||
| # Restart required for change to take effect. | ||
| # MySQL 8+ is required. | ||
| # Uses the same ClassInfo serializers and serialized values as normal Skript storage. | ||
| # List variables are saved as individual leaves. Unsupported values are not written. | ||
| # Unreadable rows are retained for recovery; other variables still load. | ||
| # Existing tables must match the current schema; no automatic schema migrations are performed. | ||
| enabled: false | ||
| pattern: .* | ||
| host: localhost | ||
| port: 3306 | ||
| database: skript | ||
| user: skript | ||
| password: change-me | ||
| table: skript_vars | ||
| # TLS modes: VERIFY_IDENTITY, VERIFY_CA, REQUIRED, DISABLED. | ||
| ssl mode: REQUIRED | ||
| # Invalid settings or startup failures explicitly fall back to databases below. | ||
| # Pending writes are retained in plugins/Skript/mysql-pending.bin on shutdown. | ||
| # Keep this recovery file with its database; changing the target while it contains | ||
| # pending writes is rejected. Do not delete it to work around a database outage. | ||
| # Like CSV, a sudden crash can lose changes that have not reached the writer yet. | ||
|
|
There was a problem hiding this comment.
we should be using the existing databases: section for variables databases, not new mysql sections.
| // Select the optional backend before opening any legacy storage. Loading both | ||
| // would trigger the existing automatic redistribution (including source deletion). | ||
| Node mysql = config.getMainNode().get("mysql"); | ||
| if (mysql instanceof SectionNode mysqlConfig) { | ||
| String enabled = mysqlConfig.getValue("enabled"); | ||
| if ("true".equalsIgnoreCase(enabled)) { | ||
| PooledMySQLStorage storage = new PooledMySQLStorage(); | ||
| if (storage.load(mysqlConfig)) { | ||
| STORAGES.add(storage); | ||
| optionalMySQLActive = true; | ||
| Skript.info("MySQL enabled, loading variables from database; Other databases are left untouched."); | ||
| return true; | ||
| } | ||
| Skript.error("MySQL initialization failed. Falling back to the default database. " | ||
| + "MySQL data and pending recovery data have been left intact."); | ||
| } else if (enabled != null && !"false".equalsIgnoreCase(enabled)) { | ||
| Skript.error("mysql.enabled must be true or false. Using the configured databases."); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
as mentioned in config file, sql should act no differently from all the other databases.
| try { | ||
| SerializedVariable.Value serialized = serialize(value); | ||
| if (value == null || serialized != null) { | ||
| serializationFailures.remove(name); | ||
| return new SerializedVariable(name, serialized); | ||
| } | ||
| } catch (Exception | LinkageError e) { | ||
| // Report once per variable below; keep the previous persisted value intact. | ||
| } | ||
| if (serializationFailures.add(name)) | ||
| Skript.error("Cannot persist variable {" + name + "}; its type or a nested value has no usable serializer. " | ||
| + "It remains in memory and its last saved value is unchanged."); | ||
| return null; |
There was a problem hiding this comment.
This handling isn't necessary. Exceptions thrown at this point are not based on not having a serializer (this will just return null if it's non-serializable). Erroring for every non-serializable value would be extremely annoying for users (consider set {temp} to a chest inventory with 3 rows) and isn't very helpful.
| if (optionalMySQLActive) { | ||
| boolean interrupted = false; | ||
| while (saveThread.isAlive()) { | ||
| try { | ||
| saveThread.join(); | ||
| } catch (InterruptedException e) { | ||
| interrupted = true; | ||
| } | ||
| } | ||
| if (interrupted) | ||
| Thread.currentThread().interrupt(); | ||
| } |
There was a problem hiding this comment.
again, there should be no special handling for sql, the goal is that this class should have no clue that sql exists or doesn't exist.
| * Enum implementations must use this object envelope too, otherwise Classes would | ||
| * reconstruct a Color object header for a payload written with an enum header. | ||
| */ | ||
| public final class ColorSerializer extends Serializer<Color> { |
There was a problem hiding this comment.
this should go in the Color file
| if (color instanceof ColorRGB rgb) { | ||
| fields = rgb.serialize(); | ||
| implementation = "rgb"; | ||
| } else if (color instanceof SkriptColor skriptColor) { | ||
| fields = named.serialize(skriptColor); | ||
| implementation = "named"; | ||
| } else { | ||
| throw new NotSerializableException("Unsupported Color implementation: " + color.getClass().getName()); | ||
| } |
There was a problem hiding this comment.
this is a pretty awkward solution, as it doesn't handle inheritance very nicely (no custom impls allowed), but i suppose it does work ok. I wouldn't call this a final solution though, we should still aim for improving yggsdrasil behavior outside of bandaids like this. Out of scope for this pr though.
There was a problem hiding this comment.
what's going on with this file? did you commit it by accident? why is it outside the src tree?
There was a problem hiding this comment.
you're using this wrong aliases, this pr should not be touching skript-aliases.
|
I would still like the colour serializer changes to be put in a separate pr from the sql changes though |
|
Thats alot to account for, i will work on them soon |
Problem
I saw this on discord with months of no attempts, so i took it upon myself because why not?
As for the serialisation part, its annoying to deal with
Solution
Removing the no-longer-functional SQLibrary soft-dependency and implementing an option from within
Testing Completed
just typical save & broadcast
Confirmed working: int, floats, strings, chars, items, itemstacks, locations, worlds, vectors, particles, bukkit colors, arrays/lists.
Supporting Information
!! Using this optional feature will generate a bin file used for storing pending variable saves that will be accessed on sudden server stops (thus, saving variables that may or may not have already been saved)
Most of the work is on sql, inventories are still not supported but im working on them.
Planning to make unserializable datatypes work on CSV too in due time.
Completes: this should be revised prior to any confirmation and merging.
Related: i dont think anything?
AI assistance: none
THIS IS NOT COMPLETE YET