feat(operations): add a put operation, and fix the target-database authorization mismatch

dataLayer/harperBridge/ResourceBridge.ts

@@ -8,6 +8,7 @@ import {
 	VALUE_SEARCH_COMPARATORS,
 	VALUE_SEARCH_COMPARATORS_REVERSE_LOOKUP,
 	READ_AUDIT_LOG_SEARCH_TYPES_ENUM,
+	UNSET_ATTRIBUTES as OPERATIONS_UNSET_KEY,
 } from '../../utility/hdbTerms.ts';
 import * as signalling from '../../utility/signalling.ts';
 import { SchemaEventMsg } from '../../server/threads/itc.js';
@@ -23,7 +24,7 @@ import type {
 	Operator,
 } from '../../resources/ResourceInterface.ts';
 import { collapseData } from '../../resources/tracked.ts';
-import { errorToString } from '../../utility/logging/harper_logger.ts';
+import logger, { errorToString } from '../../utility/logging/harper_logger.ts';
 import { RocksDatabase } from '@harperfast/rocksdb-js';
 import { BridgeMethods } from './BridgeMethods.ts';
 import lmdbGetBackup from './lmdbBridge/lmdbMethods/lmdbGetBackup.js';
@@ -35,6 +36,11 @@ const { HDB_ERROR_MSGS } = hdbErrors;
 const DEFAULT_DATABASE = 'data';
 const DELETE_CHUNK = 10000;
 const DELETE_PAUSE_MS = 10;
+/** Shared result for the overwhelmingly common case of a record carrying no `__unset__`, so the
+ * default write path allocates nothing per record. Frozen because it is handed out repeatedly. */
+const NO_UNSET_ATTRIBUTES = Object.freeze([]) as unknown as string[];
+/** Latch for the malformed-directive warning; see takeUnsetAttributes. */
+let warnedMalformedUnset = false;
 
 export type SearchByConditionsRequest = Query &
 	Context & {
@@ -203,6 +209,23 @@ export class ResourceBridge extends BridgeMethods {
 		const { attributes } = insertUpdateValidate(upsertObj);
 
 		let new_attributes;
+		// `full_record: true` makes this a full replace instead of a merge: an attribute absent from
+		// the submitted record is REMOVED rather than left at its stored value. Without it, a write
+		// over an existing record lands on `Table.patch`, so there is no way to drop an attribute
+		// through the operations API — omitting it keeps the stored value and `null` stores a null
+		// (HarperFast/studio#1643).
+		//
+		// The flag is orthogonal to each operation's create rule, which is what makes one flag cover
+		// both useful shapes: `update` still requires an existing record (a missing one is skipped),
+		// while `upsert` still creates one — so `upsert` + `full_record` is exactly REST's
+		// `PUT /Table/id`, and `update` + `full_record` is that same replace, refused if the record
+		// isn't there. It has no effect on `insert`, which never writes over an existing record.
+		//
+		// Compared with the delete-then-insert this replaces, a full replace is one operation rather
+		// than two: the record is never absent between them, subscribers see a single write instead of
+		// a delete followed by an insert, and `__createdtime__` survives (`Table._writeUpdate` retains
+		// the stored created time on a full update and only stamps a new one for a new entry).
+		const fullRecord = upsertObj.full_record === true;
 		const Table = getDatabases()[upsertObj.schema][upsertObj.table];
 		const context: Context = {
 			user: upsertObj.hdb_user,
@@ -259,12 +282,38 @@ export class ResourceBridge extends BridgeMethods {
 						}
 					}
 				}
+				// Keyed on whether there IS a record to remove from, not on which operation asked. Guarding
+				// the insert flag alone left `upsert` with no primary key reaching the same silent partial
+				// write one branch over: `insertUpdateValidate` requires a hash attribute only for
+				// `update`, so such a record takes the `id == undefined → Table.create` path and stored an
+				// auto-keyed record with the named attributes stripped, returning 200.
+				const unset = takeUnsetAttributes(record, Table, !existingRecord);
+				// `__unset__` removes named attributes while everything else still merges, which a patch
+				// cannot express — so it resolves the merge here, against the record this transaction
+				// already loaded, and writes the result as a full replace. Doing it server-side inside the
+				// write transaction is the point: a client emulating this with read-then-replace races
+				// every other writer, and `full_record` would make the caller resend attributes it never
+				// meant to touch. Under `full_record` the submitted record is already the whole intended
+				// state, so there is nothing to merge and the names are simply dropped from it.
+				//
+				// SINGLE-WRITER ONLY, and the surrounding wording should not be read as more than that.
+				// Resolving the merge here still writes a `put`, and a full put is last-writer-wins over
+				// the whole record: out-of-order reconciliation folds field-wise only for patches
+				// (`resources/Table.ts:2902-2914`) and the commit-retry path re-reads the stored record
+				// only when `!fullUpdate` (`resources/Table.ts:2530`). So a concurrent write to an
+				// attribute this request never named can be lost — the opposite of what "everything else
+				// merges" implies. Tracked in HarperFast/harper#2350; fixing it properly means resolving
+				// removals at the patch layer instead.
+				const toWrite = unset.length && existingRecord && !fullRecord ? { ...existingRecord, ...record } : record;
+				for (const attribute of unset) {
+					delete toWrite[attribute];
+				}
 				await (id == undefined
-					? Table.create(record, context)
-					: existingRecord
-						? Table.patch(record, context)
-						: Table.put(record, context));
-				keys.push(record[Table.primaryKey]);
+					? Table.create(toWrite, context)
+					: existingRecord && !fullRecord && !unset.length
+						? Table.patch(toWrite, context)
+						: Table.put(toWrite, context));
+				keys.push(toWrite[Table.primaryKey]);
 			}
 			return {
 				txn_time: (transaction as any).timestamp,
@@ -771,3 +820,57 @@ async function* groupRecordsInHistory(table, start?, end?, limit?) {
 	}
 	if (enqueued) yield enqueued;
 }
+
+/**
+ * Read and remove a record's `__unset__` list, returning the attribute names to drop.
+ *
+ * The key is removed because it is a directive, not data — left in place it would be stored as an
+ * attribute. The refusals below live here rather than in the validator because only this layer knows
+ * the table: which attribute is the primary key, and what the managed timestamps are called. Each is
+ * refused rather than skipped, because `_writeUpdate` re-asserts all three on a full update, so
+ * accepting them would return 200 and change nothing.
+ */
+function takeUnsetAttributes(record: Record<string, unknown>, Table: any, hasNoExistingRecord: boolean): string[] {
+	if (!(OPERATIONS_UNSET_KEY in record)) return NO_UNSET_ATTRIBUTES;
+	const unset = record[OPERATIONS_UNSET_KEY];
+	// Before the shape is judged, and on every exit: `in` rather than a value check, so an
+	// explicitly-undefined key still comes off the record.
+	delete record[OPERATIONS_UNSET_KEY];
+	if (hasNoExistingRecord) {
+		throw new ClientError(
+			`'${OPERATIONS_UNSET_KEY}' needs an existing record to remove attributes from; nothing is stored under this primary key`
+		);
+	}
+	// Ignored with a warning rather than thrown: this is the write and replication apply path, where
+	// throwing aborts the commit and can wedge a subscription, and skipping the directive degrades to
+	// a plain merge, which removes nothing. `resources/tracked.ts` makes the same choice for an
+	// unrecognized CRDT operation. Checked at all because the shape is validated one layer up, in a
+	// module this one does not own: a non-iterable would throw in the caller's `for…of` and take the
+	// write down, and a bare string would iterate per character, deleting single-letter attributes.
+	if (!Array.isArray(unset) || unset.some((name) => typeof name !== 'string' || name.length === 0)) {
+		// Latched, following `warnedNullSourcePut` in resources/Table.ts: one line per record would
+		// bury the log under a single bad batch.
+		if (!warnedMalformedUnset) {
+			warnedMalformedUnset = true;
+			logger.warn(
+				`Ignoring a malformed '${OPERATIONS_UNSET_KEY}' on a record in ${Table.databaseName}.${Table.tableName}; expected an array of attribute names`
+			);
+		}
+		return NO_UNSET_ATTRIBUTES;
+	}
+	const names = unset as string[];
+	const primaryKey = Table.primaryKey;
+	if (primaryKey && names.includes(primaryKey)) {
+		throw new ClientError(`'${primaryKey}' is the primary key of this table and cannot be unset`);
+	}
+	// `createdTimeProperty`/`updatedTimeProperty` resolve `assignCreatedTime` OR the legacy
+	// `__createdtime__` spelling (resources/Table.ts), so this covers a schema-declared
+	// `createdAt: Float @createdTime` as well as the legacy names — which a static list in the
+	// validator, with no schema in scope, could not.
+	for (const managed of [Table.createdTimeProperty, Table.updatedTimeProperty]) {
+		if (managed?.name && names.includes(managed.name)) {
+			throw new ClientError(`'${managed.name}' is maintained by Harper and cannot be unset`);
+		}
+	}
+	return names;
+}

dataLayer/harperBridge/bridgeUtility/insertUpdateValidate.js

@@ -4,6 +4,7 @@ const hdbUtils = require('../../../utility/common_utils.ts');
 const log = require('../../../utility/logging/harper_logger.ts');
 const { getDatabases } = require('../../../resources/databases.ts');
 const { ClientError } = require('../../../utility/errors/hdbError.ts');
+const { UNSET_ATTRIBUTES } = require('../../../utility/hdbTerms.ts');
 
 module.exports = insertUpdateValidate;
 
@@ -73,6 +74,15 @@ function insertUpdateValidate(writeObject) {
 		dups.add(hdbUtils.autoCast(record[hash_attribute]));
 
 		for (let attr in record) {
+			// `__unset__` names attributes to remove; it is not one itself. Collecting it would make an
+			// open (non-schemaDefined) table register `__unset__` as a real attribute, since
+			// upsertRecords feeds this list straight to Table.addAttributes.
+			//
+			// Deliberately does NOT add the names it removes, unlike the async twin's list in
+			// dataLayer/insert.ts: that one is the bulk-load attribute-permission input, where a removal
+			// must be authorized, while this one creates table attributes — registering an attribute in
+			// order to delete it would be backwards. Keep both comments in step.
+			if (attr === UNSET_ATTRIBUTES) continue;
 			attributes[attr] = 1;
 		}
 	});

dataLayer/insert.ts

@@ -15,6 +15,7 @@ import * as globalSchema from '../utility/globalSchema.ts';
 import log from '../utility/logging/harper_logger.ts';
 import { handleHDBError } from '../utility/errors/hdbError.ts';
 import { HTTP_STATUS_CODES } from '../utility/errors/commonErrors.ts';
+import * as terms from '../utility/hdbTerms.ts';
 
 const pGlobalSchema = util.promisify(globalSchema.getTableSchema);
 
@@ -89,6 +90,32 @@ export async function validation(writeObject: any) {
 		dups.add(hdbUtils.autoCast(record[hash_attribute]));
 
 		for (let attr in record) {
+			// `__unset__` names attributes to remove and is not one itself, so the key never counts as
+			// an attribute — but the names it removes DO. This list is the bulk-load path's
+			// attribute-permission input: `bulkLoad.validateChunk` hands it to
+			// `verifyBulkLoadAttributePerms`, so a name missing from it is a removal nobody authorized.
+			// The direct operations path gets the same treatment from `getRecordAttributes`
+			// (utility/operation_authorization.ts); without it here, `import_from_s3` with
+			// `action: "update"` could unset an attribute the role has no permission to write while the
+			// equivalent `update` returns 403.
+			//
+			// Contributed for every action, not just update/upsert: requiring the permission is the safe
+			// direction, and `__unset__` is refused outright on an insert anyway (see
+			// ResourceBridge.takeUnsetAttributes).
+			//
+			// The sync twin in harperBridge/bridgeUtility/insertUpdateValidate.js feeds
+			// `Table.addAttributes` instead, so it must skip the key WITHOUT adding the removed names —
+			// registering an attribute in order to delete it would be backwards. The two lists are
+			// deliberately not identical; keep both comments in step.
+			if (attr === terms.UNSET_ATTRIBUTES) {
+				const unset = record[attr];
+				if (Array.isArray(unset)) {
+					for (const name of unset) {
+						if (typeof name === 'string' && name.length > 0) attributes[name] = 1;
+					}
+				}
+				continue;
+			}
 			attributes[attr] = 1;
 		}
 	});