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

Add a migration feature #366

Draft
wants to merge 12 commits into
base: trunk
Choose a base branch
from
35 changes: 35 additions & 0 deletions inc/Config/DataSource/HttpDataSource.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ final public function get_service_name(): string {
* define their own validation schema.
*/
public static function preprocess_config( array $config ): array|WP_Error {
$config = static::perform_migration( $config );
Copy link
Contributor Author

@ingeniumed ingeniumed Feb 21, 2025

Choose a reason for hiding this comment

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

@chriszarate - the new preprocess_config method seems like the right place to call the migration method from within HttpDataSource looks like. Though if a custom implementor is used then it wouldn't be called via from_array so that is a downside.

What do you think about the way the migration methods are called here?

What I haven't done yet:

  • Move the version field up one level
  • Update the server and UI to account for that
  • Added in a sample migration. The active/version changes would really be examples of those
  • Modify the schema of a data source, and see if the migration works or not properly.

Copy link
Member

Choose a reason for hiding this comment

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

Though if a custom implementor is used then it wouldn't be called via from_array so that is a downside.

from_array is now marked as final so that shouldn't be an issue

Copy link
Member

Choose a reason for hiding this comment

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

In general, I think we should delegate as much as possible and follow the pattern we've used elsewhere of "return the thing or a WP_Error, which we then elevate to the user to remedy"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$config = static::perform_migration( $config );
$config = static::migrate_config( $config );
if ( is_wp_error( $config ) ) {
return $config;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was to allow us to be able to migrate the fields that we control within the config as well.

A good example of this would be the first migration:

  • Adding in active field to indicate if a data source is active or not. This could be useful for cases where a migration was unsuccessful or if a data source has errors and we need it to be fixed. This would help build that UI feature.
  • Moving the version field to the top of the config rather than within the service config.

There could be more down the line but that's why it's been done that way.

Are you saying to move what I have in perform_migration into migrate_config instead and allow for the data from the overriden methods to be merged with that?

Hope that makes sense, and I correctly articulated what you are suggesting.

Copy link
Member

Choose a reason for hiding this comment

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

Adding in active field to indicate if a data source is active or not.

Can't we just use WP_Error as a signal for that? The migration either works or it doesn't. And if it doesn't we have no way to inspect the problem other than to surface an error. In that case we can also treat it as inactive.

Copy link
Member

Choose a reason for hiding this comment

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

Moving the version field to the top of the config rather than within the service config.

This is a design mistake that I think we should just fix. Doesn't need to be handled by a migration, IMO

Copy link
Member

Choose a reason for hiding this comment

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

Put another way, if the migration fails and you set active = false, how are you going to read and respect that? The config still needs to pass validation (which it likely won't, so it will get elevated to an error anyway).


$service_config = $config['service_config'] ?? [];
$validator = new Validator( static::get_service_config_schema() );
$validated = $validator->validate( $service_config );
Expand All @@ -63,6 +65,31 @@ public static function preprocess_config( array $config ): array|WP_Error {
);
}

/**
* Performs a migration if the config is out of date.
*/
private static function perform_migration( array $config ): array {
// By default, we want to have an active data source.
if ( ! isset( $config['active'] ) ) {
$config['active'] = true;
}

if ( static::SERVICE_SCHEMA_VERSION === $config['service_config']['__version'] ) {
return $config;
}

$migrated_config = static::migrate_config( $config );

if ( is_wp_error( $migrated_config ) ) {
$config['active'] = false;
} else {
$config['service_config']['__version'] = static::SERVICE_SCHEMA_VERSION;
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be delegated to the migrate_config version. Maybe there are scenarios where they don't want to advance to the latest version because there is missing info. I think we want to completely delegate to migrate_config and remove this method entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. Having us control would negate the point of letting data source owners take charge of the migration process.

$config['active'] = true;
}

return $config;
}

public static function from_uuid( string $uuid ): DataSourceInterface|WP_Error {
$config = DataSourceCrud::get_config_by_uuid( $uuid );

Expand Down Expand Up @@ -105,4 +132,12 @@ protected static function map_service_config( array $service_config ): array {
'request_headers' => $service_config['request_headers'] ?? [],
];
}

/**
* Migrates the config to the current schema version.
* Can be overridden by child classes to perform custom migrations.
*/
protected static function migrate_config( array $config ): array {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected static function migrate_config( array $config ): array {
protected static function migrate_config( array $config ): array|WP_Error {

return $config;
}
}
Loading