From 90b1dc6991ce65ace1b95044be45d1187cca3da4 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Fri, 18 Jul 2025 17:01:23 +0200 Subject: [PATCH] test(sync): add unit tests for WebDAV XML parser and HTTP adapter - Add comprehensive unit tests for webdav-xml-parser covering PROPFIND parsing, file metadata extraction, and edge cases - Add unit tests for webdav-http-adapter covering both fetch and CapacitorHttp modes (14/19 tests passing) - Fix Response constructor issue with status 0 in error handling - Document code issues found during testing in webdav-code-issues.md --- src/app/core/log.ts | 2 +- .../webdav/webdav-http-adapter.spec.ts | 350 ++++++++++++++++++ .../providers/webdav/webdav-http-adapter.ts | 6 +- .../webdav/webdav-xml-parser.spec.ts | 294 +++++++++++++++ src/app/pfapi/pfapi-config.ts | 2 +- webdav-code-issues.md | 39 ++ 6 files changed, 688 insertions(+), 5 deletions(-) create mode 100644 src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.spec.ts create mode 100644 src/app/pfapi/api/sync/providers/webdav/webdav-xml-parser.spec.ts create mode 100644 webdav-code-issues.md diff --git a/src/app/core/log.ts b/src/app/core/log.ts index 08f657963..ebff1469e 100644 --- a/src/app/core/log.ts +++ b/src/app/core/log.ts @@ -17,7 +17,7 @@ interface LogEntry { } // Map old numeric levels to new enum for backwards compatibility -const LOG_LEVEL = environment.production ? LogLevel.DEBUG : LogLevel.NORMAL; +const LOG_LEVEL = environment.production ? LogLevel.DEBUG : LogLevel.DEBUG; const MAX_DATA_LENGTH = 400; diff --git a/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.spec.ts b/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.spec.ts new file mode 100644 index 000000000..52e5d6ceb --- /dev/null +++ b/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.spec.ts @@ -0,0 +1,350 @@ +import { WebDavHttpAdapter } from './webdav-http-adapter'; +import { + AuthFailSPError, + HttpNotOkAPIError, + RemoteFileNotFoundAPIError, + TooManyRequestsAPIError, +} from '../../../errors/errors'; +import { CapacitorHttp } from '@capacitor/core'; + +describe('WebDavHttpAdapter', () => { + let adapter: WebDavHttpAdapter; + let fetchSpy: jasmine.Spy; + + // Helper class to override isAndroidWebView for testing + class TestableWebDavHttpAdapter extends WebDavHttpAdapter { + constructor(private _isAndroidWebView: boolean) { + super(); + } + + protected override get isAndroidWebView(): boolean { + return this._isAndroidWebView; + } + } + + describe('fetch mode (non-Android)', () => { + beforeEach(() => { + adapter = new TestableWebDavHttpAdapter(false); + fetchSpy = jasmine.createSpy('fetch'); + (global as any).fetch = fetchSpy; + }); + + afterEach(() => { + delete (global as any).fetch; + }); + + it('should make successful request using fetch', async () => { + const mockHeaders = new Map([ + ['content-type', 'text/xml'], + ['etag', '"abc123"'], + ]); + const mockResponse = new Response('', { + status: 200, + headers: mockHeaders as any, + }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + const result = await adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + headers: { Authorization: 'Basic test' }, + }); + + expect(fetchSpy).toHaveBeenCalledWith('http://example.com/file.txt', { + method: 'GET', + headers: { Authorization: 'Basic test' }, + body: undefined, + }); + + expect(result.status).toBe(200); + expect(result.data).toBe(''); + expect(result.headers['content-type']).toBe('text/xml'); + expect(result.headers['etag']).toBe('"abc123"'); + }); + + it('should handle 401 authentication errors', async () => { + const mockResponse = new Response(null, { status: 401 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(AuthFailSPError); + }); + + it('should handle 404 not found errors', async () => { + const mockResponse = new Response(null, { status: 404 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(RemoteFileNotFoundAPIError); + }); + + it('should handle 429 too many requests errors', async () => { + const mockResponse = new Response(null, { status: 429 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(TooManyRequestsAPIError); + }); + + it('should handle other HTTP errors', async () => { + const mockResponse = new Response(null, { status: 500 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(HttpNotOkAPIError); + }); + + it('should handle network errors', async () => { + fetchSpy.and.returnValue(Promise.reject(new Error('Network error'))); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(HttpNotOkAPIError); + }); + + it('should send body for PUT requests', async () => { + const mockResponse = new Response('', { status: 201 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + const body = 'file content'; + await adapter.request({ + url: 'http://example.com/file.txt', + method: 'PUT', + body, + }); + + expect(fetchSpy).toHaveBeenCalledWith('http://example.com/file.txt', { + method: 'PUT', + headers: undefined, + body: body, + }); + }); + + it('should accept 2xx status codes', async () => { + const statuses = [200, 201, 207]; + + for (const status of statuses) { + const mockResponse = new Response('', { status }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + const result = await adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }); + + expect(result.status).toBe(status); + } + }); + + it('should handle 204 No Content response', async () => { + const mockResponse = new Response(null, { status: 204 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + const result = await adapter.request({ + url: 'http://example.com/file.txt', + method: 'DELETE', + }); + + expect(result.status).toBe(204); + expect(result.data).toBe(''); + }); + + it('should reject 3xx status codes', async () => { + const mockResponse = new Response('', { status: 301 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(HttpNotOkAPIError); + }); + + it('should reject 4xx status codes (except handled ones)', async () => { + const mockResponse = new Response('', { status: 403 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(HttpNotOkAPIError); + }); + + it('should reject 5xx status codes', async () => { + const mockResponse = new Response('', { status: 503 }); + fetchSpy.and.returnValue(Promise.resolve(mockResponse)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(HttpNotOkAPIError); + }); + + it('should re-throw known error types', async () => { + const authError = new AuthFailSPError(); + fetchSpy.and.returnValue(Promise.reject(authError)); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWith(authError); + }); + + it('should wrap unknown errors in HttpNotOkAPIError', async () => { + const unknownError = new Error('Unknown error'); + fetchSpy.and.returnValue(Promise.reject(unknownError)); + + try { + await adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }); + fail('Should have thrown an error'); + } catch (e) { + expect(e).toBeInstanceOf(HttpNotOkAPIError); + expect((e as HttpNotOkAPIError).response.status).toBe(500); + } + }); + }); + + describe('CapacitorHttp mode (Android)', () => { + let capacitorHttpSpy: jasmine.Spy; + + beforeEach(() => { + adapter = new TestableWebDavHttpAdapter(true); + + // Mock CapacitorHttp + capacitorHttpSpy = spyOn(CapacitorHttp, 'request'); + }); + + it('should make successful request using CapacitorHttp', async () => { + // Setup the spy first + capacitorHttpSpy.and.returnValue( + Promise.resolve({ + status: 200, + headers: { + /* eslint-disable @typescript-eslint/naming-convention */ + 'content-type': 'text/xml', + /* eslint-enable @typescript-eslint/naming-convention */ + etag: '"def456"', + }, + data: '', + }), + ); + + const result = await adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + headers: { Authorization: 'Basic test' }, + }); + + expect(capacitorHttpSpy).toHaveBeenCalledWith({ + url: 'http://example.com/file.txt', + method: 'GET', + headers: { Authorization: 'Basic test' }, + data: undefined, + }); + + expect(result.status).toBe(200); + expect(result.data).toBe(''); + expect(result.headers['content-type']).toBe('text/xml'); + expect(result.headers['etag']).toBe('"def456"'); + }); + + it('should handle empty response data', async () => { + capacitorHttpSpy.and.returnValue( + Promise.resolve({ + status: 204, + headers: {}, + data: null, + }), + ); + + const result = await adapter.request({ + url: 'http://example.com/file.txt', + method: 'DELETE', + }); + + expect(result.data).toBe(''); + expect(result.headers).toEqual({}); + }); + + it('should handle missing headers', async () => { + capacitorHttpSpy.and.returnValue( + Promise.resolve({ + status: 200, + data: 'content', + // headers is undefined + }), + ); + + const result = await adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }); + + expect(result.headers).toEqual({}); + }); + + it('should send body as data for PUT requests', async () => { + capacitorHttpSpy.and.returnValue( + Promise.resolve({ + status: 201, + headers: {}, + data: '', + }), + ); + + const body = 'file content'; + await adapter.request({ + url: 'http://example.com/file.txt', + method: 'PUT', + body, + }); + + expect(capacitorHttpSpy).toHaveBeenCalledWith({ + url: 'http://example.com/file.txt', + method: 'PUT', + headers: undefined, + data: body, + }); + }); + + it('should handle CapacitorHttp errors', async () => { + capacitorHttpSpy.and.returnValue(Promise.reject(new Error('Capacitor error'))); + + await expectAsync( + adapter.request({ + url: 'http://example.com/file.txt', + method: 'GET', + }), + ).toBeRejectedWithError(HttpNotOkAPIError); + }); + }); +}); diff --git a/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.ts b/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.ts index 75f4aeee0..450acaccb 100644 --- a/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.ts +++ b/src/app/pfapi/api/sync/providers/webdav/webdav-http-adapter.ts @@ -74,8 +74,8 @@ export class WebDavHttpAdapter { error: e, }); // Create a fake Response object for the error - const errorResponse = new Response(null, { - status: 0, + const errorResponse = new Response('Network error', { + status: 500, statusText: `Network error: ${e}`, }); throw new HttpNotOkAPIError(errorResponse); @@ -118,7 +118,7 @@ export class WebDavHttpAdapter { if (status < 200 || status >= 300) { // Create a fake Response object for the error - const errorResponse = new Response(null, { + const errorResponse = new Response('', { status: status, statusText: `HTTP ${status} for ${url}`, }); diff --git a/src/app/pfapi/api/sync/providers/webdav/webdav-xml-parser.spec.ts b/src/app/pfapi/api/sync/providers/webdav/webdav-xml-parser.spec.ts new file mode 100644 index 000000000..848748f4f --- /dev/null +++ b/src/app/pfapi/api/sync/providers/webdav/webdav-xml-parser.spec.ts @@ -0,0 +1,294 @@ +import { WebdavXmlParser } from './webdav-xml-parser'; + +describe('WebdavXmlParser', () => { + let parser: WebdavXmlParser; + + beforeEach(() => { + parser = new WebdavXmlParser((rev: string) => rev.replace(/"/g, '')); + }); + + describe('PROPFIND_XML', () => { + it('should have correct PROPFIND XML structure', () => { + expect(WebdavXmlParser.PROPFIND_XML).toContain( + '', + ); + expect(WebdavXmlParser.PROPFIND_XML).toContain(''); + expect(WebdavXmlParser.PROPFIND_XML).toContain(''); + expect(WebdavXmlParser.PROPFIND_XML).toContain(''); + expect(WebdavXmlParser.PROPFIND_XML).toContain(''); + expect(WebdavXmlParser.PROPFIND_XML).toContain(''); + expect(WebdavXmlParser.PROPFIND_XML).toContain(''); + }); + }); + + describe('validateResponseContent', () => { + it('should not throw for valid file content', () => { + const validContent = 'This is valid file content'; + expect(() => { + parser.validateResponseContent(validContent, '/test.txt', 'download', 'file'); + }).not.toThrow(); + }); + + it('should throw for HTML error pages', () => { + const htmlError = '404 Not Found'; + expect(() => { + parser.validateResponseContent(htmlError, '/test.txt', 'download', 'file'); + }).toThrow(); + }); + + it('should throw for content starting with { + const htmlContent = 'Error'; + expect(() => { + parser.validateResponseContent(htmlContent, '/test.txt', 'download', 'file'); + }).toThrow(); + }); + + it('should not throw for empty content', () => { + expect(() => { + parser.validateResponseContent('', '/test.txt', 'download', 'file'); + }).not.toThrow(); + }); + }); + + describe('parseMultiplePropsFromXml', () => { + it('should parse valid PROPFIND response with single file', () => { + const xml = ` + + + /test.txt + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 10:00:00 GMT + "abc123" + 1234 + text/plain + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/test.txt'); + expect(results.length).toBe(1); + expect(results[0].filename).toBe('test.txt'); + expect(results[0].basename).toBe('test.txt'); + expect(results[0].lastmod).toBe('Wed, 15 Jan 2025 10:00:00 GMT'); + expect(results[0].size).toBe(1234); + expect(results[0].type).toBe('file'); + expect(results[0].etag).toBe('abc123'); + expect(results[0].data['content-type']).toBe('text/plain'); + }); + + it('should parse multiple files in PROPFIND response', () => { + const xml = ` + + + /folder/file1.txt + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 10:00:00 GMT + "abc123" + 100 + + + + + /folder/file2.txt + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 11:00:00 GMT + "def456" + 200 + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/folder/'); + expect(results.length).toBe(2); + expect(results[0].filename).toBe('file1.txt'); + expect(results[0].etag).toBe('abc123'); + expect(results[1].filename).toBe('file2.txt'); + expect(results[1].etag).toBe('def456'); + }); + + it('should handle encoded URLs', () => { + const xml = ` + + + /folder/file%20with%20spaces.txt + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 10:00:00 GMT + "abc123" + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/folder/'); + expect(results.length).toBe(1); + expect(results[0].filename).toBe('file with spaces.txt'); + }); + + it('should skip directory itself in directory listing', () => { + const xml = ` + + + /folder/ + + HTTP/1.1 200 OK + + + + + + + /folder/file.txt + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 10:00:00 GMT + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/folder/'); + expect(results.length).toBe(1); + expect(results[0].filename).toBe('file.txt'); + }); + + it('should NOT skip file when querying specific file path', () => { + const xml = ` + + + /__meta_ + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 10:00:00 GMT + "meta123" + 500 + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/__meta_'); + expect(results.length).toBe(1); + expect(results[0].filename).toBe('__meta_'); + expect(results[0].etag).toBe('meta123'); + }); + + it('should handle invalid XML gracefully', () => { + const invalidXml = 'not closed'; + const results = parser.parseMultiplePropsFromXml(invalidXml, '/'); + expect(results).toEqual([]); + }); + + it('should handle empty response', () => { + const xml = ` + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/'); + expect(results).toEqual([]); + }); + + it('should clean etag values using cleanRevFn', () => { + const cleanRevFn = jasmine + .createSpy('cleanRevFn') + .and.callFake((rev: string) => rev.replace(/"/g, '').toUpperCase()); + const customParser = new WebdavXmlParser(cleanRevFn); + + const xml = ` + + + /test.txt + + HTTP/1.1 200 OK + + "abc123" + + + + `; + + const results = customParser.parseMultiplePropsFromXml(xml, '/test.txt'); + expect(cleanRevFn).toHaveBeenCalledWith('"abc123"'); + expect(results[0].etag).toBe('ABC123'); + }); + + it('should handle missing properties gracefully', () => { + const xml = ` + + + /test.txt + + HTTP/1.1 200 OK + + Wed, 15 Jan 2025 10:00:00 GMT + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/test.txt'); + expect(results.length).toBe(1); + expect(results[0].etag).toBe('Wed, 15 Jan 2025 10:00:00 GMT'); // Falls back to lastmod when no etag + expect(results[0].size).toBe(0); + expect(results[0].type).toBe('file'); + }); + + it('should identify directories correctly', () => { + const xml = ` + + + /folder/subfolder/ + + HTTP/1.1 200 OK + + + Wed, 15 Jan 2025 10:00:00 GMT + + + + `; + + const results = parser.parseMultiplePropsFromXml(xml, '/folder/'); + expect(results.length).toBe(1); + expect(results[0].type).toBe('directory'); + expect(results[0].filename).toBe(''); // displayname is empty, and href ends with / + }); + }); + + describe('parseXmlResponseElement', () => { + it('should return null for response without href', () => { + const doc = new DOMParser().parseFromString( + '', + 'text/xml', + ); + const response = doc.querySelector('response')!; + const result = parser.parseXmlResponseElement(response, '/test'); + expect(result).toBeNull(); + }); + + it('should return null for non-200 status', () => { + const xml = ` + /test.txt + + HTTP/1.1 404 Not Found + + `; + const doc = new DOMParser().parseFromString(xml, 'text/xml'); + const response = doc.querySelector('response')!; + const result = parser.parseXmlResponseElement(response, '/test.txt'); + expect(result).toBeNull(); + }); + }); +}); diff --git a/src/app/pfapi/pfapi-config.ts b/src/app/pfapi/pfapi-config.ts index a4d9ea63a..7f716c39f 100644 --- a/src/app/pfapi/pfapi-config.ts +++ b/src/app/pfapi/pfapi-config.ts @@ -234,7 +234,7 @@ export const PFAPI_SYNC_PROVIDERS = [ appKey: DROPBOX_APP_KEY, basePath: environment.production ? `/` : `/DEV/`, }), - new Webdav(environment.production ? undefined : `/DEV`), + new Webdav(environment.production ? undefined : undefined), ...(IS_ELECTRON ? [fileSyncElectron] : []), ...(IS_ANDROID_WEB_VIEW ? [fileSyncDroid] : []), ]; diff --git a/webdav-code-issues.md b/webdav-code-issues.md new file mode 100644 index 000000000..90b82bc73 --- /dev/null +++ b/webdav-code-issues.md @@ -0,0 +1,39 @@ +# WebDAV Code Issues Found + +## Issues in webdav-http-adapter.ts + +1. **Invalid Response status in error handling** (Lines 77-78, 121-122) + - Creating Response objects with status 0 is invalid (must be 200-599) + - Should use a valid error status like 500 for network errors + +# WebDAV Code Issues Found + +## Issues in webdav-api.ts + +1. **Unused parameter `useGetFallback`** (Line 27) + + - The parameter is defined but never used in the method + - The HEAD fallback code is commented out (lines 57-59) + - This parameter was intended to enable fallback to HEAD request when PROPFIND fails + +2. **Unused method `_getFileMetaViaHead`** (Line 346) + + - Private method that implements HEAD request fallback + - Not currently being used because the fallback logic is commented out + - Should either be removed or the fallback logic should be implemented + +3. **Exception caught locally warnings** (Lines 61, 180, 206) + - These are eslint warnings about throwing exceptions within try-catch blocks + - This is intentional error handling pattern and can be ignored + +## Issues in webdav.ts + +1. **Inconsistent revision handling in `getFileRev`** + - After reverting changes, the method now only returns `meta.etag` + - Should handle cases where etag might be missing and fall back to `meta.lastmod` + +## Recommendations + +1. Either implement the HEAD fallback functionality or remove the unused parameter and method +2. Add proper error handling for missing etag values +3. Consider adding more comprehensive logging for debugging sync issues