Skip to content

Commit d591b4a

Browse files
paulfitzpaulfitz
committed
refuse to try table/column level changes for now
1 parent 275c0a8 commit d591b4a

File tree

5 files changed

+91
-52
lines changed

5 files changed

+91
-52
lines changed

‎app/client/ui/ProposedChangesPage.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ export class ProposedChangesPage extends Disposable {
128128
const docId = this.gristDoc.docId();
129129
const origUrlId = buildOriginalUrlId(docId, isSnapshot);
130130
const isReadOnly = this.gristDoc.docPageModel.currentDoc.get()?.isReadonly;
131-
131+
132132
return [
133133
dom('p',
134134
t('This is a list of changes relative to the {{originalDocument}}.', {
@@ -243,8 +243,15 @@ export class ProposedChangesPage extends Disposable {
243243
bigPrimaryButton(
244244
applied ? t("Reapply") : t("Apply"),
245245
dom.on('click', async () => {
246-
const result = await this.gristDoc.docComm.applyProposal(proposal.shortId);
247-
this._proposalsObs.splice(idx, 1, result.proposal);
246+
const outcome = await this.gristDoc.docComm.applyProposal(proposal.shortId);
247+
this._proposalsObs.splice(idx, 1, outcome.proposal);
248+
// For the moment, send debug information to console
249+
for (const change of outcome.log.changes) {
250+
if (change.fail) {
251+
reportError(new Error(change.msg));
252+
}
253+
console.log(change);
254+
}
248255
}),
249256
testId('propose'),
250257
),

‎app/common/ActiveDocAPI.ts‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {BulkAddRecord, CellValue, TableDataAction, UserAction} from 'app/common/
33
import {DocStateComparison} from 'app/common/DocState';
44
import {PredicateFormulaProperties} from 'app/common/PredicateFormula';
55
import {FetchUrlOptions, UploadResult} from 'app/common/uploads';
6-
import {PermissionData, UserAccessData} from 'app/common/UserAPI';
6+
import {PermissionData, Proposal, UserAccessData} from 'app/common/UserAPI';
77
import {ParseOptions} from 'app/plugin/FileParserAPI';
88
import {AccessTokenOptions, AccessTokenResult, UIRowId} from 'app/plugin/GristAPI';
99
import {IMessage} from 'grain-rpc';
@@ -545,5 +545,20 @@ export interface ActiveDocAPI {
545545

546546
applyProposal(proposalId: number, option?: {
547547
dismiss?: boolean,
548-
}): Promise<any>;
548+
}): Promise<ApplyProposalResult>;
549+
}
550+
551+
export interface ApplyProposalResult {
552+
proposal: Proposal;
553+
log: PatchLog;
554+
}
555+
556+
export interface PatchLog {
557+
changes: PatchItem[];
558+
applied: boolean;
559+
}
560+
561+
export interface PatchItem {
562+
msg: string;
563+
fail?: boolean;
549564
}

‎app/gen-server/entity/Proposal.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,12 @@ import { BaseEntity, Column, Entity, JoinColumn, ManyToOne, PrimaryColumn } from
3838
* in a full timeline, but it doesn't make sense to invest in that now, and
3939
* some kind of summary state would be needed anyway.
4040
*
41+
* There's at least one security problem with proposals. The source
42+
* document id may be a "secret" if it was created by an anonymous
43+
* user, in the sense that anyone who knows the id could edit
44+
* it. Something to bear in mind. Some proposal endpoints do some
45+
* censoring but may not be the right way to go.
46+
*
4147
*/
4248

4349
@Entity({name: 'proposals'})

‎app/server/lib/ActiveDoc.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {ActionSummary} from 'app/common/ActionSummary';
1717
import {
1818
AclResources,
1919
AclTableDescription,
20+
ApplyProposalResult,
2021
ApplyUAExtendedOptions,
2122
ApplyUAOptions,
2223
ApplyUAResult,
@@ -28,6 +29,7 @@ import {
2829
ImportResult,
2930
ISuggestionWithValue,
3031
MergeOptions,
32+
PatchLog,
3133
PermissionDataWithExtraUsers,
3234
QueryResult,
3335
ServerQuery,
@@ -167,7 +169,7 @@ import {createAttachmentsIndex, DocStorage, REMOVE_UNUSED_ATTACHMENTS_DELAY} fro
167169
import {expandQuery, getFormulaErrorForExpandQuery} from 'app/server/lib/ExpandedQuery';
168170
import {GranularAccess, GranularAccessForBundle} from 'app/server/lib/GranularAccess';
169171
import {OnDemandActions} from 'app/server/lib/OnDemandActions';
170-
import {Patch} from 'app/server/lib/Patch';
172+
import {Patch } from 'app/server/lib/Patch';
171173
import {findOrAddAllEnvelope, Sharing} from 'app/server/lib/Sharing';
172174
import {Cancelable} from 'lodash';
173175
import cloneDeep = require('lodash/cloneDeep');
@@ -904,7 +906,7 @@ export class ActiveDoc extends EventEmitter {
904906

905907
public async applyProposal(docSession: OptDocSession, proposalId: number, options?: {
906908
dismiss?: boolean,
907-
}) {
909+
}): Promise<ApplyProposalResult> {
908910
const urlId = this.docName;
909911
const proposal = await this._getHomeDbManagerOrFail().getProposal(urlId, proposalId);
910912
if (!proposal) {
@@ -914,20 +916,23 @@ export class ActiveDoc extends EventEmitter {
914916
if (!origDetails) {
915917
throw new ApiError('Proposal details not found', 500);
916918
}
919+
let result: PatchLog = {changes: [], applied: false};
917920
if (!options?.dismiss) {
918921
const patch = new Patch(this, docSession);
919922
const {details} = removeMetadataChangesFromDetails(origDetails);
920-
await patch.applyChanges(details);
921-
await this._getHomeDbManagerOrFail().updateProposalStatus(urlId, proposalId, {
922-
status: 'applied'
923-
});
923+
result = await patch.applyChanges(details);
924+
if (result.applied) {
925+
await this._getHomeDbManagerOrFail().updateProposalStatus(urlId, proposalId, {
926+
status: 'applied'
927+
});
928+
}
924929
} else {
925930
await this._getHomeDbManagerOrFail().updateProposalStatus(urlId, proposalId, {
926931
status: 'dismissed'
927932
});
928933
}
929934
const proposal2 = await this._getHomeDbManagerOrFail().getProposal(urlId, proposalId);
930-
return {proposal: proposal2};
935+
return {proposal: proposal2 as any, log: result};
931936
}
932937

933938
/**

‎app/server/lib/Patch.ts‎

Lines changed: 46 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,56 +6,61 @@
66
*/
77

88
import { TableDelta } from 'app/common/ActionSummary';
9+
import { PatchItem, PatchLog } from 'app/common/ActiveDocAPI';
910
import { UserAction } from 'app/common/DocActions';
1011
import { DocStateComparisonDetails } from 'app/common/DocState';
1112
import { ActiveDoc } from 'app/server/lib/ActiveDoc';
1213
import { OptDocSession } from 'app/server/lib/DocSession';
1314

14-
export interface Change {
15-
msg: string;
16-
fail?: boolean;
17-
}
18-
19-
export type Changes = Change[];
20-
2115
export class Patch {
2216
private _otherId: number|undefined;
2317
private _linkId: number|undefined;
2418

2519
public constructor(public gristDoc: ActiveDoc, public docSession: OptDocSession) {}
2620

27-
public async applyChanges(details: DocStateComparisonDetails): Promise<Changes> {
28-
const changes: Changes = [];
29-
const summary = details.leftChanges;
30-
if (summary.tableRenames.length > 0) {
31-
changes.push({
32-
msg: 'table renames ignored',
33-
});
34-
}
35-
for (const [tableId, delta] of Object.entries(summary.tableDeltas)) {
36-
if (tableId.startsWith('_grist_')) {
37-
continue;
38-
}
39-
if (delta.columnRenames.length > 0) {
40-
changes.push({
41-
msg: 'column renames ignored',
42-
});
43-
}
44-
if (delta.removeRows.length > 0) {
45-
changes.push(...await this.removeRows(tableId, delta));
21+
public async applyChanges(details: DocStateComparisonDetails): Promise<PatchLog> {
22+
const changes: PatchItem[] = [];
23+
try {
24+
const summary = details.leftChanges;
25+
if (summary.tableRenames.length > 0) {
26+
throw new Error('table-level changes cannot be handled yet');
4627
}
47-
if (delta.updateRows.length > 0) {
48-
changes.push(...await this.updateRows(tableId, delta));
28+
for (const [, delta] of Object.entries(summary.tableDeltas)) {
29+
if (delta.columnRenames.length > 0) {
30+
throw new Error('column-level changes cannot be handled yet');
31+
}
4932
}
50-
if (delta.addRows.length > 0) {
51-
changes.push(...await this.addRows(tableId, delta));
33+
for (const [tableId, delta] of Object.entries(summary.tableDeltas)) {
34+
if (tableId.startsWith('_grist_')) {
35+
continue;
36+
}
37+
if (delta.columnRenames.length > 0) {
38+
changes.push({
39+
msg: 'column renames ignored',
40+
});
41+
}
42+
if (delta.removeRows.length > 0) {
43+
changes.push(...await this.removeRows(tableId, delta));
44+
}
45+
if (delta.updateRows.length > 0) {
46+
changes.push(...await this.updateRows(tableId, delta));
47+
}
48+
if (delta.addRows.length > 0) {
49+
changes.push(...await this.addRows(tableId, delta));
50+
}
5251
}
52+
} catch (e) {
53+
changes.push({
54+
msg: String(e),
55+
fail: true,
56+
});
5357
}
54-
return changes;
58+
const applied = changes.some(change => !change.fail);
59+
return {changes, applied};
5560
}
5661

5762
public async change(delta: TableDelta, tableId: string, rowId: number, colId: string,
58-
pre: any, post: any): Promise<Change> {
63+
pre: any, post: any): Promise<PatchItem> {
5964
await this.applyUserActions([
6065
['UpdateRecord', tableId, rowId, { [colId]: post }],
6166
]);
@@ -80,7 +85,7 @@ export class Patch {
8085
this._linkId = result.actionNum;
8186
}
8287

83-
public async doAdd(delta: TableDelta, tableId: string, rowId: number, rec: Record<string, any>): Promise<Change> {
88+
public async doAdd(delta: TableDelta, tableId: string, rowId: number, rec: Record<string, any>): Promise<PatchItem> {
8489
if (rec.manualSort) {
8590
delete rec.manualSort;
8691
}
@@ -95,7 +100,8 @@ export class Patch {
95100
};
96101
}
97102

98-
public async doRemove(delta: TableDelta, tableId: string, rowId: number, rec: Record<string, any>): Promise<Change> {
103+
public async doRemove(delta: TableDelta, tableId: string, rowId: number,
104+
rec: Record<string, any>): Promise<PatchItem> {
99105
await this.applyUserActions([
100106
['RemoveRecord', tableId, rowId],
101107
]);
@@ -107,8 +113,8 @@ export class Patch {
107113
};
108114
}
109115

110-
public async updateRows(tableId: string, delta: TableDelta): Promise<Changes> {
111-
const changes: Changes = [];
116+
public async updateRows(tableId: string, delta: TableDelta): Promise<PatchItem[]> {
117+
const changes: PatchItem[] = [];
112118
const rows = remaining(delta.updateRows, undefined); //, delta.accepted?.updateRows);
113119
const columnDeltas = delta.columnDeltas;
114120
for (const row of rows) {
@@ -128,8 +134,8 @@ export class Patch {
128134
return changes;
129135
}
130136

131-
public async addRows(tableId: string, delta: TableDelta): Promise<Changes> {
132-
const changes: Changes = [];
137+
public async addRows(tableId: string, delta: TableDelta): Promise<PatchItem[]> {
138+
const changes: PatchItem[] = [];
133139
const rows = remaining(delta.addRows, undefined); //delta.accepted?.addRows);
134140
const columnDeltas = delta.columnDeltas;
135141
for (const row of rows) {
@@ -149,8 +155,8 @@ export class Patch {
149155
return changes;
150156
}
151157

152-
public async removeRows(tableId: string, delta: TableDelta): Promise<Changes> {
153-
const changes: Changes = [];
158+
public async removeRows(tableId: string, delta: TableDelta): Promise<PatchItem[]> {
159+
const changes: PatchItem[] = [];
154160
const rows = remaining(delta.removeRows, undefined); //delta.accepted?.removeRows);
155161
const columnDeltas = delta.columnDeltas;
156162
for (const row of rows) {

0 commit comments

Comments
 (0)