fix(column): validate interface rather than the instance type

We cannot use `instanceof` because two apps will have different
libraries so the prototype is not the same and the check will always
fail.
Instead validate the column using the already existing validation method.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Author: susnux

__tests__/view.spec.ts

@@ -81,6 +81,17 @@ describe('Invalid View creation', () => {
 		).toThrowError('View order must be a number')
 	})
 	test('Invalid columns', () => {
+		expect(() => new View({
+			id: 'test',
+			name: 'Test',
+			order: 1,
+			icon: '<svg></svg>',
+			getContents: () => Promise.reject(new Error()),
+			columns: [{}],
+		} as unknown as View),
+		).toThrowError('A column id is required')
+	})
+	test('Invalid columns with null', () => {
 		expect(() => new View({
 			id: 'test',
 			name: 'Test',
@@ -89,7 +100,7 @@ describe('Invalid View creation', () => {
 			getContents: () => Promise.reject(new Error()),
 			columns: [null],
 		} as unknown as View),
-		).toThrowError('View columns must be an array of Column. Invalid column found')
+		).toThrowError('View column must be an object')
 	})
 	test('Invalid emptyView', () => {
 		expect(() => new View({

lib/navigation/column.ts

@@ -6,6 +6,8 @@
 import type { INode } from '../node/node.ts'
 import type { IView } from './view.ts'
 
+import { checkOptionalProperty } from '../utils/objectValidation.ts'
+
 interface ColumnData {
 	/** Unique column ID */
 	id: string
@@ -27,7 +29,7 @@ export class Column implements ColumnData {
 	private _column: ColumnData
 
 	constructor(column: ColumnData) {
-		isValidColumn(column)
+		validateColumn(column)
 		this._column = column
 	}
 
@@ -54,13 +56,16 @@ export class Column implements ColumnData {
 }
 
 /**
- * Typescript cannot validate an interface.
- * Please keep in sync with the Column interface requirements.
+ * Validate a column definition
  *
- * @param {ColumnData} column the column to check
- * @return {boolean} true if the column is valid
+ * @param column - the column to check
+ * @throws {Error} if the column is not valid
  */
-const isValidColumn = function(column: ColumnData): boolean {
+export function validateColumn(column: ColumnData): void {
+	if (typeof column !== 'object' || column === null) {
+		throw new Error('View column must be an object')
+	}
+
 	if (!column.id || typeof column.id !== 'string') {
 		throw new Error('A column id is required')
 	}
@@ -74,13 +79,6 @@ const isValidColumn = function(column: ColumnData): boolean {
 	}
 
 	// Optional properties
-	if (column.sort && typeof column.sort !== 'function') {
-		throw new Error('Column sortFunction must be a function')
-	}
-
-	if (column.summary && typeof column.summary !== 'function') {
-		throw new Error('Column summary must be a function')
-	}
-
-	return true
+	checkOptionalProperty(column, 'sort', 'function')
+	checkOptionalProperty(column, 'summary', 'function')
 }

lib/navigation/view.ts

@@ -6,7 +6,8 @@
 import type { IFolder, INode } from '../node/index.ts'
 
 import isSvg from 'is-svg'
-import { Column } from './column.ts'
+import { Column, validateColumn } from './column.ts'
+import { checkOptionalProperty } from '../utils/objectValidation.ts'
 
 export type ContentsWithRoot = {
 	folder: IFolder,
@@ -241,37 +242,13 @@ export function validateView(view: IView) {
 	checkOptionalProperty(view, 'sticky', 'boolean')
 
 	if (view.columns) {
-		view.columns.forEach((column) => {
-			if (!(column instanceof Column)) {
-				throw new Error('View columns must be an array of Column. Invalid column found')
-			}
-		})
-	}
-}
-
-/**
- * Check an optional property type
- *
- * @param obj - the object to check
- * @param property - the property name
- * @param type - the expected type
- * @throws {Error} if the property is defined and not of the expected type
- */
-function checkOptionalProperty(
-	obj: Partial<IView>,
-	property: keyof IView,
-	type: 'array' | 'function' | 'string' | 'boolean' | 'number' | 'object',
-): void {
-	if (typeof obj[property] !== 'undefined') {
-		if (type === 'array') {
-			if (!Array.isArray(obj[property])) {
-				throw new Error(`View ${property} must be an array`)
-			}
-		// eslint-disable-next-line valid-typeof
-		} else if (typeof obj[property] !== type) {
-			throw new Error(`View ${property} must be a ${type}`)
-		} else if (type === 'object' && (obj[property] === null || Array.isArray(obj[property]))) {
-			throw new Error(`View ${property} must be an object`)
+		// we cannot use `instanceof` here because if the Navigation and the Column class are loaded by different apps
+		// (Navigation is set by files app and Column by a 3rd party app),
+		// the `instanceof` check will fail even if the object has the correct shape because they are different classes in memory.
+		view.columns.forEach(validateColumn)
+		const columnIds = view.columns.reduce((set, column) => set.add(column.id), new Set<string>())
+		if (columnIds.size !== view.columns.length) {
+			throw new Error('View columns must have unique ids')
 		}
 	}
 }

lib/utils/objectValidation.ts

@@ -0,0 +1,31 @@
+/*
+ * SPDX-FileCopyrightText: Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+/**
+ * Check an optional property type
+ *
+ * @param obj - the object to check
+ * @param property - the property name
+ * @param type - the expected type
+ * @throws {Error} if the property is defined and not of the expected type
+ */
+export function checkOptionalProperty<T extends object>(
+	obj: T,
+	property: Exclude<keyof T, symbol>,
+	type: 'array' | 'function' | 'string' | 'boolean' | 'number' | 'object',
+): void {
+	if (typeof obj[property] !== 'undefined') {
+		if (type === 'array') {
+			if (!Array.isArray(obj[property])) {
+				throw new Error(`View ${property} must be an array`)
+			}
+		// eslint-disable-next-line valid-typeof
+		} else if (typeof obj[property] !== type) {
+			throw new Error(`View ${property} must be a ${type}`)
+		} else if (type === 'object' && (obj[property] === null || Array.isArray(obj[property]))) {
+			throw new Error(`View ${property} must be an object`)
+		}
+	}
+}