fix(view): ensure all optional properties are validated

This fixes an issue when using the `View` class as in this case the
check `'property' in view` will always return true,
thus the typecheck would fail when using the view class and
`navigation.register`.

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

Author: susnux

__tests__/fixtures/view.ts

@@ -0,0 +1,39 @@
+/*!
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import { Folder, View } from '../../lib/index.ts'
+
+/**
+ * Creates a mock View and its associated Folder for testing purposes.
+ */
+export function mockView() {
+	const folder = new Folder({
+		source: 'https://example.org/dav/files/admin/',
+		root: '/files/admin',
+		owner: 'admin',
+	})
+
+	const view = new View({
+		id: 'test',
+		name: 'Test',
+		caption: 'Test caption',
+		emptyTitle: 'Test empty title',
+		emptyCaption: 'Test empty caption',
+		getContents: () => Promise.resolve({ folder, contents: [] }),
+		hidden: true,
+		icon: '<svg></svg>',
+		order: 1,
+		params: {},
+		columns: [],
+		emptyView: () => {},
+		parent: 'parent',
+		sticky: false,
+		expanded: false,
+		defaultSortKey: 'key',
+		loadChildViews: async () => {},
+	})
+
+	return { folder, view }
+}

__tests__/navigation.spec.ts

@@ -2,9 +2,11 @@
  * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
+
 import { describe, it, expect, vi } from 'vitest'
 import { Navigation, getNavigation } from '../lib/navigation/navigation'
-import { mockView } from './view.spec'
+import { mockView } from './fixtures/view.ts'
+import { View } from '../lib/index.ts'
 
 describe('getNavigation', () => {
 	it('creates a new navigation if needed', () => {
@@ -44,6 +46,19 @@ describe('Navigation', () => {
 		expect(navigation.views).toEqual([view])
 	})
 
+	it('Can register a view with only required files', async () => {
+		const view = new View({
+			id: 'minimal',
+			name: 'Minimal view',
+			icon: '<svg></svg>',
+			getContents: () => Promise.reject(new Error('Not implemented')),
+		})
+
+		const navigation = new Navigation()
+		navigation.register(view)
+		expect(navigation.views).toEqual([view])
+	})
+
 	it('Throws when trying to register invalid view', async () => {
 		const navigation = new Navigation()
 		expect(() => {

__tests__/view.spec.ts

@@ -5,8 +5,8 @@
 
 import { describe, expect, test } from 'vitest'
 
-import { View } from '../lib/navigation/view.ts'
-import { Folder } from '../lib/index.ts'
+import { IView, View } from '../lib/navigation/view.ts'
+import { mockView } from './fixtures/view.ts'
 
 describe('Invalid View creation', () => {
 	test('Invalid id', () => {
@@ -63,10 +63,10 @@ describe('Invalid View creation', () => {
 		expect(() => new View({
 			id: 'test',
 			name: 'Test',
-			order: 1,
-			hidden: 'true',
+			hidden: 'yes',
+			icon: '<svg></svg>',
 			getContents: () => Promise.reject(new Error()),
-		} as unknown as View),
+		} as unknown as IView),
 		).toThrowError('View hidden must be a boolean')
 	})
 
@@ -157,6 +157,28 @@ describe('Invalid View creation', () => {
 		} as unknown as View),
 		).toThrowError('View loadChildViews must be a function')
 	})
+	test('Invalid params', () => {
+		expect(() => new View({
+			id: 'test',
+			name: 'Test',
+			order: 1,
+			icon: '<svg></svg>',
+			getContents: () => Promise.reject(new Error()),
+			params: [],
+		} as unknown as View),
+		).toThrowError('View params must be an object')
+	})
+	test('Invalid params with null', () => {
+		expect(() => new View({
+			id: 'test',
+			name: 'Test',
+			order: 1,
+			icon: '<svg></svg>',
+			getContents: () => Promise.reject(new Error()),
+			params: null,
+		} as unknown as View),
+		).toThrowError('View params must be an object')
+	})
 })
 
 describe('View creation', () => {
@@ -167,7 +189,7 @@ describe('View creation', () => {
 		expect(view.caption).toBe('Test caption')
 		expect(view.emptyTitle).toBe('Test empty title')
 		expect(view.emptyCaption).toBe('Test empty caption')
-		await expect(view.getContents('/')).resolves.toStrictEqual({ folder, contents: [] })
+		await expect(view.getContents('/', { signal: new AbortController().signal })).resolves.toStrictEqual({ folder, contents: [] })
 		expect(view.hidden).toBe(true)
 		expect(view.icon).toBe('<svg></svg>')
 		expect(view.order).toBe(1)
@@ -181,36 +203,3 @@ describe('View creation', () => {
 		await expect(view.loadChildViews?.({} as unknown as View)).resolves.toBe(undefined)
 	})
 })
-
-/**
- * Creates a mock View and its associated Folder for testing purposes.
- */
-export function mockView() {
-	const folder = new Folder({
-		source: 'https://example.org/dav/files/admin/',
-		root: '/files/admin',
-		owner: 'admin',
-	})
-
-	const view = new View({
-		id: 'test',
-		name: 'Test',
-		caption: 'Test caption',
-		emptyTitle: 'Test empty title',
-		emptyCaption: 'Test empty caption',
-		getContents: () => Promise.resolve({ folder, contents: [] }),
-		hidden: true,
-		icon: '<svg></svg>',
-		order: 1,
-		params: {},
-		columns: [],
-		emptyView: () => {},
-		parent: 'parent',
-		sticky: false,
-		expanded: false,
-		defaultSortKey: 'key',
-		loadChildViews: async () => {},
-	})
-
-	return { folder, view }
-}

lib/navigation/view.ts

@@ -208,64 +208,70 @@ export class View implements IView {
  * @throws {Error} if the view is not valid
  */
 export function validateView(view: IView) {
-	if (!view.id || typeof view.id !== 'string') {
-		throw new Error('View id is required and must be a string')
-	}
-
-	if (!view.name || typeof view.name !== 'string') {
-		throw new Error('View name is required and must be a string')
+	if (!view.icon || typeof view.icon !== 'string' || !isSvg(view.icon)) {
+		throw new Error('View icon is required and must be a valid svg string')
 	}
 
-	if ('caption' in view && typeof view.caption !== 'string') {
-		throw new Error('View caption must be a string')
+	if (!view.id || typeof view.id !== 'string') {
+		throw new Error('View id is required and must be a string')
 	}
 
 	if (!view.getContents || typeof view.getContents !== 'function') {
 		throw new Error('View getContents is required and must be a function')
 	}
 
-	if ('hidden' in view && typeof view.hidden !== 'boolean') {
-		throw new Error('View hidden must be a boolean')
+	if (!view.name || typeof view.name !== 'string') {
+		throw new Error('View name is required and must be a string')
 	}
 
-	if (!view.icon || typeof view.icon !== 'string' || !isSvg(view.icon)) {
-		throw new Error('View icon is required and must be a valid svg string')
-	}
+	// optional properties type checks
 
-	if ('order' in view && typeof view.order !== 'number') {
-		throw new Error('View order must be a number')
-	}
+	checkOptionalProperty(view, 'caption', 'string')
+	checkOptionalProperty(view, 'columns', 'array')
+	checkOptionalProperty(view, 'defaultSortKey', 'string')
+	checkOptionalProperty(view, 'emptyCaption', 'string')
+	checkOptionalProperty(view, 'emptyTitle', 'string')
+	checkOptionalProperty(view, 'emptyView', 'function')
+	checkOptionalProperty(view, 'expanded', 'boolean')
+	checkOptionalProperty(view, 'hidden', 'boolean')
+	checkOptionalProperty(view, 'loadChildViews', 'function')
+	checkOptionalProperty(view, 'order', 'number')
+	checkOptionalProperty(view, 'params', 'object')
+	checkOptionalProperty(view, 'parent', 'string')
+	checkOptionalProperty(view, 'sticky', 'boolean')
 
-	// Optional properties
 	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')
 			}
 		})
 	}
+}
 
-	if (view.emptyView && typeof view.emptyView !== 'function') {
-		throw new Error('View emptyView must be a function')
-	}
-
-	if (view.parent && typeof view.parent !== 'string') {
-		throw new Error('View parent must be a string')
-	}
-
-	if ('sticky' in view && typeof view.sticky !== 'boolean') {
-		throw new Error('View sticky must be a boolean')
-	}
-
-	if ('expanded' in view && typeof view.expanded !== 'boolean') {
-		throw new Error('View expanded must be a boolean')
-	}
-
-	if (view.defaultSortKey && typeof view.defaultSortKey !== 'string') {
-		throw new Error('View defaultSortKey must be a string')
-	}
-
-	if (view.loadChildViews && typeof view.loadChildViews !== 'function') {
-		throw new Error('View loadChildViews must be a function')
+/**
+ * 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`)
+		}
 	}
 }