incubator-devlake icon indicating copy to clipboard operation
incubator-devlake copied to clipboard

[Feature][plugin-core] PluginConnectionHandler interface

Open tk103331 opened this issue 2 years ago • 2 comments

Search before asking

  • [X] I had searched in the issues and found no similar feature requirement.

Description

I found that the plugin's ApiResources registration has a declaration about connections.

func (plugin Github) ApiResources() map[string]map[string]core.ApiResourceHandler {
	return map[string]map[string]core.ApiResourceHandler{
		"test": {
			"POST": api.TestConnection,
		},
		"connections": {
			"POST": api.PostConnections,
			"GET":  api.ListConnections,
		},
		"connections/:connectionId": {
			"GET":    api.GetConnection,
			"PATCH":  api.PatchConnection,
			"DELETE": api.DeleteConnection,
		},
	}
}

Is it possible to abstract as a golang interface? like this:

type PluginConnectionHandler interface {
	TestConnection(input *ApiResourceInput) (*ApiResourceOutput, error)
	PostConnections(input *ApiResourceInput) (*ApiResourceOutput, error)
	PatchConnection(input *ApiResourceInput) (*ApiResourceOutput, error)
	DeleteConnection(input *ApiResourceInput) (*ApiResourceOutput, error)
	ListConnections(input *ApiResourceInput) (*ApiResourceOutput, error)
	GetConnection(input *ApiResourceInput) (*ApiResourceOutput, error)
}

The benefits of doing this are:

  • The backend can determine whether all methods are implemented by checking whether the interface is implemented.
  • Plugin authors only need to implement the interface and do not need to care about the details of API requests (such as request paths and request methods).
  • The front end can use the same API interface.

Use case

No response

Related issues

No response

Are you willing to submit a PR?

  • [ ] Yes I am willing to submit a PR!

Code of Conduct

tk103331 avatar Aug 29 '22 02:08 tk103331

Good one, actually, most of us thought about it in the past. The reason we didn't do it are listed below:

  1. Connection is plugin specific, it may contain different fields with different types against different data sources (API, database, TCP, MessageQueue), although all our existing connections look alike, it is not necessarily true in the future.
  2. And because Connection is plugin specific, it makes more sense to let plugins implement the API. take test for example, each plugin has a similar but quite different implementation, in terms of what kind of Authentication mechanism or which URL to validate. A totally different implementation would appear sooner or later, for example, to connect to a 3rd database directly. So implementing Connection API via interface is not very extensible for the system.
  3. The idea is to handle connection by Convention, that is, we check if a plugin implements those APIs(test/list/create/edit...) if we need to in the future.

On the other hand, we provide connection_helper to help developers work with connections, which could encrypt/decrypt the secret fields when storing/loading to/from database, https://github.com/apache/incubator-devlake/blob/5b7859c662eef8cbb3f0df5dc8f98501e581d61e/plugins/github/api/connection.go#L138

I'm happy that you asked, feel free to add more context if I misunderstood your intention. 😃

klesh avatar Aug 29 '22 07:08 klesh

Maybe I didn't express it clearly. I mean use implementing golang interface to standardize the definition of connection. The specific implementation of testing/creating/updating/querying the connection is still in the plugin.

tk103331 avatar Aug 29 '22 08:08 tk103331

This issue has been automatically marked as stale because it has not had recent activity for 30 days. It will be closed in next 7 days if no further activity occurs.

github-actions[bot] avatar Sep 29 '22 00:09 github-actions[bot]

This issue has been closed because it has not received response for too long time. You could reopen it if you encountered similar problems in the future.

github-actions[bot] avatar Oct 08 '22 00:10 github-actions[bot]