Merge pull request #1442 from nextcloud-libraries/fix/column

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

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`)
+		}
+	}
+}