Skip to content

MySQL Support & Data classes serialisations - #8872

Open
Voltro1 wants to merge 5 commits into
SkriptLang:masterfrom
Voltro1:feature/mysql
Open

Voltro1 wants to merge 5 commits into
SkriptLang:masterfrom
Voltro1:feature/mysql

Conversation

@Voltro1

@Voltro1 Voltro1 commented Sep 8, 2026

Copy link
Copy Markdown

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

- 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)
@Voltro1
Voltro1 requested review from a team as code owners September 8, 2026 13:48
@Voltro1
Voltro1 requested review from Pesekjak and UnderscoreTud and removed request for a team September 8, 2026 13:48
@skriptlang-automation skriptlang-automation Bot added needs reviews A PR that needs additional reviews needs triage An issue that hasn't been classified or verified yet labels Sep 8, 2026
@skriptlang-automation

skriptlang-automation Bot commented Sep 8, 2026

Copy link
Copy Markdown

🔍 Triage Checklist

Type

  • bug
  • feature
  • enhancement

Project

  • 2.16 Releases
  • 2.17 Releases

  • ✅ Mark as triaged

@bluelhf

bluelhf commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

great that you are interested :)

  • for inventories, are you going to handle things like furnaces where the inventory has to be tied to a specific inventory holder? what happens if the block no longer exists after the variable is loaded again? what about player inventories?
  • it does not seem like a wise architectural choice to hardcode exemptions for the serialisation rules all in one place; perhaps you could focus on storage for those variables which can be serialised and then maybe expand on those once the storage is working?
  • it would be a good idea to document the general architecture of the storage including javadocs
  • is this intended to support multiple servers interacting with the same database? if not, does it lock the database? do you plan on serialising, say, trigger sections as transactions to prevent users having to worry about race conditions?
  • did you see skNetwork? do you think it is better to focus on adding minimal database connection support to skript or contribute to the addon instead?

@Voltro1

Voltro1 commented Sep 8, 2026

Copy link
Copy Markdown
Author

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.
I will probably only focus on fixing yggdrasil for CSV color serialization, and make the sql database follow some regulations (i will try as much as possible to make it data-driven and compact, with no need for specific serialization per data class), but my current end goal changed to extend the save-able values by csv, then implementing them in sql. Not sure if i would work on it again after 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 made it that way as my testing server does not allow me to create databases for some reason, and i think keeping it like this would be benefitial for some rare cases, personally i'd use this.

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
@Voltro1

Voltro1 commented Sep 9, 2026

Copy link
Copy Markdown
Author

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

@bluelhf

bluelhf commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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 none erroneously? It seems at least the documentation is written by an LLM. If you describe the system here in Lebanese Arabic or whichever language is most comfortable for you, it will help us trust that you actually understand the content of the pull request. This feature is not really the kind of thing that state-of-the-art LLMs can implement correctly yet.

@Voltro1

Voltro1 commented Sep 9, 2026

Copy link
Copy Markdown
Author

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
And sure, do you want me to fully explain everything i have done? I could do it in english as i cant really explain it code that well in my language (since ykno, coding is in english and i lowk use it more than arabic atp)

@Voltro1

Voltro1 commented Sep 9, 2026

Copy link
Copy Markdown
Author

Missformed sentence, i meant to say "i didnt use ai PRIOR to creating the pull request"

@bluelhf

bluelhf commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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 And sure, do you want me to fully explain everything i have done? I could do it in english as i cant really explain it code that well in my language (since ykno, coding is in english and i lowk use it more than arabic atp)

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.

@Voltro1

Voltro1 commented Sep 9, 2026

Copy link
Copy Markdown
Author

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.

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
@Voltro1

Voltro1 commented Sep 9, 2026

Copy link
Copy Markdown
Author

I assume you marked AI assistance as none erroneously? It seems at least the documentation is written by an LLM. If you describe the system here in Lebanese Arabic or whichever language is most comfortable for you, it will help us trust that you actually understand the content of the pull request. This feature is not really the kind of thing that state-of-the-art LLMs can implement correctly yet.

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
If this pr would be accepted, i will work on the rework here, otherwise ill work on a different branch

- 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 sovdeeth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +300 to +325
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be using the existing databases: section for variables databases, not new mysql sections.

Comment on lines +223 to +242
// 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.");
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned in config file, sql should act no differently from all the other databases.

Comment on lines +961 to +973
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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1033 to +1044
if (optionalMySQLActive) {
boolean interrupted = false;
while (saveThread.isAlive()) {
try {
saveThread.join();
} catch (InterruptedException e) {
interrupted = true;
}
}
if (interrupted)
Thread.currentThread().interrupt();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go in the Color file

Comment on lines +31 to +39
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());
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread persistence-test.sk

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's going on with this file? did you commit it by accident? why is it outside the src tree?

Comment thread skript-aliases

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're using this wrong aliases, this pr should not be touching skript-aliases.

@sovdeeth

Copy link
Copy Markdown
Member

I would still like the colour serializer changes to be put in a separate pr from the sql changes though
they are unrelated and do not depend on each other, so separating them would mean reviews for one don't have to deal or wait for changes to the other.

@Voltro1

Voltro1 commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thats alot to account for, i will work on them soon
I will make a separate PR for the yggdrasil & color changes, albeit might start from scratch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs reviews A PR that needs additional reviews needs triage An issue that hasn't been classified or verified yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants