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 Plugin Type: KVStorage #1208

Open
sosyz opened this issue Dec 22, 2024 · 5 comments
Open

Add Plugin Type: KVStorage #1208

sosyz opened this issue Dec 22, 2024 · 5 comments
Assignees
Labels
feature New feature request

Comments

@sosyz
Copy link
Member

sosyz commented Dec 22, 2024

Is your feature request related to a problem? Please describe

During plugin development, we frequently need persistent data storage capabilities. The current plugin system only supports reading user configurations, which severely limits plugin functionality. For example, when developing Passkey login features, plugins need to store client public key information, but the existing architecture cannot support this requirement.

Describe the solution you'd like

I propose adding a new plugin type called KVStorage to provide key-value storage capabilities for plugins. Here is the specific design:

// util
type PluginKVStorage struct {
    Get(ctx context.Context, key string) (string, error)
    Set(ctx context.Context, key, value string) error
    Del(ctx context.Context, key string) error
    Tx(ctx context.Context, call func(ctx context.Context)) error
}

// entity
type KVStorage struct{
    ID             string `xorm:"not null pk BIGINT(20) id"`
    PluginSlugName string `xorm:"not null VARCHAR(128) plugin_slug_name"`
    Status         int    `xorm:"not null default 1 INT(11) status"`
    Key            string `xorm:"not null VARCHAR(255) key"`
    Value          string `xorm:"not null MEDIUMTEXT value"`
}

// plugin
type KVStorage interface {
    // Inject PluginKVStorage
    InjectKVStorage func(kvs *PluginKVStorage)
}

This design ensures data isolation through the PluginSlugName field, where each plugin can only access its own data. It also provides transaction support to ensure atomic data operations.

Describe alternatives you've considered

  1. Direct Database Access: We considered allowing plugins to access the database directly, but this approach poses serious security risks. Direct database access would break system encapsulation and make it difficult to manage permissions and ensure data security.
  2. File System Storage: We considered letting plugins use the file system for data storage, but this approach would make unified management difficult and increase operational complexity and management costs.

Thanks to @LinkinStars for the help. Please provide any feedback - I'm happy to make adjustments.

@sosyz sosyz added the feature New feature request label Dec 22, 2024
@LinkinStars LinkinStars self-assigned this Dec 24, 2024
@LinkinStars
Copy link
Member

Very nice feature design. Let's add some more details.

  1. Let's add details on how status is used. Is it for deletion? Or is it for other identification purposes?
  2. Tx is a good design, but this one seems to be more difficult to implement. I don't have a good idea of how to implement it elegantly. Perhaps you might describe it more. I would consider this feature an enhancement, not a required item.
  3. Do you think we need to add a group concept and range query? As the plugin's data may hold data under different features. Under the current design, we can differentiate by key.

key=value

login_abc(userid)=123
feat_ced(userid)=456

Importing the concept of groups is actually

key=abc; group=login; value=123
key=ced; group=feat; value=456

In this way, perhaps we can give the plugin the ability to page through the data under a group.

Of course I would consider this an enhancement as well. Maybe for cases where the plugin has a scheduled task to scan all the existing data for processing.

@sosyz
Copy link
Member Author

sosyz commented Dec 29, 2024

@LinkinStars
Thanks for your valuable feedback! Let me address each point:

  1. Regarding the status field:
    In my current design, data is restored when updating if records with the same group and key exist. Do you think direct deletion would be a better approach? Looking forward to your feedback on this design choice.
  2. About transaction support:
    I've implemented a simple transaction mechanism that leverages xorm's session. Here's how it works:
type PluginKVStorage struct {
	db             *xorm.Engine
	session        *xorm.Session
	pluginSlugName string
}
// ...
func (kv * PluginKVStorage) Tx(ctx context.Context, fn func(ctx context.Context, kv KVOperator) error) error {
	if kv.session != nil {
		return fn(ctx, *kv)
	}

	session := kv.db.NewSession()
	defer session.Close()
	err := fn(ctx, KVOperator{
		session:        session,
		db:             kv.db,
		pluginSlugName: kv.pluginSlugName,
	})
	if err != nil {
		session.Rollback()
		return err
	}
	return session.Commit()
}

func (kv *PluginKVStorage) Get(ctx context.Context, key, group string) (string, error) {
	var data entity. KVStorage

	if key == "" && group == "" {
		return "", fmt.Errorf("either key or group must be provided")
	}

	// build query
	var query *xorm.Session
	if kv.session != nil {
		query = kv.session
	} else {
		query = kv.db.NewSession()
		defer query.Close()
	}
        // ...
}

Usage example:

err := kv.Tx(ctx, func(ctx context.Context, txKv KVOperator) error {
    if err := txKv.Set(ctx, "group1", "key1", "value1"); err != nil {
        return err
    }
    return txKv.Set(ctx, "group1", "key2", "value2")
})
  1. About groups and range query:
    Great suggestion on groups and range query! I've implemented it in the PR.

Please feel free to review and provide more feedback!

@sosyz
Copy link
Member Author

sosyz commented Dec 29, 2024

By the way, I have a question. After adding new plugin types, how should we handle cases where plugins use plugin types that are only supported after certain versions?

@LinkinStars
Copy link
Member

  1. I think this data can be deleted directly.
  2. Aha. Because there is only one context in the original design, I find it difficult to implement.Tx(ctx context.Context, call func(ctx context.Context)) error

BTW, you need to call Begin() first.

	session := engine.NewSession()
	defer session.Close()

	// call begin
	if err := session.Begin(); err != nil {
		return nil, err
	}

	// TODO

	if err := session.Commit(); err != nil {
		return result, err
	}
  1. Great.

By the way, I have a question. After adding new plugin types, how should we handle cases where plugins use plugin types that are only supported after certain versions?

Don't worry, we only support after a certain version.

@sosyz
Copy link
Member Author

sosyz commented Dec 30, 2024

@LinkinStars OK~ Thanks for your feedback, I'll make these changes and submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request
Projects
None yet
Development

No branches or pull requests

2 participants