Skip to content
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

Removes obsolete network settings from system.xml #5482

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented Mar 11, 2021

Manually migrates properties that no longer exist in the ServerConfiguration class, and moves them into the NetworkConfiguration object before saving them to network.xml.

Works with strings, int, bool, and string[]

Also removed network properties from NetworkConfiguration that did not make the 10.7 cut.

@BaronGreenback BaronGreenback changed the title Cleanup of system.xml and better small migration. Cleanup of system.xml and better network migration. Mar 11, 2021
@BaronGreenback BaronGreenback marked this pull request as ready for review Mar 11, 2021
@BaronGreenback BaronGreenback requested review from cvium and crobibero Mar 11, 2021
@BaronGreenback BaronGreenback force-pushed the BaronGreenback:SettingsMigration branch from 85735a7 to 8348145 Mar 11, 2021
@BaronGreenback BaronGreenback changed the title Cleanup of system.xml and better network migration. Removes obsolete network settings from system.xml Mar 11, 2021
@@ -647,5 +654,78 @@ private static string NormalizeCommandLineArgument(string arg)

return "\"" + arg + "\"";
}

private static void MigrateNetworkSettings(ILogger logger, ServerApplicationPaths appPaths)

This comment has been minimized.

@cvium

cvium Mar 13, 2021
Member

This should really be in a separate file. I also don't really like that it uses current classes. If someone were to apply this migration in 10.8 it might not work as expected. I really want migrations to be version-agnostic if possible.

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 13, 2021
Author Contributor

You mean a static oldXml.value -> newXml value migration?

This comment has been minimized.

@cvium

cvium Mar 13, 2021
Member

Pretty much yes. Something that can be run if you're migrating from 10.6 to whatever.

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 15, 2021
Author Contributor

Moved migration code into the Jellyfin.Server\Migrations where the rest resides.

Tried static migration but it turned into coding nighmare with pages of string comparisons, so i decided to look at the original code to see what can be done.

For the properties to be absent from ServerConfiguration, the file has to be treated as an XML. The callback function pemits us to intercept properties that are no longer in the class, so the migration only works with properties that are not longer valid in ServerConfiguration. If any properties are redefined in ServerConfiguration in the future, they will not be migrated.

The second part is not ideal - instead of migrating if network.xml is not present, it only migrates once using the migrate.xml similar to other migration functions. However, since CommonConfiguration overwrites system.xml with a 'clean' version at creation, the existing Migration routine cannot be used, so i've had to add the config by hand. (not ideal, but i can't find another solution without altering lots of other code).

The advantage of this is that all migration results are in one place.

This comment has been minimized.

@cvium

cvium Mar 16, 2021
Member

If it makes it easier I would just make a config_migrations.xml.

And then to make it even simpler I would create two private classes that contains the properties you want to migrate; one for system.xml and one for network.xml. Then utilize the xmlserializer to move it from A to B. The end.

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 22, 2021
Author Contributor

One phrase for this one - ball ache!

What i've learnt so far, on what should have been a simple thing.

Serialization classes cannot be private - best they can be is internal.
You cannot typecast on a serialiser.

I had originally thought to declare two classes, the original inheritied from the network - that way properties would only need to declared once. The plan was then to read from source into ServerConfig, and write typecasting. Nope - writing saves the original class' object into the file.

So i've done the next best thing. desearialised into legacy class. Copied properties. Save into network class.

@BaronGreenback BaronGreenback force-pushed the BaronGreenback:SettingsMigration branch from 9a89dc6 to 5d28ac0 Mar 22, 2021
var xmlSerializer = new MyXmlSerializer();
var source = xmlSerializer.DeserializeFromFile(typeof(Legacy.ServerConfiguration), appPaths.SystemConfigurationFilePath);

var target = new NetworkConfiguration();

This comment has been minimized.

@cvium

cvium Mar 23, 2021
Member

This should be its own "legacy" class as well.

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 23, 2021
Author Contributor

It is, (sort of)- it's Legacy.ServerConfiguration.

Legacy.ServerConfiguration contains the properties are mutual to the 10.7 versions of system.xml and network.xml.

The settings subsystem loads the current xml, discards and changes and re-saves a clean on if there is anything "extra"
Storing into the 10.X version of network.xml here, just saves a resave by the settings subsystem.

This comment has been minimized.

@cvium

cvium Mar 23, 2021
Member

The point I'm trying to make is that migrations should be version agnostic. To do that we need to migrate the settings using only what we know in 10.7. The changes I'm requesting are sort of paving the way for future config migrations (we will need a dedicated migration runner when that time comes though, but that's out of scope for now).

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 23, 2021
Author Contributor

This should then work, as only the properties present in Legacy.ServerConfiguration should get migrated across into the current structure.

The only additional safeguard I can possibily think of, is to wrap the setValue (line 112) in its own try/catch block just in case the datatype of a property changes in the future.

This comment has been minimized.

@cvium

cvium Mar 23, 2021
Member

current structure is ephemeral though. If this migration is executed in 10.10 it might have different options that were migrated between other releases. Information will be lost in this case.

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 23, 2021
Author Contributor

so you suggesting this should be static, and updated every release if the structure changes?

This comment has been minimized.

@cvium

cvium Mar 23, 2021
Member

Yes and no. This is a one-off migration. Any future changes will need a new migration, which is why I also said we'd need a configuration migration runner some time in the future (overkill for now, I think).

This comment has been minimized.

@BaronGreenback

BaronGreenback Mar 23, 2021
Author Contributor

Sorry, but I think I'm still missing something. This is how I see the process.

ServerConfiguration is ephemeral, so to overcome this Legacy.ServerConfiguration has been created - which is basically a fixed point in time 10.7.0 snapshot of NetworkConfiguration.

NetworkConfiguration is ephemeral. So only the values that appear both in Legacy.ServerConfiguration and NetworkConfiguration are copied across.

If NetworkConfiguration changes in the future then under this PR the properties won't copy. A new migration PR would be required.

If Legacy.ServerConfiguration were copied into Legacy.NetworkConfiguration, and NetworkConfiguration changes - the issues will still remain, with invalid settings in Legacy.NetworkConfiguration begin dropped.

This comment has been minimized.

@cvium

cvium Mar 23, 2021
Member

If Legacy.ServerConfiguration were copied into Legacy.NetworkConfiguration, and NetworkConfiguration changes - the issues will still remain, with invalid settings in Legacy.NetworkConfiguration begin dropped.

In this scenario, other migrations would handle it. After all the migrations have run, the resulting NetworkConfiguration would be whole and valid. This migration is just a small cog responsible for migrating from <=10.6.4 to 10.7.0. The point of the migrations is that they are version agnostic and build on each other.

{
// Migrate version 7.0.3 network configuration.
RunSettingMigration<Legacy.V703.ServerConfiguration, Legacy.V703.NetworkConfiguration>(logger, appPaths.SystemConfigurationFilePath, destFile);
logger.LogDebug("Migrated to network configuration 7.0.3");

This comment has been minimized.

@crobibero

crobibero May 6, 2021
Member

What does 7.0.3 refer to?

This comment has been minimized.

@BaronGreenback

BaronGreenback May 6, 2021
Author Contributor

10.7.0.3

This comment has been minimized.

@crobibero

crobibero May 6, 2021
Member

The proper version is 10.7.3

This comment has been minimized.

@BaronGreenback

BaronGreenback May 6, 2021
Author Contributor

We were at 10.7.0rc3 at the time of submisson.

Hard coded the version as we'd know when the snapshot was taken - but your comment raises an issue - do we keep upping the version number on each release until something changes?

This comment has been minimized.

@crobibero

crobibero May 6, 2021
Member

I would say set the version to the major.minor version (10.7), as we shouldn't be changing settings in patch versions

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 7, 2021

Improved the setting migration process including a history file (based on what i perceived i would need to do for 10.8)

The sub-system traverses the legacy migration namespace, building up a list of all migration classes, and orders them ascending order based on the version number in their name. (This stops the need of each being instantiated).

eg. Jellyfin.Server.Migrations.Legacy.V10v7 is seen as the migration for version 10.7.

The class Jellyfin.Server.Migrations.Legacy.V10v7.RunMigration is instantiated which does the actual migration. (In this case migrating the network settings from 10.x to 10.7.)

It means that for 10.8, all we have to do is to create a class Jellyfin.Server.Migrations.Legacy.V10v8.RunMigration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants