From 9f49568776dd1ce3a02396d66ed3f1d8639f7803 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 28 Dec 2024 14:20:48 +0100 Subject: [PATCH] [api-major] Replace `MissingPDFException` and `UnexpectedResponseException` with one exception These old exceptions have a fair amount of overlap given how/where they are being used, which is likely because they were introduced at different points in time, hence we can shorten and simplify the code by replacing them with a more general `ResponseException` instead. Besides an error message, the new `ResponseException` instances also include: - A numeric `status` field containing the server response status, similar to the old `UnexpectedResponseException`. - A boolean `missing` field, to allow easily detecting the situations where `MissingPDFException` was previously thrown. --- examples/mobile-viewer/viewer.mjs | 8 +++--- src/display/fetch_stream.js | 6 ++--- src/display/network.js | 6 ++--- src/display/network_utils.js | 18 +++++-------- src/display/node_stream.js | 5 ++-- src/pdf.js | 6 ++--- src/shared/message_handler.js | 12 +++------ src/shared/util.js | 16 ++++-------- test/unit/api_spec.js | 6 +++-- test/unit/network_spec.js | 7 ++---- test/unit/network_utils_spec.js | 42 +++++++++++++------------------ test/unit/pdf_spec.js | 6 ++--- web/app.js | 11 ++++---- web/pdfjs.js | 6 ++--- 14 files changed, 63 insertions(+), 92 deletions(-) diff --git a/examples/mobile-viewer/viewer.mjs b/examples/mobile-viewer/viewer.mjs index fedd5d8e23ead..bf496c91e62aa 100644 --- a/examples/mobile-viewer/viewer.mjs +++ b/examples/mobile-viewer/viewer.mjs @@ -91,10 +91,10 @@ const PDFViewerApplication = { let key = "pdfjs-loading-error"; if (reason instanceof pdfjsLib.InvalidPDFException) { key = "pdfjs-invalid-file-error"; - } else if (reason instanceof pdfjsLib.MissingPDFException) { - key = "pdfjs-missing-file-error"; - } else if (reason instanceof pdfjsLib.UnexpectedResponseException) { - key = "pdfjs-unexpected-response-error"; + } else if (reason instanceof pdfjsLib.ResponseException) { + key = reason.missing + ? "pdfjs-missing-file-error" + : "pdfjs-unexpected-response-error"; } self.l10n.get(key).then(msg => { self.error(msg, { message: reason?.message }); diff --git a/src/display/fetch_stream.js b/src/display/fetch_stream.js index 24604e43b68df..acbf3c868e37e 100644 --- a/src/display/fetch_stream.js +++ b/src/display/fetch_stream.js @@ -16,7 +16,7 @@ import { AbortException, assert, warn } from "../shared/util.js"; import { createHeaders, - createResponseStatusError, + createResponseError, extractFilenameFromHeader, getResponseOrigin, validateRangeRequestCapabilities, @@ -127,7 +127,7 @@ class PDFFetchStreamReader { stream._responseOrigin = getResponseOrigin(response.url); if (!validateResponseStatus(response.status)) { - throw createResponseStatusError(response.status, url); + throw createResponseError(response.status, url); } this._reader = response.body.getReader(); this._headersCapability.resolve(); @@ -230,7 +230,7 @@ class PDFFetchStreamRangeReader { ); } if (!validateResponseStatus(response.status)) { - throw createResponseStatusError(response.status, url); + throw createResponseError(response.status, url); } this._readCapability.resolve(); this._reader = response.body.getReader(); diff --git a/src/display/network.js b/src/display/network.js index 8b65a5a27e675..34fa53ab92f5d 100644 --- a/src/display/network.js +++ b/src/display/network.js @@ -16,7 +16,7 @@ import { assert, stringToBytes, warn } from "../shared/util.js"; import { createHeaders, - createResponseStatusError, + createResponseError, extractFilenameFromHeader, getResponseOrigin, validateRangeRequestCapabilities, @@ -329,7 +329,7 @@ class PDFNetworkStreamFullRequestReader { } _onError(status) { - this._storedError = createResponseStatusError(status, this._url); + this._storedError = createResponseError(status, this._url); this._headersCapability.reject(this._storedError); for (const requestCapability of this._requests) { requestCapability.reject(this._storedError); @@ -454,7 +454,7 @@ class PDFNetworkStreamRangeRequestReader { } _onError(status) { - this._storedError ??= createResponseStatusError(status, this._url); + this._storedError ??= createResponseError(status, this._url); for (const requestCapability of this._requests) { requestCapability.reject(this._storedError); } diff --git a/src/display/network_utils.js b/src/display/network_utils.js index 98ba103fdba43..f7b2df6143430 100644 --- a/src/display/network_utils.js +++ b/src/display/network_utils.js @@ -13,11 +13,7 @@ * limitations under the License. */ -import { - assert, - MissingPDFException, - UnexpectedResponseException, -} from "../shared/util.js"; +import { assert, ResponseException } from "../shared/util.js"; import { getFilenameFromContentDispositionHeader } from "./content_disposition.js"; import { isPdfFile } from "./display_utils.js"; @@ -108,13 +104,11 @@ function extractFilenameFromHeader(responseHeaders) { return null; } -function createResponseStatusError(status, url) { - if (status === 404 || (status === 0 && url.startsWith("file:"))) { - return new MissingPDFException('Missing PDF "' + url + '".'); - } - return new UnexpectedResponseException( +function createResponseError(status, url) { + return new ResponseException( `Unexpected server response (${status}) while retrieving PDF "${url}".`, - status + status, + /* missing = */ status === 404 || (status === 0 && url.startsWith("file:")) ); } @@ -124,7 +118,7 @@ function validateResponseStatus(status) { export { createHeaders, - createResponseStatusError, + createResponseError, extractFilenameFromHeader, getResponseOrigin, validateRangeRequestCapabilities, diff --git a/src/display/node_stream.js b/src/display/node_stream.js index fdc686d732788..36f1b1a8dd4bb 100644 --- a/src/display/node_stream.js +++ b/src/display/node_stream.js @@ -14,7 +14,8 @@ */ /* globals process */ -import { AbortException, assert, MissingPDFException } from "../shared/util.js"; +import { AbortException, assert } from "../shared/util.js"; +import { createResponseError } from "./network_utils.js"; if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")) { throw new Error( @@ -111,7 +112,7 @@ class PDFNodeStreamFsFullReader { }, error => { if (error.code === "ENOENT") { - error = new MissingPDFException(`Missing PDF "${this._url}".`); + error = createResponseError(/* status = */ 0, this._url.href); } this._storedError = error; this._headersCapability.reject(error); diff --git a/src/pdf.js b/src/pdf.js index 94d7547f0f832..757e97e4ccf11 100644 --- a/src/pdf.js +++ b/src/pdf.js @@ -31,13 +31,12 @@ import { FeatureTest, ImageKind, InvalidPDFException, - MissingPDFException, normalizeUnicode, OPS, PasswordResponses, PermissionFlag, + ResponseException, shadow, - UnexpectedResponseException, Util, VerbosityLevel, } from "./shared/util.js"; @@ -112,7 +111,6 @@ export { InvalidPDFException, isDataScheme, isPdfFile, - MissingPDFException, noContextMenu, normalizeUnicode, OPS, @@ -124,12 +122,12 @@ export { PermissionFlag, PixelsPerInch, RenderingCancelledException, + ResponseException, setLayerDimensions, shadow, stopEvent, TextLayer, TouchManager, - UnexpectedResponseException, Util, VerbosityLevel, version, diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js index 9ea0e5f79e030..a1af0aab27b54 100644 --- a/src/shared/message_handler.js +++ b/src/shared/message_handler.js @@ -17,9 +17,8 @@ import { AbortException, assert, InvalidPDFException, - MissingPDFException, PasswordException, - UnexpectedResponseException, + ResponseException, UnknownErrorException, unreachable, } from "./util.js"; @@ -46,9 +45,8 @@ function wrapReason(ex) { if ( ex instanceof AbortException || ex instanceof InvalidPDFException || - ex instanceof MissingPDFException || ex instanceof PasswordException || - ex instanceof UnexpectedResponseException || + ex instanceof ResponseException || ex instanceof UnknownErrorException ) { // Avoid re-creating the exception when its type is already correct. @@ -65,12 +63,10 @@ function wrapReason(ex) { return new AbortException(ex.message); case "InvalidPDFException": return new InvalidPDFException(ex.message); - case "MissingPDFException": - return new MissingPDFException(ex.message); case "PasswordException": return new PasswordException(ex.message, ex.code); - case "UnexpectedResponseException": - return new UnexpectedResponseException(ex.message, ex.status); + case "ResponseException": + return new ResponseException(ex.message, ex.status, ex.missing); case "UnknownErrorException": return new UnknownErrorException(ex.message, ex.details); } diff --git a/src/shared/util.js b/src/shared/util.js index e0f66716ab9ca..b4f8824b365b6 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -501,16 +501,11 @@ class InvalidPDFException extends BaseException { } } -class MissingPDFException extends BaseException { - constructor(msg) { - super(msg, "MissingPDFException"); - } -} - -class UnexpectedResponseException extends BaseException { - constructor(msg, status) { - super(msg, "UnexpectedResponseException"); +class ResponseException extends BaseException { + constructor(msg, status, missing) { + super(msg, "ResponseException"); this.status = status; + this.missing = missing; } } @@ -1161,7 +1156,6 @@ export { LINE_DESCENT_FACTOR, LINE_FACTOR, MAX_IMAGE_SIZE_TO_CACHE, - MissingPDFException, normalizeUnicode, objectFromMap, objectSize, @@ -1171,6 +1165,7 @@ export { PasswordResponses, PermissionFlag, RenderingIntentFlag, + ResponseException, setVerbosityLevel, shadow, string32, @@ -1180,7 +1175,6 @@ export { TextRenderingMode, toBase64Util, toHexUtil, - UnexpectedResponseException, UnknownErrorException, unreachable, utf8StringToString, diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 4cf594cd78bf4..c364472ea2cd1 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -20,12 +20,12 @@ import { ImageKind, InvalidPDFException, isNodeJS, - MissingPDFException, objectSize, OPS, PasswordException, PasswordResponses, PermissionFlag, + ResponseException, UnknownErrorException, } from "../../src/shared/util.js"; import { @@ -297,7 +297,9 @@ describe("api", function () { // Shouldn't get here. expect(false).toEqual(true); } catch (reason) { - expect(reason instanceof MissingPDFException).toEqual(true); + expect(reason instanceof ResponseException).toEqual(true); + expect(reason.status).toEqual(isNodeJS ? 0 : 404); + expect(reason.missing).toEqual(true); } await loadingTask.destroy(); diff --git a/test/unit/network_spec.js b/test/unit/network_spec.js index 0cdc5dbe5eafb..a640c074a4108 100644 --- a/test/unit/network_spec.js +++ b/test/unit/network_spec.js @@ -13,10 +13,7 @@ * limitations under the License. */ -import { - AbortException, - UnexpectedResponseException, -} from "../../src/shared/util.js"; +import { AbortException, ResponseException } from "../../src/shared/util.js"; import { PDFNetworkStream } from "../../src/display/network.js"; import { testCrossOriginRedirects } from "./common_pdfstream_tests.js"; import { TestPdfsServer } from "./test_utils.js"; @@ -155,7 +152,7 @@ describe("network", function () { // Shouldn't get here. expect(false).toEqual(true); } catch (ex) { - expect(ex instanceof UnexpectedResponseException).toEqual(true); + expect(ex instanceof ResponseException).toEqual(true); expect(ex.status).toEqual(0); } } diff --git a/test/unit/network_utils_spec.js b/test/unit/network_utils_spec.js index e697e24deeb74..2b68c4dbf36fa 100644 --- a/test/unit/network_utils_spec.js +++ b/test/unit/network_utils_spec.js @@ -15,15 +15,12 @@ import { createHeaders, - createResponseStatusError, + createResponseError, extractFilenameFromHeader, validateRangeRequestCapabilities, validateResponseStatus, } from "../../src/display/network_utils.js"; -import { - MissingPDFException, - UnexpectedResponseException, -} from "../../src/shared/util.js"; +import { ResponseException } from "../../src/shared/util.js"; describe("network_utils", function () { describe("createHeaders", function () { @@ -370,31 +367,28 @@ describe("network_utils", function () { }); }); - describe("createResponseStatusError", function () { - it("handles missing PDF file responses", function () { - expect(createResponseStatusError(404, "https://foo.com/bar.pdf")).toEqual( - new MissingPDFException('Missing PDF "https://foo.com/bar.pdf".') - ); + describe("createResponseError", function () { + function testCreateResponseError(url, status, missing) { + const error = createResponseError(status, url); - expect(createResponseStatusError(0, "file://foo.pdf")).toEqual( - new MissingPDFException('Missing PDF "file://foo.pdf".') + expect(error instanceof ResponseException).toEqual(true); + expect(error.message).toEqual( + `Unexpected server response (${status}) while retrieving PDF "${url}".` ); + expect(error.status).toEqual(status); + expect(error.missing).toEqual(missing); + } + + it("handles missing PDF file responses", function () { + testCreateResponseError("https://foo.com/bar.pdf", 404, true); + + testCreateResponseError("file://foo.pdf", 0, true); }); it("handles unexpected responses", function () { - expect(createResponseStatusError(302, "https://foo.com/bar.pdf")).toEqual( - new UnexpectedResponseException( - "Unexpected server response (302) while retrieving PDF " + - '"https://foo.com/bar.pdf".' - ) - ); + testCreateResponseError("https://foo.com/bar.pdf", 302, false); - expect(createResponseStatusError(0, "https://foo.com/bar.pdf")).toEqual( - new UnexpectedResponseException( - "Unexpected server response (0) while retrieving PDF " + - '"https://foo.com/bar.pdf".' - ) - ); + testCreateResponseError("https://foo.com/bar.pdf", 0, false); }); }); diff --git a/test/unit/pdf_spec.js b/test/unit/pdf_spec.js index d4a32c48a22ce..a513b8a4f1fe0 100644 --- a/test/unit/pdf_spec.js +++ b/test/unit/pdf_spec.js @@ -23,13 +23,12 @@ import { ImageKind, InvalidPDFException, isNodeJS, - MissingPDFException, normalizeUnicode, OPS, PasswordResponses, PermissionFlag, + ResponseException, shadow, - UnexpectedResponseException, Util, VerbosityLevel, } from "../../src/shared/util.js"; @@ -90,7 +89,6 @@ const expectedAPI = Object.freeze({ InvalidPDFException, isDataScheme, isPdfFile, - MissingPDFException, noContextMenu, normalizeUnicode, OPS, @@ -102,12 +100,12 @@ const expectedAPI = Object.freeze({ PermissionFlag, PixelsPerInch, RenderingCancelledException, + ResponseException, setLayerDimensions, shadow, stopEvent, TextLayer, TouchManager, - UnexpectedResponseException, Util, VerbosityLevel, version, diff --git a/web/app.js b/web/app.js index 109318be687f5..a935e8eb1f658 100644 --- a/web/app.js +++ b/web/app.js @@ -50,12 +50,11 @@ import { InvalidPDFException, isDataScheme, isPdfFile, - MissingPDFException, PDFWorker, + ResponseException, shadow, stopEvent, TouchManager, - UnexpectedResponseException, version, } from "pdfjs-lib"; import { AppOptions, OptionKind } from "./app_options.js"; @@ -1108,10 +1107,10 @@ const PDFViewerApplication = { let key = "pdfjs-loading-error"; if (reason instanceof InvalidPDFException) { key = "pdfjs-invalid-file-error"; - } else if (reason instanceof MissingPDFException) { - key = "pdfjs-missing-file-error"; - } else if (reason instanceof UnexpectedResponseException) { - key = "pdfjs-unexpected-response-error"; + } else if (reason instanceof ResponseException) { + key = reason.missing + ? "pdfjs-missing-file-error" + : "pdfjs-unexpected-response-error"; } return this._documentError(key, { message: reason.message }).then( () => { diff --git a/web/pdfjs.js b/web/pdfjs.js index a9fe6ca9ac4a2..7ac129513be5a 100644 --- a/web/pdfjs.js +++ b/web/pdfjs.js @@ -37,7 +37,6 @@ const { InvalidPDFException, isDataScheme, isPdfFile, - MissingPDFException, noContextMenu, normalizeUnicode, OPS, @@ -49,12 +48,12 @@ const { PermissionFlag, PixelsPerInch, RenderingCancelledException, + ResponseException, setLayerDimensions, shadow, stopEvent, TextLayer, TouchManager, - UnexpectedResponseException, Util, VerbosityLevel, version, @@ -85,7 +84,6 @@ export { InvalidPDFException, isDataScheme, isPdfFile, - MissingPDFException, noContextMenu, normalizeUnicode, OPS, @@ -97,12 +95,12 @@ export { PermissionFlag, PixelsPerInch, RenderingCancelledException, + ResponseException, setLayerDimensions, shadow, stopEvent, TextLayer, TouchManager, - UnexpectedResponseException, Util, VerbosityLevel, version,