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

Split connector's config model from connector's logic #206

Open
davinov opened this issue Sep 6, 2020 · 1 comment
Open

Split connector's config model from connector's logic #206

davinov opened this issue Sep 6, 2020 · 1 comment
Labels
question Further information is requested

Comments

@davinov
Copy link
Member

davinov commented Sep 6, 2020

With the arrival of new connectors, like the new google sheets one (#202), I see new uses of connector's properties:

  • some "flagging" attributes: like auth_flow: "oAuth2". These attributes are useful to indicate behavior changes for some groups of connectors.
  • some "internal" attributes: like secrets. These attributes are necessary to run a query, but should not be filled directly by a user when configuring a connector. Instead, they are populated by the server that makes use of the connector.
    Both have in common that they should not appear in the schema (which will be used to display a form) to create or edit a connector.

For the first type, I tested the ClassVar annotation and making the attribute private (prepended with _). It does prevent to display the info in the schema, but also hide the info when exporting the document as dict. Therefore, a front-end would not be able to have this information, and for example open a consent screen for connectors having the authFlow property to "oAuth2".
This starts to show that we want our users to configure, and what we want to export as a connector could sometimes becomes a bit different.

For the second type, private attributes works well combined with setters. But it's still a bit of tricky logic to have to think of making them private to exclude them from being stored.

What I suggest is to separate the ConnectorConfig from the Connector itself.
ConnectorConfig is a good application of pydantic's BaseModel. It's something that will comes from user input and stored in a DB or a config file. It should be validated.
Connector stores a lot of logic, and this logic needs the configuration object, but also more and more other info.

Users provides the ConnectorConfig, which is stored in DB.
Server creates a Connector from a ConnectorConfig and other relevant info.
Serve can serialize a Connector and send it, with its config and other relevant info if necessary.

What are your opinion on this?

Related links:
pydantic/pydantic#655 specially this comment about God-like objects

@davinov davinov added the question Further information is requested label Sep 6, 2020
@testinnplayin
Copy link
Contributor

testinnplayin commented Sep 15, 2020

I think it's a good idea to separate out the configuration but not sure in a way about the database from the point of view that so far no connector sends queries to a database/our database. Currently, it's only our server (private repo) that handles communication with a database. We lose the abstraction that the connectors project so far has from any of our other projects if we store the configuration in the database.

I'm for keeping them in configuration files and in my mind that's what we should ultimately do (for eg. in JSON files). However, this would only be great for static properties like baseroute. If we have to make a new file for every set of secrets we'd want to store, that would become problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants