Skip to content

Commit

Permalink
[api-major] Replace MissingPDFException and `UnexpectedResponseExce…
Browse files Browse the repository at this point in the history 
…ption` 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.
  • Loading branch information
@Snuffleupagus
Snuffleupagus committed Jan 2, 2025
1 parent 5905eb1 commit eda9788
Show file tree
Hide file tree
Showing 14 changed files with 63 additions and 92 deletions.
8 changes: 4 additions & 4 deletions examples/mobile-viewer/viewer.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
6 changes: 3 additions & 3 deletions src/display/fetch_stream.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import { AbortException, assert, warn } from "../shared/util.js";
import {
createHeaders,
createResponseStatusError,
createResponseError,
extractFilenameFromHeader,
getResponseOrigin,
validateRangeRequestCapabilities,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
6 changes: 3 additions & 3 deletions src/display/network.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import { assert, stringToBytes, warn } from "../shared/util.js";
import {
createHeaders,
createResponseStatusError,
createResponseError,
extractFilenameFromHeader,
getResponseOrigin,
validateRangeRequestCapabilities,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
18 changes: 6 additions & 12 deletions src/display/network_utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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:"))
);
}

Expand All @@ -124,7 +118,7 @@ function validateResponseStatus(status) {

export {
createHeaders,
createResponseStatusError,
createResponseError,
extractFilenameFromHeader,
getResponseOrigin,
validateRangeRequestCapabilities,
Expand Down
5 changes: 3 additions & 2 deletions src/display/node_stream.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 2 additions & 4 deletions src/pdf.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,12 @@ import {
FeatureTest,
ImageKind,
InvalidPDFException,
MissingPDFException,
normalizeUnicode,
OPS,
PasswordResponses,
PermissionFlag,
ResponseException,
shadow,
UnexpectedResponseException,
Util,
VerbosityLevel,
} from "./shared/util.js";
Expand Down Expand Up @@ -112,7 +111,6 @@ export {
InvalidPDFException,
isDataScheme,
isPdfFile,
MissingPDFException,
noContextMenu,
normalizeUnicode,
OPS,
Expand All @@ -124,12 +122,12 @@ export {
PermissionFlag,
PixelsPerInch,
RenderingCancelledException,
ResponseException,
setLayerDimensions,
shadow,
stopEvent,
TextLayer,
TouchManager,
UnexpectedResponseException,
Util,
VerbosityLevel,
version,
Expand Down
12 changes: 4 additions & 8 deletions src/shared/message_handler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@ import {
AbortException,
assert,
InvalidPDFException,
MissingPDFException,
PasswordException,
UnexpectedResponseException,
ResponseException,
UnknownErrorException,
unreachable,
} from "./util.js";
Expand All @@ -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.
Expand All @@ -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);
}
Expand Down
16 changes: 5 additions & 11 deletions src/shared/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -1161,7 +1156,6 @@ export {
LINE_DESCENT_FACTOR,
LINE_FACTOR,
MAX_IMAGE_SIZE_TO_CACHE,
MissingPDFException,
normalizeUnicode,
objectFromMap,
objectSize,
Expand All @@ -1171,6 +1165,7 @@ export {
PasswordResponses,
PermissionFlag,
RenderingIntentFlag,
ResponseException,
setVerbosityLevel,
shadow,
string32,
Expand All @@ -1180,7 +1175,6 @@ export {
TextRenderingMode,
toBase64Util,
toHexUtil,
UnexpectedResponseException,
UnknownErrorException,
unreachable,
utf8StringToString,
Expand Down
6 changes: 4 additions & 2 deletions test/unit/api_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ import {
ImageKind,
InvalidPDFException,
isNodeJS,
MissingPDFException,
objectSize,
OPS,
PasswordException,
PasswordResponses,
PermissionFlag,
ResponseException,
UnknownErrorException,
} from "../../src/shared/util.js";
import {
Expand Down Expand Up @@ -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();
Expand Down
7 changes: 2 additions & 5 deletions test/unit/network_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
}
}
Expand Down
42 changes: 18 additions & 24 deletions test/unit/network_utils_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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);
});
});

Expand Down
6 changes: 2 additions & 4 deletions test/unit/pdf_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,12 @@ import {
ImageKind,
InvalidPDFException,
isNodeJS,
MissingPDFException,
normalizeUnicode,
OPS,
PasswordResponses,
PermissionFlag,
ResponseException,
shadow,
UnexpectedResponseException,
Util,
VerbosityLevel,
} from "../../src/shared/util.js";
Expand Down Expand Up @@ -90,7 +89,6 @@ const expectedAPI = Object.freeze({
InvalidPDFException,
isDataScheme,
isPdfFile,
MissingPDFException,
noContextMenu,
normalizeUnicode,
OPS,
Expand All @@ -102,12 +100,12 @@ const expectedAPI = Object.freeze({
PermissionFlag,
PixelsPerInch,
RenderingCancelledException,
ResponseException,
setLayerDimensions,
shadow,
stopEvent,
TextLayer,
TouchManager,
UnexpectedResponseException,
Util,
VerbosityLevel,
version,
Expand Down
Loading

0 comments on commit eda9788

Please sign in to comment.