Postgres Query Source v2 #3008
Postgres Query Source v2 #3008
Conversation
pedroslopez
force-pushed
the
181274014-postgres-query-v2
branch
from
2335f01
to
3699987
Compare
| // check if properties are still valid | ||
| const properties = await this.$get("properties"); | ||
| for (const property of properties) { | ||
| try { | ||
| await property.validateOptions(); | ||
| } catch (err) { | ||
| throw new Error(`Error when validating properties: ${err}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
because validating the property options throws an error if the column is not valid, this prevents us from saving the source with a query that no longer returns the data for the properties.
Note that this would also affects changing the table on a table source with properties from the initial table.
I think getting a validation error when updating the source is better than getting a bunch of errors down the line when trying to import records
| getPropertyValues, | ||
| getRowCount, | ||
| }) => { | ||
| const useAggregations = false; |
There was a problem hiding this comment.
Query sources can technically support aggregations, but we're disabling them since it's probably more efficient / natural for the user to do that within the query
| const tableName = sourceOptions[tableNameKey]?.toString(); | ||
| const sourceQuery = sourceOptions[sourceQueryKey]?.toString(); |
There was a problem hiding this comment.
here's the main change in these app-templates: in addition to the tableName if it exists, we are also passing down the sourceQuery if it exists so that the plugin can do what it needs to in either scenario. The rest of the code is basically the same.
| const aggregationMethod = useAggregations | ||
| ? <AggregationMethod>propertyOptions[aggregationMethodKey] | ||
| : AggregationMethod.Exact; |
There was a problem hiding this comment.
Query sources don't have an aggregationMethod option, but we still want to group all the properties so we can get them in one go. Because of this I'm now defaulting to exact in core when determining groupings. Because the plugin is still the one saying which aggregations can be grouped via groupAggregations, I think this is safe.
Initially I had the aggregation method option for query sources too, just with a single option to select, but this added an extra unnecessary step in the UI that I don't think should be there only for internal reasons.
| const params = [tableName]; | ||
| let query = `SELECT ${aggSelect} as __result FROM %I WHERE`; | ||
| const params: string[] = []; | ||
| const from = makeFromClause({ tableName, sourceQuery }, params); |
There was a problem hiding this comment.
On the postgres side, lots of the table methods are also reused, but they now use this makeFromClause helper any time we need to query their source
There was a problem hiding this comment.
This is really great!
I'm a little concerned that in a few places you have code that assumes a missing aggregationMethod implies "exact"
const aggregationMethod = useAggregations
? <AggregationMethod>(
propertyOptions[Object.keys(propertyOptions)[0]][aggregationMethodKey]
)
: AggregationMethod.Exact;
Maybe it would be better to hard-code only the Exact aggregation methods for these new query-sources? I'm worried that there are other places in the code (grouping record property imports?) that need to be able to look this up
| await expect( | ||
| source.setOptions({ | ||
| table: "admins", | ||
| tableWithOptions: "admins", | ||
| }) | ||
| ).rejects.toThrow( | ||
| /Error when validating properties: Error: "email" is not a valid value/ | ||
| ); |
| const aggregationMethod = | ||
| (options.aggregationMethod as AggregationMethod) ?? | ||
| AggregationMethod.Exact; |
There was a problem hiding this comment.
A reasonable default... but might confuse us in the future
|
@evantahler should we add a It's a bit odd because core is grouping property imports based on a property option that the plugin may or may not have. (and we need this so we can actually import these properties returned in our query at once) The alternative that I was using earlier was actually having the aggregationMethod as an option with only one thing to select ( Edit implemented the |
Change description
Postgres Query source can define a single query as a source option that will be used for schedules and importing properties.
Checklists
Development
Impact
Please explain any security, performance, migration, or other impacts if relevant:
Code review