-
Notifications
You must be signed in to change notification settings - Fork 117
Postgres Query Source v2 #3008
base: main
Are you sure you want to change the base?
Postgres Query Source v2 #3008
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/ | ||
); |
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.
Nice
const aggregationMethod = | ||
(options.aggregationMethod as AggregationMethod) ?? | ||
AggregationMethod.Exact; |
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.
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