From 7f1f8fc9e60a2e131419301161992c8c6bc80dd4 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 10 Aug 2021 20:21:03 +0200 Subject: [PATCH] (core) Linking summary tables grouped by list columns Summary: Prefix keys of `LinkingState.filterColValues` with `_contains:` when the source column is a ChoiceList or ReferenceList. This is parsed out to make a boolean `isContainsFilter` which is kept in each value of `QueryRefs.filterTuples` (previously `filterPairs`). Then when converting back in `convertQueryFromRefs` we construct `Query.contains: {[colId: string]: boolean}`. Finally `getFilterFunc` uses `Query.contains` to decide what kind of filtering to do. This is not pretty, but the existing code is already very complex and it was hard to find something that wouldn't require touching loads of code just to make things compile. Test Plan: Added a new nbrowser test and fixture, tests that selecting a source table by summary tables grouped by a choicelist column, non-list column, and both all filter the correct data. Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2940 --- app/client/components/BaseView.js | 18 ++++-- app/client/components/LinkingState.js | 27 +++++---- app/client/models/QuerySet.ts | 84 +++++++++++++++++---------- app/common/ActiveDocAPI.ts | 21 ++++++- app/server/lib/ActiveDoc.ts | 12 ++-- app/server/lib/ExpandedQuery.ts | 6 +- app/server/lib/GranularAccess.ts | 11 +--- sandbox/grist/engine.py | 17 ++++-- 8 files changed, 128 insertions(+), 68 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 16b619b9..2e9050db 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -26,6 +26,7 @@ const {copyToClipboard} = require('app/client/lib/copyToClipboard'); const {setTestState} = require('app/client/lib/testState'); const {ExtraRows} = require('app/client/models/DataTableModelWithDiff'); const {createFilterMenu} = require('app/client/ui/ColumnFilterMenu'); +const {encodeObject} = require("app/plugin/objtypes"); /** * BaseView forms the basis for ViewSection classes. @@ -140,7 +141,12 @@ function BaseView(gristDoc, viewSectionModel, options) { this._linkingFilter = this.autoDispose(ko.computed(() => { const linking = this._linkingState(); - return linking && linking.filterColValues ? linking.filterColValues() : {}; + const result = linking && linking.filterColValues ? linking.filterColValues() : {filters: {}}; + result.operations = result.operations || {}; + for (const key in result.filters) { + result.operations[key] = result.operations[key] || 'in'; + } + return result; })); // A computed for the rowId of the row selected by section linking. @@ -204,7 +210,8 @@ function BaseView(gristDoc, viewSectionModel, options) { // dependency changes. this.autoDispose(ko.computed(() => { this._isLoading(true); - this._queryRowSource.makeQuery(this._linkingFilter(), (err) => { + const linkingFilter = this._linkingFilter(); + this._queryRowSource.makeQuery(linkingFilter.filters, linkingFilter.operations, (err) => { if (this.isDisposed()) { return; } if (err) { window.gristNotify(`Query error: ${err.message}`); } this.onTableLoaded(); @@ -373,8 +380,11 @@ BaseView.prototype._parsePasteForView = function(data, cols) { }; BaseView.prototype._getDefaultColValues = function() { - const filterValues = this._linkingFilter.peek(); - return _.mapObject(_.pick(filterValues, v => (v.length > 0)), v => v[0]); + const {filters, operations} = this._linkingFilter.peek(); + return _.mapObject( + _.pick(filters, v => (v.length > 0)), + (value, key) => operations[key] === "intersects" ? encodeObject(value) : value[0] + ); }; /** diff --git a/app/client/components/LinkingState.js b/app/client/components/LinkingState.js index 76541e76..272c53f6 100644 --- a/app/client/components/LinkingState.js +++ b/app/client/components/LinkingState.js @@ -77,7 +77,7 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl } } } - return {[tgtColId]: Array.from(srcValues)}; + return {filters: {[tgtColId]: Array.from(srcValues)}}; })); } else if (srcColId) { let srcRowModel = this.autoDispose(srcTableModel.createFloatingRowModel()); @@ -88,13 +88,13 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl this.filterColValues = this.autoDispose(ko.computed(() => { const srcRowId = srcSection.activeRowId(); srcRowModel.assign(srcRowId); - return {[tgtColId]: [srcCell()]}; + return {filters: {[tgtColId]: [srcCell()]}}; })); } } else { this.filterColValues = this.autoDispose(ko.computed(() => { const srcRowId = srcSection.activeRowId(); - return {[tgtColId]: [srcRowId]}; + return {filters: {[tgtColId]: [srcRowId]}}; })); } } else if (isSummaryOf(srcSection.table(), tgtSection.table())) { @@ -103,17 +103,24 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl // those in the srcSection). // TODO: This approach doesn't help cursor-linking (the other direction). If we have the // inverse of summary-table's 'group' column, we could implement both, and more efficiently. + const isDirectSummary = srcSection.table().summarySourceTable() === tgtSection.table().getRowId(); this.filterColValues = this.autoDispose(ko.computed(() => { const srcRowId = srcSection.activeRowId(); - const filter = {}; - for (const c of srcSection.table().columns().all()) { - if (c.summarySourceCol()) { - const colId = c.summarySource().colId(); - const srcValue = srcTableData.getValue(srcRowId, colId); - filter[colId] = [srcValue]; + const filters = {}; + const operations = {}; + for (const c of srcSection.table().groupByColumns()) { + const col = c.summarySource(); + const colId = col.colId(); + const srcValue = srcTableData.getValue(srcRowId, colId); + filters[colId] = [srcValue]; + if (isDirectSummary) { + const tgtColType = col.type(); + if (tgtColType === 'ChoiceList' || tgtColType.startsWith('RefList:')) { + operations[colId] = 'intersects'; + } } } - return filter; + return {filters, operations}; })); } else if (isSummaryOf(tgtSection.table(), srcSection.table())) { // TODO: We should move the cursor, but don't currently it for summaries. For that, we need a diff --git a/app/client/models/QuerySet.ts b/app/client/models/QuerySet.ts index 96a91233..c463cc2d 100644 --- a/app/client/models/QuerySet.ts +++ b/app/client/models/QuerySet.ts @@ -30,8 +30,8 @@ import * as DataTableModel from 'app/client/models/DataTableModel'; import {DocModel} from 'app/client/models/DocModel'; import {BaseFilteredRowSource, RowId, RowList, RowSource} from 'app/client/models/rowset'; import {TableData} from 'app/client/models/TableData'; -import {ActiveDocAPI, Query} from 'app/common/ActiveDocAPI'; -import {TableDataAction} from 'app/common/DocActions'; +import {ActiveDocAPI, ClientQuery, QueryOperation} from 'app/common/ActiveDocAPI'; +import {CellValue, TableDataAction} from 'app/common/DocActions'; import {DocData} from 'app/common/DocData'; import {nativeCompare} from 'app/common/gutil'; import {IRefCountSub, RefCountMap} from 'app/common/RefCountMap'; @@ -41,6 +41,8 @@ import {tbind} from 'app/common/tbind'; import {Disposable, Holder, IDisposableOwnerT} from 'grainjs'; import * as ko from 'knockout'; import debounce = require('lodash/debounce'); +import {isList} from "app/common/gristTypes"; +import {decodeObject} from "app/plugin/objtypes"; // Limit on the how many rows to request for OnDemand tables. const ON_DEMAND_ROW_LIMIT = 10000; @@ -54,9 +56,11 @@ const MAX_SQL_PARAMS = 500; */ export interface QueryRefs { tableRef: number; - filterPairs: Array<[number, any[]]>; + filterTuples: Array; } +type FilterTuple = [number, QueryOperation, any[]]; + /** * QuerySetManager keeps track of all queries for a GristDoc instance. It is also responsible for * disposing all state associated with queries when a GristDoc is disposed. @@ -83,7 +87,7 @@ export class QuerySetManager extends Disposable { })); } - public useQuerySet(owner: IDisposableOwnerT>, query: Query): QuerySet { + public useQuerySet(owner: IDisposableOwnerT>, query: ClientQuery): QuerySet { // Convert the query to a string key which identifies it. const queryKey: string = encodeQuery(convertQueryToRefs(this._docModel, query)); @@ -150,8 +154,10 @@ export class DynamicQuerySet extends RowSource { * argument to cb() is true if any data was changed, and false if not. Note that for a series of * makeQuery() calls, cb() is always called at least once, and always asynchronously. */ - public makeQuery(filters: {[colId: string]: any[]}, cb: (err: Error|null, changed: boolean) => void): void { - const query: Query = {tableId: this._tableModel.tableData.tableId, filters}; + public makeQuery(filters: {[colId: string]: any[]}, + operations: {[colId: string]: QueryOperation}, + cb: (err: Error|null, changed: boolean) => void): void { + const query: ClientQuery = {tableId: this._tableModel.tableData.tableId, filters, operations}; const newQuerySet = this._querySetManager.useQuerySet(this._holder, query); // CB should be called asynchronously, since surprising hard-to-debug interactions can happen @@ -200,7 +206,7 @@ export class QuerySet extends BaseFilteredRowSource { constructor(docModel: DocModel, docComm: ActiveDocAPI, queryKey: string, qsm: QuerySetManager) { const queryRefs: QueryRefs = decodeQuery(queryKey); - const query: Query = convertQueryFromRefs(docModel, queryRefs); + const query: ClientQuery = convertQueryFromRefs(docModel, queryRefs); super(getFilterFunc(docModel.docData, query)); this.isTruncated = false; @@ -296,22 +302,34 @@ export class TableQuerySets { /** * Returns a filtering function which tells whether a row matches the given query. */ -export function getFilterFunc(docData: DocData, query: Query): RowFilterFunc { +export function getFilterFunc(docData: DocData, query: ClientQuery): RowFilterFunc { // NOTE we rely without checking on tableId and colIds being valid. const tableData: BaseTableData = docData.getTable(query.tableId)!; - const colIds = Object.keys(query.filters).sort(); - const colPairs = colIds.map( - (c) => [tableData.getRowPropFunc(c)!, new Set(query.filters[c])] as [RowPropFunc, Set]); - return (rowId: RowId) => colPairs.every(([getter, values]) => values.has(getter(rowId))); + const colFuncs = Object.keys(query.filters).sort().map( + (colId) => { + const getter = tableData.getRowPropFunc(colId)!; + const values = new Set(query.filters[colId]); + switch (query.operations![colId]) { + case "intersects": + return (rowId: RowId) => { + const value = getter(rowId) as CellValue; + return isList(value) && + (decodeObject(value) as unknown[]).some(v => values.has(v)); + }; + case "in": + return (rowId: RowId) => values.has(getter(rowId)); + default: + throw new Error("Unknown operation"); + } + }); + return (rowId: RowId) => colFuncs.every(f => f(rowId)); } -type RowPropFunc = (rowId: RowId) => any; - /** * Helper that converts a Query (with tableId/colIds) to an object with tableRef/colRefs (i.e. * rowIds), and consistently sorted. We use that to identify a Query across table/column renames. */ -function convertQueryToRefs(docModel: DocModel, query: Query): QueryRefs { +function convertQueryToRefs(docModel: DocModel, query: ClientQuery): QueryRefs { const tableRec: any = docModel.dataTables[query.tableId].tableMetaRow; const colRefsByColId: {[colId: string]: number} = {}; @@ -319,26 +337,32 @@ function convertQueryToRefs(docModel: DocModel, query: Query): QueryRefs { colRefsByColId[col.colId.peek()] = col.getRowId(); } - const colIds = Object.keys(query.filters); - const filterPairs = colIds.map((c) => [colRefsByColId[c], query.filters[c]] as [number, any]); + const filterTuples = Object.keys(query.filters).map((colId) => { + const values = query.filters[colId]; + // Keep filter values sorted by value, for consistency. + values.sort(nativeCompare); + return [colRefsByColId[colId], query.operations![colId], values] as FilterTuple; + }); // Keep filters sorted by colRef, for consistency. - filterPairs.sort((a, b) => nativeCompare(a[0], b[0])); - // Keep filter values sorted by value, for consistency. - filterPairs.forEach(([colRef, values]) => values.sort(nativeCompare)); - return {tableRef: tableRec.getRowId(), filterPairs}; + filterTuples.sort((a, b) => + nativeCompare(a[0], b[0]) || nativeCompare(a[1], b[1])); + return {tableRef: tableRec.getRowId(), filterTuples}; } /** * Helper to convert a QueryRefs (using tableRef/colRefs) object back to a Query (using * tableId/colIds). */ -function convertQueryFromRefs(docModel: DocModel, queryRefs: QueryRefs): Query { +function convertQueryFromRefs(docModel: DocModel, queryRefs: QueryRefs): ClientQuery { const tableRec = docModel.dataTablesByRef.get(queryRefs.tableRef)!.tableMetaRow; const filters: {[colId: string]: any[]} = {}; - for (const [colRef, values] of queryRefs.filterPairs) { - filters[docModel.columns.getRowModel(colRef).colId.peek()] = values; + const operations: {[colId: string]: QueryOperation} = {}; + for (const [colRef, operation, values] of queryRefs.filterTuples) { + const colId = docModel.columns.getRowModel(colRef).colId.peek(); + filters[colId] = values; + operations[colId] = operation; } - return {tableId: tableRec.tableId.peek(), filters}; + return {tableId: tableRec.tableId.peek(), filters, operations}; } /** @@ -349,13 +373,13 @@ function convertQueryFromRefs(docModel: DocModel, queryRefs: QueryRefs): Query { * guaranteed. This is important to produce consistent results (same query => same encoding). */ function encodeQuery(queryRefs: QueryRefs): string { - return JSON.stringify([queryRefs.tableRef, queryRefs.filterPairs]); + return JSON.stringify([queryRefs.tableRef, queryRefs.filterTuples]); } // Decode an encoded QueryRefs. function decodeQuery(queryKey: string): QueryRefs { - const [tableRef, filterPairs] = JSON.parse(queryKey); - return {tableRef, filterPairs}; + const [tableRef, filterTuples] = JSON.parse(queryKey); + return {tableRef, filterTuples}; } /** @@ -364,7 +388,7 @@ function decodeQuery(queryKey: string): QueryRefs { */ function makeQueryInvalidComputed(docModel: DocModel, queryRefs: QueryRefs): ko.Computed { const tableFlag: ko.Observable = docModel.tables.getRowModel(queryRefs.tableRef)._isDeleted; - const colFlags: Array> = queryRefs.filterPairs.map( - ([colRef, values]) => docModel.columns.getRowModel(colRef)._isDeleted); + const colFlags: Array> = queryRefs.filterTuples.map( + ([colRef, , ]) => docModel.columns.getRowModel(colRef)._isDeleted); return ko.computed(() => Boolean(tableFlag() || colFlags.some((c) => c()))); } diff --git a/app/common/ActiveDocAPI.ts b/app/common/ActiveDocAPI.ts index b395bced..1079fe44 100644 --- a/app/common/ActiveDocAPI.ts +++ b/app/common/ActiveDocAPI.ts @@ -62,16 +62,33 @@ export interface ImportTableResult { * {tableId: "Projects", filters: {}} * {tableId: "Employees", filters: {Status: ["Active"], Dept: ["Sales", "HR"]}} */ -export interface Query { +interface BaseQuery { tableId: string; filters: { [colId: string]: any[]; }; +} +/** + * Query that can only be used on the client side. + * Allows filtering with more complex operations. + */ +export interface ClientQuery extends BaseQuery { + operations?: { + [colId: string]: QueryOperation; + } +} + +/** + * Query intended to be sent to a server. + */ +export interface ServerQuery extends BaseQuery { // Queries to server for onDemand tables will set a limit to avoid bringing down the browser. limit?: number; } +export type QueryOperation = "in" | "intersects"; + /** * Response from useQuerySet(). A query returns data AND creates a subscription to receive * DocActions that affect this data. The querySubId field identifies this subscription, and must @@ -113,7 +130,7 @@ export interface ActiveDocAPI { * docActions that affect this query's results. The subscription remains functional even when * tables or columns get renamed. */ - useQuerySet(query: Query): Promise; + useQuerySet(query: ServerQuery): Promise; /** * Removes the subscription to a Query, identified by QueryResult.querySubId, so that the diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 688654ef..8bf74fde 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -22,8 +22,8 @@ import { DataSourceTransformed, ForkResult, ImportResult, - Query, - QueryResult + QueryResult, + ServerQuery } from 'app/common/ActiveDocAPI'; import {ApiError} from 'app/common/ApiError'; import {mapGetOrSet, MapWithTTL} from 'app/common/AsyncCreate'; @@ -612,7 +612,7 @@ export class ActiveDoc extends EventEmitter { * @param {Boolean} waitForFormulas: If true, wait for all data to be loaded/calculated. If false, * special "pending" values may be returned. */ - public async fetchQuery(docSession: OptDocSession, query: Query, + public async fetchQuery(docSession: OptDocSession, query: ServerQuery, waitForFormulas: boolean = false): Promise { this._inactivityTimer.ping(); // The doc is in active use; ping it to stay open longer. @@ -693,7 +693,7 @@ export class ActiveDoc extends EventEmitter { * Makes a query (documented elsewhere) and subscribes to it, so that the client receives * docActions that affect this query's results. */ - public async useQuerySet(docSession: OptDocSession, query: Query): Promise { + public async useQuerySet(docSession: OptDocSession, query: ServerQuery): Promise { this.logInfo(docSession, "useQuerySet(%s, %s)", docSession, query); // TODO implement subscribing to the query. // - Convert tableId+colIds to TableData/ColData references @@ -1356,7 +1356,7 @@ export class ActiveDoc extends EventEmitter { } } - private async _fetchQueryFromDB(query: Query, onDemand: boolean): Promise { + private async _fetchQueryFromDB(query: ServerQuery, onDemand: boolean): Promise { // Expand query to compute formulas (or include placeholders for them). const expandedQuery = expandQuery(query, this.docData!, onDemand); const marshalled = await this.docStorage.fetchQuery(expandedQuery); @@ -1372,7 +1372,7 @@ export class ActiveDoc extends EventEmitter { return toTableDataAction(query.tableId, table); } - private async _fetchQueryFromDataEngine(query: Query): Promise { + private async _fetchQueryFromDataEngine(query: ServerQuery): Promise { return this._pyCall('fetch_table', query.tableId, true, query.filters); } diff --git a/app/server/lib/ExpandedQuery.ts b/app/server/lib/ExpandedQuery.ts index 326822e3..0016a984 100644 --- a/app/server/lib/ExpandedQuery.ts +++ b/app/server/lib/ExpandedQuery.ts @@ -1,4 +1,4 @@ -import {Query} from 'app/common/ActiveDocAPI'; +import {ServerQuery} from 'app/common/ActiveDocAPI'; import {ApiError} from 'app/common/ApiError'; import {DocData} from 'app/common/DocData'; import {parseFormula} from 'app/common/Formula'; @@ -10,7 +10,7 @@ import {quoteIdent} from 'app/server/lib/SQLiteDB'; * formulas. Use of this representation should be limited to within a * trusted part of Grist since it assembles SQL strings. */ -export interface ExpandedQuery extends Query { +export interface ExpandedQuery extends ServerQuery { // Errors detected for given columns because of formula issues. We // need to make sure the result of the query contains these error // objects. It is awkward to write a sql selection that constructs @@ -38,7 +38,7 @@ export interface ExpandedQuery extends Query { * * If onDemandFormulas is set, ignore stored formula columns, and compute them using SQL. */ -export function expandQuery(iquery: Query, docData: DocData, onDemandFormulas: boolean = true): ExpandedQuery { +export function expandQuery(iquery: ServerQuery, docData: DocData, onDemandFormulas: boolean = true): ExpandedQuery { const query: ExpandedQuery = { tableId: iquery.tableId, filters: iquery.filters, diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 56ec1f19..ab69cb51 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -2,7 +2,7 @@ import { ALL_PERMISSION_PROPS } from 'app/common/ACLPermissions'; import { ACLRuleCollection, SPECIAL_RULES_TABLE_ID } from 'app/common/ACLRuleCollection'; import { ActionGroup } from 'app/common/ActionGroup'; import { createEmptyActionSummary } from 'app/common/ActionSummary'; -import { Query } from 'app/common/ActiveDocAPI'; +import { ServerQuery } from 'app/common/ActiveDocAPI'; import { ApiError } from 'app/common/ApiError'; import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord } from 'app/common/DocActions'; import { RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions'; @@ -170,7 +170,7 @@ export class GranularAccess implements GranularAccessForBundle { public constructor( private _docData: DocData, private _docClients: DocClients, - private _fetchQueryFromDB: (query: Query) => Promise, + private _fetchQueryFromDB: (query: ServerQuery) => Promise, private _recoveryMode: boolean, private _homeDbManager: HomeDBManager | null, private _docId: string) { @@ -204,13 +204,6 @@ export class GranularAccess implements GranularAccessForBundle { this._userAttributesMap = new WeakMap(); } - /** - * Check whether user can carry out query. - */ - public hasQueryAccess(docSession: OptDocSession, query: Query) { - return this.hasTableAccess(docSession, query.tableId); - } - public getUser(docSession: OptDocSession): Promise { return this._getUser(docSession); } diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index eb13ced6..6dd68cad 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -980,13 +980,22 @@ class Engine(object): old_tables = self.tables self.tables = {} + sorted_tables = [] for table_id, user_table in six.iteritems(self.gencode.usercode.__dict__): - if isinstance(user_table, table_module.UserTable): - self.tables[table_id] = (old_tables.get(table_id) or table_module.Table(table_id, self)) + if not isinstance(user_table, table_module.UserTable): + continue + self.tables[table_id] = table = ( + old_tables.get(table_id) or table_module.Table(table_id, self) + ) + + # Process non-summary tables first so that summary tables + # can read correct metadata about their source tables + key = (hasattr(user_table.Model, '_summarySourceTable'), table_id) + sorted_tables.append((key, table, user_table)) + sorted_tables.sort() # Now update the table model for each table, and tie it to its UserTable object. - for table_id, table in six.iteritems(self.tables): - user_table = getattr(self.gencode.usercode, table_id) + for _, table, user_table in sorted_tables: self._update_table_model(table, user_table) user_table._set_table_impl(table)