From d26a7f64abb583ecc5473e2c5b6caf57dadd47d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jind=C5=99ich=20B=C3=A4r?= Date: Fri, 28 Aug 2026 09:21:05 +0200 Subject: [PATCH 1/2] feat(core): pass Request to onSkippedRequest (#3992) Skipped requests now get the full `Request` instead of just a URL string, so `userData` and other metadata survive a skip; the `Request` is built lazily so crawlers that don't use `onSkippedRequest` pay nothing for it. Closes #3867 Closes #3801 --- docs/public-api/crawlee-core.api.md | 2 +- docs/upgrading/upgrading_v4.md | 4 ++ .../src/internals/basic-crawler.ts | 39 ++++++++++------ packages/core/src/enqueue_links/shared.ts | 40 ++++++++++++++--- .../internals/enqueue-links/click-elements.ts | 18 +++++--- .../internals/enqueue-links/click-elements.ts | 18 +++++--- test/core/crawlers/basic_crawler.test.ts | 44 ++++++++++++------- test/core/enqueue_links/enqueue_links.test.ts | 14 +++--- .../actor/main.js | 3 +- 9 files changed, 123 insertions(+), 59 deletions(-) diff --git a/docs/public-api/crawlee-core.api.md b/docs/public-api/crawlee-core.api.md index 785777d49b16..f324e51f80b7 100644 --- a/docs/public-api/crawlee-core.api.md +++ b/docs/public-api/crawlee-core.api.md @@ -1797,7 +1797,7 @@ export interface SitemapRequestLoaderOptions extends UrlConstraints { // @public (undocumented) export type SkippedRequestCallback = (args: { - url: string; + request: Request_2; reason: SkippedRequestReason; }) => Awaitable; diff --git a/docs/upgrading/upgrading_v4.md b/docs/upgrading/upgrading_v4.md index b7312d3e5f7d..1dba7cc3611f 100644 --- a/docs/upgrading/upgrading_v4.md +++ b/docs/upgrading/upgrading_v4.md @@ -843,6 +843,10 @@ const urls = await extractLinks({ selector: '.product-link' }); The `robotsTxtFile` / `respectRobotsTxtFile` per-call options are removed from `enqueueLinks()` — robots.txt filtering is applied by the crawler consistently via `BasicCrawlerOptions.respectRobotsTxtFile`. +### `onSkippedRequest` receives a `Request` instead of a URL string + +The callback now gets `{ request, reason }` instead of `{ url, reason }` — use `request.url` for the URL. + ### Internal KVS keys renamed Several internal Crawlee keys were prefixed with the `SDK_` prefix for legacy reasons — these keys now start with `CRAWLEE_` instead. These are, e.g., `CRAWLEE_SESSION_POOL_STATE` or `CRAWLEE_CRAWLER_STATISTICS_{n}`. diff --git a/packages/basic-crawler/src/internals/basic-crawler.ts b/packages/basic-crawler/src/internals/basic-crawler.ts index 64c7923ab5c0..a3d8399477b1 100644 --- a/packages/basic-crawler/src/internals/basic-crawler.ts +++ b/packages/basic-crawler/src/internals/basic-crawler.ts @@ -18,6 +18,7 @@ import type { IRequestLoader, IRequestManager, IStatistics, + RequestOptions, RequestsLike, RouterHandler, RouterRoutes, @@ -45,6 +46,7 @@ import { ContextPipelineInitializationError, ContextPipelineInterruptedError, createRequestOptions, + createSkippedRequestArgs, createStorageTransaction, Request, CriticalError, @@ -1454,7 +1456,7 @@ export class BasicCrawler< request.state = RequestState.SKIPPED; request.noRetry = true; await this.#handleSkippedRequest({ - url: request.url, + request, reason: 'robotsTxt', }); @@ -1587,7 +1589,7 @@ export class BasicCrawler< request.noRetry = true; request.state = RequestState.SKIPPED; - await this.#handleSkippedRequest({ url: request.url, reason: 'redirect' }); + await this.#handleSkippedRequest({ request, reason: 'redirect' }); throw new ContextPipelineInterruptedError(message); } @@ -2099,7 +2101,12 @@ export class BasicCrawler< const maxCrawlDepth = this.#maxCrawlDepth; const validateRequestUserData = this.validateRequestUserData.bind(this); - const allSkipped: { url: string; reason: SkippedRequestReason }[] = []; + const allSkipped: { source: string | Source; reason: SkippedRequestReason }[] = []; + // A skipped source (which can carry arbitrary userData) is only retained if something reads it - + // otherwise the URL alone is enough to build the callback argument and to log with. + const hasSkippedRequestCallback = + this.#onSkippedRequest !== undefined || options.onSkippedRequest !== undefined; + const keepSkippedSource = (source: Source) => (hasSkippedRequestCallback ? source : source.url!); async function* filteredRequests() { for await (const request of requests) { @@ -2113,16 +2120,17 @@ export class BasicCrawler< } if (maxCrawlDepth !== undefined && requestOptions.crawlDepth! > maxCrawlDepth) { - allSkipped.push({ url: requestOptions.url, reason: 'depth' }); + allSkipped.push({ source: keepSkippedSource(requestOptions), reason: 'depth' }); continue; } if (!(await isAllowedBasedOnRobotsTxtFile(requestOptions.url))) { - allSkipped.push({ url: requestOptions.url, reason: 'robotsTxt' }); + allSkipped.push({ source: keepSkippedSource(requestOptions), reason: 'robotsTxt' }); continue; } - const onSkippedFilterUrl = (url: string) => allSkipped.push({ url, reason: 'filters' }); + const onSkippedByFilter = (opts: RequestOptions) => + allSkipped.push({ source: keepSkippedSource(opts), reason: 'filters' }); // Filter by user patterns first (with exclude)... let filtered = filterRequestOptionsByPatterns( @@ -2130,7 +2138,7 @@ export class BasicCrawler< urlPatternObjects.length > 0 ? urlPatternObjects : undefined, urlExcludePatternObjects, strategy, - onSkippedFilterUrl, + onSkippedByFilter, ); // ...then filter by the enqueue strategy (making this an AND check) filtered = filterRequestOptionsByPatterns( @@ -2138,7 +2146,7 @@ export class BasicCrawler< enqueueStrategyPatterns.length > 0 ? enqueueStrategyPatterns : undefined, [], strategy, - onSkippedFilterUrl, + onSkippedByFilter, ); if (filtered.length === 0) { @@ -2149,7 +2157,7 @@ export class BasicCrawler< if (options.transformRequestFunction) { const transformed = applyRequestTransform([finalOptions], options.transformRequestFunction, (r) => - allSkipped.push({ url: r.url, reason: r.skippedReason ?? 'transform' }), + allSkipped.push({ source: keepSkippedSource(r), reason: r.skippedReason ?? 'transform' }), ); if (transformed.length === 0) { @@ -2175,11 +2183,13 @@ export class BasicCrawler< // Report requests skipped due to the maxNewRequests budget (i.e. maxRequestsPerCrawl limit, or an // explicit `limit` option) for (const request of result.requestsOverLimit ?? []) { - allSkipped.push({ url: typeof request === 'string' ? request : request.url!, reason: 'limit' }); + allSkipped.push({ source: request, reason: 'limit' }); } if (allSkipped.length > 0) { - const skippedRobotsUrls = allSkipped.filter((s) => s.reason === 'robotsTxt').map((s) => s.url); + const skippedRobotsUrls = allSkipped + .filter((s) => s.reason === 'robotsTxt') + .map(({ source }) => (typeof source === 'string' ? source : source.url!)); if (skippedRobotsUrls.length > 0) { this.log.warning( `Some requests were skipped because they were disallowed based on the robots.txt file`, @@ -2198,9 +2208,10 @@ export class BasicCrawler< } await Promise.all( - allSkipped.map(async ({ url, reason }) => { - await this.#handleSkippedRequest({ url, reason }); - await options.onSkippedRequest?.({ url, reason }); + allSkipped.map(async ({ source, reason }) => { + const args = createSkippedRequestArgs(source, reason); + await this.#handleSkippedRequest(args); + await options.onSkippedRequest?.(args); }), ); } diff --git a/packages/core/src/enqueue_links/shared.ts b/packages/core/src/enqueue_links/shared.ts index f86fb9d47234..b1804ecf47bd 100644 --- a/packages/core/src/enqueue_links/shared.ts +++ b/packages/core/src/enqueue_links/shared.ts @@ -4,7 +4,8 @@ import type { Awaitable, Dictionary } from '@crawlee/types'; import { Minimatch } from 'minimatch'; import { z } from 'zod'; -import type { RequestOptions } from '../request.js'; +import type { RequestOptions, Source } from '../request.js'; +import { Request } from '../request.js'; import { schemas } from '../validators.js'; import type { EnqueueStrategyOption } from './enqueue_links.js'; @@ -57,7 +58,32 @@ export type SkippedRequestReason = | 'redirect' | 'depth'; -export type SkippedRequestCallback = (args: { url: string; reason: SkippedRequestReason }) => Awaitable; +export type SkippedRequestCallback = (args: { request: Request; reason: SkippedRequestReason }) => Awaitable; + +/** + * Builds the `{ request, reason }` argument passed to a {@apilink SkippedRequestCallback}, constructing the + * `Request` lazily on first access to `request` (and caching it) since most skips are never observed by a + * real callback and building a `Request` isn't free. + * @ignore + */ +export function createSkippedRequestArgs( + source: string | Source, + reason: SkippedRequestReason, +): Parameters[0] { + const sourceReason = typeof source === 'string' ? undefined : source.skippedReason; + let request: Request | undefined; + + return { + reason: sourceReason ?? reason, + get request() { + request ??= + source instanceof Request + ? source + : new Request(typeof source === 'string' ? { url: source } : (source as RequestOptions)); + return request; + }, + }; +} /** * @ignore @@ -176,16 +202,16 @@ export function filterRequestOptionsByPatterns( includePatterns: UrlPatternObject[] | undefined, excludePatterns: UrlPatternObject[] = [], strategy?: EnqueueStrategyOption, - onSkippedUrl?: (url: string) => void, + onSkippedRequestOptions?: (options: RequestOptions) => void, ): RequestOptions[] { const excludeMatchers = excludePatterns.map(createPatternObjectMatcher); const includeMatchers = includePatterns?.length ? includePatterns.map(createPatternObjectMatcher) : undefined; return requestOptions - .filter(({ url }) => { - const matchesExclude = excludeMatchers.some(({ match }) => match(url)); + .filter((opts) => { + const matchesExclude = excludeMatchers.some(({ match }) => match(opts.url)); if (matchesExclude) { - onSkippedUrl?.(url); + onSkippedRequestOptions?.(opts); } return !matchesExclude; }) @@ -201,7 +227,7 @@ export function filterRequestOptionsByPatterns( } // didn't match any positive pattern - onSkippedUrl?.(opts.url); + onSkippedRequestOptions?.(opts); return null; }) .filter((opts) => opts !== null); diff --git a/packages/playwright-crawler/src/internals/enqueue-links/click-elements.ts b/packages/playwright-crawler/src/internals/enqueue-links/click-elements.ts index 155043fbd3b1..e5cbcd9694d4 100644 --- a/packages/playwright-crawler/src/internals/enqueue-links/click-elements.ts +++ b/packages/playwright-crawler/src/internals/enqueue-links/click-elements.ts @@ -12,6 +12,7 @@ import { applyRequestTransform, constructUrlPatternObjects, createRequestOptions, + createSkippedRequestArgs, filterRequestOptionsByPatterns, urlPatternSchema, Request as CrawleeRequest, @@ -236,6 +237,7 @@ export async function enqueueLinksByClickingElements( const waitForPageIdleMillis = waitForPageIdleSecs * 1000; const maxWaitForPageIdleMillis = maxWaitForPageIdleSecs * 1000; + const hasOnSkippedRequest = onSkippedRequest !== undefined; const urlExcludePatternObjects: UrlPatternObject[] = exclude?.length ? constructUrlPatternObjects(exclude) : []; const urlPatternObjects: UrlPatternObject[] = include?.length ? constructUrlPatternObjects(include) : []; @@ -248,27 +250,31 @@ export async function enqueueLinksByClickingElements( clickOptions, }); const requestOptions = createRequestOptions(interceptedRequests, parsedOptions); - const skippedByFilters: string[] = []; + const skippedByFilters: RequestOptions[] = []; let filteredOptions = filterRequestOptionsByPatterns( requestOptions, urlPatternObjects.length > 0 ? urlPatternObjects : undefined, urlExcludePatternObjects, undefined, - (url) => skippedByFilters.push(url), + hasOnSkippedRequest ? (opts) => skippedByFilters.push(opts) : undefined, ); if (onSkippedRequest && skippedByFilters.length > 0) { - await Promise.all(skippedByFilters.map(async (url) => onSkippedRequest({ url, reason: 'filters' }))); + await Promise.all( + skippedByFilters.map(async (opts) => onSkippedRequest(createSkippedRequestArgs(opts, 'filters'))), + ); } if (transformRequestFunction) { const skippedByTransform: RequestOptions[] = []; - filteredOptions = applyRequestTransform(filteredOptions, transformRequestFunction, (r) => - skippedByTransform.push(r), + filteredOptions = applyRequestTransform( + filteredOptions, + transformRequestFunction, + hasOnSkippedRequest ? (r) => skippedByTransform.push(r) : undefined, ); if (onSkippedRequest && skippedByTransform.length > 0) { await Promise.all( - skippedByTransform.map(async (r) => onSkippedRequest({ url: r.url, reason: 'transform' })), + skippedByTransform.map(async (r) => onSkippedRequest(createSkippedRequestArgs(r, 'transform'))), ); } } diff --git a/packages/puppeteer-crawler/src/internals/enqueue-links/click-elements.ts b/packages/puppeteer-crawler/src/internals/enqueue-links/click-elements.ts index 5126c8d236a5..f7372962ba30 100644 --- a/packages/puppeteer-crawler/src/internals/enqueue-links/click-elements.ts +++ b/packages/puppeteer-crawler/src/internals/enqueue-links/click-elements.ts @@ -12,6 +12,7 @@ import { applyRequestTransform, constructUrlPatternObjects, createRequestOptions, + createSkippedRequestArgs, filterRequestOptionsByPatterns, urlPatternSchema, Request, @@ -237,6 +238,7 @@ export async function enqueueLinksByClickingElements( const waitForPageIdleMillis = waitForPageIdleSecs * 1000; const maxWaitForPageIdleMillis = maxWaitForPageIdleSecs * 1000; + const hasOnSkippedRequest = onSkippedRequest !== undefined; const urlExcludePatternObjects: UrlPatternObject[] = exclude?.length ? constructUrlPatternObjects(exclude) : []; const urlPatternObjects: UrlPatternObject[] = include?.length ? constructUrlPatternObjects(include) : []; @@ -249,27 +251,31 @@ export async function enqueueLinksByClickingElements( clickOptions, }); const requestOptions = createRequestOptions(interceptedRequests, parsedOptions); - const skippedByFilters: string[] = []; + const skippedByFilters: RequestOptions[] = []; let filteredOptions = filterRequestOptionsByPatterns( requestOptions, urlPatternObjects.length > 0 ? urlPatternObjects : undefined, urlExcludePatternObjects, undefined, - (url) => skippedByFilters.push(url), + hasOnSkippedRequest ? (opts) => skippedByFilters.push(opts) : undefined, ); if (onSkippedRequest && skippedByFilters.length > 0) { - await Promise.all(skippedByFilters.map(async (url) => onSkippedRequest({ url, reason: 'filters' }))); + await Promise.all( + skippedByFilters.map(async (opts) => onSkippedRequest(createSkippedRequestArgs(opts, 'filters'))), + ); } if (transformRequestFunction) { const skippedByTransform: RequestOptions[] = []; - filteredOptions = applyRequestTransform(filteredOptions, transformRequestFunction, (r) => - skippedByTransform.push(r), + filteredOptions = applyRequestTransform( + filteredOptions, + transformRequestFunction, + hasOnSkippedRequest ? (r) => skippedByTransform.push(r) : undefined, ); if (onSkippedRequest && skippedByTransform.length > 0) { await Promise.all( - skippedByTransform.map(async (r) => onSkippedRequest({ url: r.url, reason: 'transform' })), + skippedByTransform.map(async (r) => onSkippedRequest(createSkippedRequestArgs(r, 'transform'))), ); } } diff --git a/test/core/crawlers/basic_crawler.test.ts b/test/core/crawlers/basic_crawler.test.ts index 846e4d5bbb00..b85f01afa8ec 100644 --- a/test/core/crawlers/basic_crawler.test.ts +++ b/test/core/crawlers/basic_crawler.test.ts @@ -535,8 +535,12 @@ describe('BasicCrawler', () => { const skippedRequests = onSkippedRequestMock.mock.calls.map((call) => call[0]); expect(skippedRequests).toHaveLength(2); - expect(skippedRequests[0]).toStrictEqual({ url: 'https://example.com/1/', reason: 'depth' }); - expect(skippedRequests[1]).toStrictEqual({ url: 'https://example.com/2/', reason: 'depth' }); + expect(skippedRequests[0].reason).toBe('depth'); + expect(skippedRequests[0].request).toBeInstanceOf(Request); + expect(skippedRequests[0].request.url).toBe('https://example.com/1/'); + expect(skippedRequests[1].reason).toBe('depth'); + expect(skippedRequests[1].request).toBeInstanceOf(Request); + expect(skippedRequests[1].request.url).toBe('https://example.com/2/'); }); it('should respect user provided transformRequestFunction', async () => { @@ -565,8 +569,10 @@ describe('BasicCrawler', () => { const skippedRequests = onSkippedRequestMock.mock.calls.map((call) => call[0]); expect(skippedRequests).toHaveLength(2); - expect(skippedRequests[0]).toStrictEqual({ url: 'https://example.com/1/', reason: 'transform' }); - expect(skippedRequests[1]).toStrictEqual({ url: 'https://example.com/2/', reason: 'transform' }); + expect(skippedRequests[0].reason).toBe('transform'); + expect(skippedRequests[0].request.url).toBe('https://example.com/1/'); + expect(skippedRequests[1].reason).toBe('transform'); + expect(skippedRequests[1].request.url).toBe('https://example.com/2/'); }, ); @@ -585,8 +591,12 @@ describe('BasicCrawler', () => { // The skipped reason should be 'depth', not 'transform' const skippedRequests = onSkippedRequestMock.mock.calls.map((call) => call[0]); expect(skippedRequests).toHaveLength(2); - expect(skippedRequests[0]).toStrictEqual({ url: 'https://example.com/1/', reason: 'depth' }); - expect(skippedRequests[1]).toStrictEqual({ url: 'https://example.com/2/', reason: 'depth' }); + expect(skippedRequests[0].reason).toBe('depth'); + expect(skippedRequests[0].request).toBeInstanceOf(Request); + expect(skippedRequests[0].request.url).toBe('https://example.com/1/'); + expect(skippedRequests[1].reason).toBe('depth'); + expect(skippedRequests[1].request).toBeInstanceOf(Request); + expect(skippedRequests[1].request.url).toBe('https://example.com/2/'); }); }); @@ -3311,7 +3321,7 @@ describe('BasicCrawler', () => { const skippedUrls = onSkippedRequest.mock.calls .map((call) => call[0]) .filter(({ reason }) => reason === 'limit') - .map(({ url }) => url) + .map(({ request }) => request.url) .sort(); expect(skippedUrls).toEqual([ @@ -3379,15 +3389,15 @@ describe('BasicCrawler', () => { await crawler.run(['http://example.com']); - const skipped = [ - { url: 'http://example.com/2', reason: 'limit' }, - { url: 'http://example.com/3', reason: 'limit' }, - ]; - for (const mock of [crawlerOnSkippedRequest, userOnSkippedRequest]) { - expect(mock.mock.calls.map((call) => call[0]).sort((a, b) => a.url.localeCompare(b.url))).toEqual( - skipped, - ); + const skipped = mock.mock.calls + .map((call) => ({ url: call[0].request.url, reason: call[0].reason })) + .sort((a, b) => a.url.localeCompare(b.url)); + + expect(skipped).toEqual([ + { url: 'http://example.com/2', reason: 'limit' }, + { url: 'http://example.com/3', reason: 'limit' }, + ]); } }); }); @@ -3559,8 +3569,8 @@ describe('BasicCrawler', () => { maxRequestRetries: 0, respectRobotsTxtFile: true, requestHandler: async () => {}, - onSkippedRequest: async ({ url, reason }) => { - await (await KeyValueStore.open()).setValue('skipped', { url, reason }); + onSkippedRequest: async ({ request, reason }) => { + await (await KeyValueStore.open()).setValue('skipped', { url: request.url, reason }); }, }); diff --git a/test/core/enqueue_links/enqueue_links.test.ts b/test/core/enqueue_links/enqueue_links.test.ts index 448bc6210d2f..06772447da46 100644 --- a/test/core/enqueue_links/enqueue_links.test.ts +++ b/test/core/enqueue_links/enqueue_links.test.ts @@ -667,12 +667,12 @@ describe('enqueueLinks()', () => { expect(enqueued[0].userData).toEqual({ label: 'global-label' }); const skippedCalls = onSkippedRequest.mock.calls.map( - (call: unknown[]) => call[0] as { url: string; reason: string }, + (call: unknown[]) => call[0] as { request: Request; reason: string }, ); - const transformSkipped = skippedCalls.filter((s) => s.url === 'https://example.com/a/b/first'); + const transformSkipped = skippedCalls.filter((s) => s.request.url === 'https://example.com/a/b/first'); expect(transformSkipped).toHaveLength(1); - expect(transformSkipped[0]).toEqual({ url: 'https://example.com/a/b/first', reason: 'transform' }); - const unchangedSkipped = skippedCalls.filter((s) => s.url === 'https://example.com/a/b/third'); + expect(transformSkipped[0].reason).toBe('transform'); + const unchangedSkipped = skippedCalls.filter((s) => s.request.url === 'https://example.com/a/b/third'); expect(unchangedSkipped).toHaveLength(0); }); @@ -699,11 +699,11 @@ describe('enqueueLinks()', () => { expect(enqueued[0].url).toBe('https://example.com/a/b/third'); const skippedCalls = onSkippedRequest.mock.calls.map( - (call: unknown[]) => call[0] as { url: string; reason: string }, + (call: unknown[]) => call[0] as { request: Request; reason: string }, ); - const transformSkipped = skippedCalls.filter((s) => s.url === 'https://example.com/a/b/first'); + const transformSkipped = skippedCalls.filter((s) => s.request.url === 'https://example.com/a/b/first'); expect(transformSkipped).toHaveLength(1); - expect(transformSkipped[0]).toEqual({ url: 'https://example.com/a/b/first', reason: 'transform' }); + expect(transformSkipped[0].reason).toBe('transform'); }); }); }); diff --git a/test/e2e/adaptive-playwright-robots-file/actor/main.js b/test/e2e/adaptive-playwright-robots-file/actor/main.js index f2138de3831c..202d08d70ef2 100644 --- a/test/e2e/adaptive-playwright-robots-file/actor/main.js +++ b/test/e2e/adaptive-playwright-robots-file/actor/main.js @@ -53,7 +53,8 @@ await Actor.init({ const crawler = new AdaptivePlaywrightCrawler({ maxRequestsPerCrawl: 10, respectRobotsTxtFile: true, - onSkippedRequest: (args) => crawler.log.warningOnce(`Request ${args.url} was skipped, reason: ${args.reason}`), + onSkippedRequest: (args) => + crawler.log.warningOnce(`Request ${args.request.url} was skipped, reason: ${args.reason}`), }); crawler.router.addDefaultHandler(async ({ log, request, enqueueLinks, pushData }) => { From bd4771ece1cc0149d739f25aba266764b73ebf2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ad=C3=A1mek?= Date: Fri, 28 Aug 2026 09:23:57 +0200 Subject: [PATCH 2/2] refactor: keep same-domain filtering after an off-domain redirect (#4076) Restores the #3923 fix that the v4 rebase silently reverted. The enqueueLinks split (#4010) predates #3923 and its diff carried a pre-fix copy of `resolveBaseUrlForEnqueueLinksFiltering`; when the rebase replayed it over the fixed base, the rerere cache (seeded from the old merge branch, which had resolved this exact conflict toward v4) applied that resolution again, so the fix disappeared without ever showing a conflict. The result: with `strategy: 'same-domain'` and a cross-domain redirect, the `undefined` baseUrl built no strategy patterns and every link on the redirected page got enqueued. The regression test runs the real CheerioCrawler path with an http client that reports an off-domain load. An audit of all v3 fixes since 3.14 against master found no other fix dropped by the rebase. Closes #4070 --- .../core/src/enqueue_links/enqueue_links.ts | 5 +- test/core/enqueue_links/enqueue_links.test.ts | 48 ++++++++++++++++++- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/packages/core/src/enqueue_links/enqueue_links.ts b/packages/core/src/enqueue_links/enqueue_links.ts index e7420569699f..dc74a4ffad4e 100644 --- a/packages/core/src/enqueue_links/enqueue_links.ts +++ b/packages/core/src/enqueue_links/enqueue_links.ts @@ -179,8 +179,7 @@ export function resolveBaseUrlForEnqueueLinksFiltering({ } // If the user wants to ensure the same domain is accessed, regardless of subdomains, we check to ensure the domains match - // Returning undefined here is intentional! If the domains don't match, having no baseUrl in enqueueLinks will cause it to not enqueue anything - // which is the intended behavior (since we went off domain) + // If they don't (we went off domain via a redirect), we keep filtering against the original domain - returning undefined would disable the filtering entirely if (enqueueStrategy === EnqueueStrategy.SameDomain) { const originalHostname = getDomain(originalUrlOrigin, { mixedInputs: false })!; const finalHostname = getDomain(finalUrlOrigin, { mixedInputs: false })!; @@ -189,7 +188,7 @@ export function resolveBaseUrlForEnqueueLinksFiltering({ return finalUrlOrigin; } - return undefined; + return originalUrlOrigin; } // Always enqueue urls that are from the same origin in all other cases, as the filtering happens on the original request url, even if there was a redirect diff --git a/test/core/enqueue_links/enqueue_links.test.ts b/test/core/enqueue_links/enqueue_links.test.ts index 06772447da46..19be0b3c59fe 100644 --- a/test/core/enqueue_links/enqueue_links.test.ts +++ b/test/core/enqueue_links/enqueue_links.test.ts @@ -72,7 +72,7 @@ async function createRequestQueueMock(seedUrl = 'https://example.com') { // `enqueueLinks()`'s hostname/domain-based strategies see whatever start URL a test navigates to, // regardless of where the content is actually served from. class FixtureHttpClient extends BaseHttpClient { - constructor(private readonly html: string) { + constructor(protected readonly html: string) { super(); } @@ -415,6 +415,52 @@ describe('enqueueLinks()', () => { ]); }); + test('keeps filtering by the original domain with the strategy of same-domain after an off-domain redirect', async () => { + const { enqueued, requestQueue } = await createRequestQueueMock(); + // Serves the fixture HTML while reporting the load as if it had redirected off-domain. + const redirectingHttpClient = new (class extends FixtureHttpClient { + override async sendRequest(): Promise { + return new ResponseWithUrl(this.html, { + url: 'https://another.com/', + status: 200, + headers: { 'content-type': 'text/html; charset=utf-8' }, + }); + } + })(HTML); + + const crawler = new CheerioCrawler({ + requestManager: requestQueue, + httpClient: redirectingHttpClient, + requestHandler: async ({ request, enqueueLinks }) => { + if (request.url !== 'https://example.com') return; + await enqueueLinks({ strategy: EnqueueStrategy.SameDomain }); + }, + }); + await crawler.run(['https://example.com']); + + expect(enqueued.map((r) => r.url)).toEqual([ + 'https://example.com/a/b/first', + 'https://example.com/a/second', + 'https://example.com/a/b/third', + ]); + }); + + test('ignores an explicitly undefined baseUrl and keeps the resolved one', async () => { + const { enqueued, requestQueue } = await createRequestQueueMock(); + await runCheerioEnqueueLinks( + { strategy: EnqueueStrategy.SameDomain, baseUrl: undefined }, + { requestManager: requestQueue }, + ); + + expect(enqueued.map((r) => r.url)).toEqual([ + 'https://example.com/a/b/first', + 'https://example.com/a/second', + 'https://example.com/a/b/third', + 'https://example.com/x/absolutepath', + 'https://example.com/y/relativepath', + ]); + }); + test('correctly resolves relative URLs with the strategy of all', async () => { const { enqueued, requestQueue } = await createRequestQueueMock(); await runCheerioEnqueueLinks(