-
Notifications
You must be signed in to change notification settings - Fork 6
[WIP] split config schema #417
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
base: trunk
Are you sure you want to change the base?
Conversation
- Introduced `DataSourceConnection` and `DataSourceQuery` abstract classes for managing data source configurations and queries. - Created corresponding interfaces: `DataSourceConnectionInterface` and `DataSourceQueryInterface`. - Added schema generation methods in `ConfigSchemas` for validating connection and query configurations. This lays the groundwork for splitting the config from single data source object to connection and query part.
- Introduced `DataSourceConnectionCrud` and `DataSourceQueryCrud` classes for managing data source connections and queries stored in WordPress options. - Created an abstract base class `WpOptionsConfigStore` to provide common CRUD operations for configurations. - Added constant class maps for data source connection and query configurations.
6484e6c
to
e126eec
Compare
* This class represents the query-specific parameters for a data source, | ||
* separate from the connection/authentication information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just merge this into the query, then? I don't think I understand the need for separation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpDataSource and HttpQuery classes are different in that HttpDataSource only needs the config as input and nothing else, while for HttpQuery, those configs are just one of the inputs required amongst others.
The query configs (base/table for Airtable, spreadsheet/sheet for Google Sheets, etc.) currently don't map 1:1 with our Query/HttpQuery classes since one such query config would need multiple HttpQuery instances to register blocks. For example, selecting 2 tables in Airtable would need at least 2 queries - list and search queries for each of the tables. I tend to think of configs as minimum data required for us to generate the queries and register blocks which for Airtable would mean just knowing the tables and the schema for each and our integration code would use that to generate the HttpQuery class.
The new DataSourceConnection
and DataSourceQuery
classes can focus on just one thing - validating the configs. Having the config split would allow reusing the connections.
Slack thread for more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions if this sounds like its complicating things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those configs are just one of the inputs required amongst others
What are the others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input_schema
, output_schema
, preprocess_response
which might not always be derived from the query config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we provide defaults that could then be edited in the UI, if needed?
No description provided.