[pull] master from apify:master

docs/yarn.lock

@@ -36,12 +36,12 @@ __metadata:
   linkType: hard
 
 "brace-expansion@npm:^1.1.7":
-  version: 1.1.16
-  resolution: "brace-expansion@npm:1.1.16"
+  version: 1.1.18
+  resolution: "brace-expansion@npm:1.1.18"
   dependencies:
     balanced-match: "npm:^1.0.0"
     concat-map: "npm:0.0.1"
-  checksum: 10c0/b2a915bbedbf4e45840d1fb9a4d391bbf26a79475bd134714d3cee34f1f0edb0ce982738028843be5fbaf8039429f71fa487df8c915b6065ced542c83e58fae6
+  checksum: 10c0/3432c18a9e2ebf94162d4effb62198bd0adea06a9f332b2c0188df5d5e30b1e51ea3c848b6608e47d0b857ebe1ea5b3888ed3326dd3c4f6f9645c94153cf9c14
   languageName: node
   linkType: hard
 

packages/basic-crawler/src/internals/basic-crawler.ts

@@ -383,6 +383,9 @@ export interface BasicCrawlerOptions<Context extends CrawlingContext = BasicCraw
      * 2. because they don't match enqueueLinks filters,
      * 3. because they are redirected to a URL that doesn't match the enqueueLinks strategy,
      * 4. or because the {@apilink BasicCrawlerOptions.maxRequestsPerCrawl|`maxRequestsPerCrawl`} limit has been reached
+     *
+     * When `enqueueLinks` is called with its own `onSkippedRequest` callback, both are invoked — this one first,
+     * then the `enqueueLinks` one.
      */
     onSkippedRequest?: SkippedRequestCallback;
 
@@ -1746,8 +1749,9 @@ export class BasicCrawler<Context extends CrawlingContext = BasicCrawlingContext
     /**
      * Wrapper around the crawling context's `enqueueLinks` method:
      * - Injects `crawlDepth` to each request being added based on the crawling context request.
-     * - Provides defaults for the `enqueueLinks` options based on the crawler configuration.
-     *      - These options can be overridden by the user.
+     * - Combines the `enqueueLinks` options with the crawler configuration - the user options take precedence,
+     *   but the crawler limits are always enforced (the `limit` is capped by the remaining `maxRequestsPerCrawl`
+     *   budget and skipped requests are always reported to the crawler too).
      * @internal
      */
     protected async enqueueLinksWithCrawlDepth(
@@ -1768,36 +1772,41 @@ export class BasicCrawler<Context extends CrawlingContext = BasicCrawlingContext
             return options.transformRequestFunction ? options.transformRequestFunction(newRequest) : newRequest;
         };
 
+        const limit = this.calculateEnqueuedRequestLimit(options.limit);
+
         // Create a request-scoped callback that logs enqueueLimit once per request handler call
         // Only log if an explicit limit was passed to enqueueLinks (not the internal maxRequestsPerCrawl-derived limit)
         let loggedEnqueueLimitForThisRequest = false;
         const onSkippedRequest: SkippedRequestCallback = async (skippedOptions) => {
             if (skippedOptions.reason === 'enqueueLimit') {
                 if (!loggedEnqueueLimitForThisRequest && options.limit !== undefined) {
                     this.log.info(
-                        `Skipping URLs in the handler for ${request.url} due to the enqueueLinks limit of ${options.limit}.`,
+                        limit === options.limit
+                            ? `Skipping URLs in the handler for ${request.url} due to the enqueueLinks limit of ${options.limit}.`
+                            : `Skipping URLs in the handler for ${request.url} due to the remaining maxRequestsPerCrawl budget of ${limit}, which is lower than the enqueueLinks limit of ${options.limit}.`,
                     );
                     loggedEnqueueLimitForThisRequest = true;
                 }
             }
 
             await this.handleSkippedRequest(skippedOptions);
+            await options.onSkippedRequest?.(skippedOptions);
         };
 
         // `enqueueLinks` applies `options.label`/`options.userData` to every newly enqueued request, so a single
         // validation against the label's schema covers them all (a no-op unless the router declares a schema).
         await this.validateRequestUserData({ label: options.label, userData: options.userData });
 
         return enqueueLinks({
-            requestQueue,
-            robotsTxtFile: await this.getRobotsTxtFileForUrl(request!.url),
-            respectRobotsTxtFile: this.respectRobotsTxtFile,
-            onSkippedRequest,
-            limit: this.calculateEnqueuedRequestLimit(options.limit),
-
-            // Allow user options to override defaults set above ⤴
             ...options,
 
+            // The options below are merged with the user options, so an explicitly `undefined` value
+            // (e.g. `enqueueLinks({ urls, limit: config.limit })`) cannot discard the crawler defaults ⤵
+            requestQueue: options.requestQueue ?? requestQueue,
+            robotsTxtFile: options.robotsTxtFile ?? (await this.getRobotsTxtFileForUrl(request.url)),
+            respectRobotsTxtFile: options.respectRobotsTxtFile ?? this.respectRobotsTxtFile,
+            onSkippedRequest,
+            limit,
             transformRequestFunction: transformRequestFunctionWrapper,
         });
     }

packages/browser-crawler/src/internals/browser-crawler.ts

@@ -642,7 +642,9 @@ export abstract class BrowserCrawler<
         const contextEnqueueLinks = crawlingContext.enqueueLinks;
         crawlingContext.enqueueLinks = async (enqueueOptions) => {
             return browserCrawlerEnqueueLinks({
-                options: { ...enqueueOptions, limit: this.calculateEnqueuedRequestLimit(enqueueOptions?.limit) },
+                // `contextEnqueueLinks` clamps `limit` by the remaining `maxRequestsPerCrawl` budget itself;
+                // pre-clamping it here would make the crawler log the internal limit as a user-provided one
+                options: enqueueOptions,
                 page,
                 requestQueue: await this.getRequestQueue(),
                 robotsTxtFile: await this.getRobotsTxtFileForUrl(crawlingContext.request.url),

packages/cheerio-crawler/src/internals/cheerio-crawler.ts

@@ -197,7 +197,9 @@ export class CheerioCrawler extends HttpCrawler<CheerioCrawlingContext> {
             body,
             enqueueLinks: async (enqueueOptions?: EnqueueLinksOptions) => {
                 return cheerioCrawlerEnqueueLinks({
-                    options: { ...enqueueOptions, limit: this.calculateEnqueuedRequestLimit(enqueueOptions?.limit) },
+                    // `originalEnqueueLinks` clamps `limit` by the remaining `maxRequestsPerCrawl` budget itself;
+                    // pre-clamping it here would make the crawler log the internal limit as a user-provided one
+                    options: enqueueOptions,
                     $,
                     requestQueue: await this.getRequestQueue(),
                     robotsTxtFile: await this.getRobotsTxtFileForUrl(crawlingContext.request.url),

packages/core/src/enqueue_links/enqueue_links.ts

@@ -34,7 +34,11 @@ import {
 export { EnqueueStrategy };
 
 export interface EnqueueLinksOptions extends RequestQueueOperationOptions {
-    /** Limit the amount of actually enqueued URLs to this number. Useful for testing across the entire crawling scope. */
+    /**
+     * Limit the amount of actually enqueued URLs to this number. Useful for testing across the entire crawling scope.
+     * When called from a crawler context, the limit is further capped by what's left of the crawler's
+     * {@apilink BasicCrawlerOptions.maxRequestsPerCrawl|`maxRequestsPerCrawl`} budget.
+     */
     limit?: number;
 
     /** An array of URLs to enqueue. */
@@ -197,6 +201,9 @@ export interface EnqueueLinksOptions extends RequestQueueOperationOptions {
      * 1. based on robots.txt file,
      * 2. because they don't match enqueueLinks filters,
      * 3. or because the maxRequestsPerCrawl limit has been reached
+     *
+     * When calling `enqueueLinks` through a crawler context, this callback runs in addition to (after) the
+     * crawler-level `onSkippedRequest`, it does not replace it.
      */
     onSkippedRequest?: SkippedRequestCallback;
 }

test/core/crawlers/basic_crawler.test.ts

@@ -2127,6 +2127,215 @@ describe('BasicCrawler', () => {
             expect(visitedUrls).toContain('http://example.com/');
             expect(visitedUrls).toContain('http://example.com/new');
         });
+
+        test('enqueueLinks should respect maxRequestsPerCrawl when passed an explicitly undefined limit', async () => {
+            const requestQueue = await RequestQueue.open();
+            const onSkippedRequest = vitest.fn();
+
+            const requestsToAdd = Array.from({ length: 6 }, (_, i) => `http://example.com/${i + 1}`);
+
+            const crawler = new BasicCrawler({
+                requestQueue,
+                maxRequestsPerCrawl: 5,
+                onSkippedRequest,
+                requestHandler: async (context) => {
+                    if (context.request.label) {
+                        return;
+                    }
+
+                    crawler.stats.state.requestsFinished = 2;
+
+                    // e.g. `enqueueLinks({ urls, limit: config.limit })` where `config.limit` is not set
+                    await context.enqueueLinks({ urls: requestsToAdd, limit: undefined, label: 'child' });
+                },
+            });
+
+            await crawler.run(['http://example.com']);
+
+            // 2 requests already finished and 1 is in progress, so only 2 more fit into the limit
+            expect(requestQueue.getTotalCount()).toBe(3);
+
+            const skippedUrls = onSkippedRequest.mock.calls
+                .map((call) => call[0])
+                .filter(({ reason }) => reason === 'enqueueLimit')
+                .map(({ url }) => url)
+                .sort();
+
+            expect(skippedUrls).toEqual([
+                'http://example.com/3',
+                'http://example.com/4',
+                'http://example.com/5',
+                'http://example.com/6',
+            ]);
+        });
+
+        test('enqueueLinks should clamp an explicit limit to the remaining maxRequestsPerCrawl budget', async () => {
+            const requestQueue = await RequestQueue.open();
+
+            const requestsToAdd = Array.from({ length: 6 }, (_, i) => `http://example.com/${i + 1}`);
+
+            const crawler = new BasicCrawler({
+                requestQueue,
+                maxRequestsPerCrawl: 5,
+                requestHandler: async (context) => {
+                    if (context.request.label) {
+                        return;
+                    }
+
+                    crawler.stats.state.requestsFinished = 2;
+
+                    await context.enqueueLinks({ urls: requestsToAdd, limit: 4, label: 'child' });
+                },
+            });
+
+            const infoSpy = vitest.spyOn(crawler.log, 'info');
+
+            await crawler.run(['http://example.com']);
+
+            // The user limit of 4 is higher than what's left of maxRequestsPerCrawl, so only 2 are enqueued
+            expect(requestQueue.getTotalCount()).toBe(3);
+
+            // ...and the log message must not blame the user limit of 4 for it
+            expect(infoSpy).toHaveBeenCalledWith(
+                expect.stringContaining('due to the remaining maxRequestsPerCrawl budget of 2'),
+            );
+        });
+
+        test('enqueueLinks should keep reporting skipped requests when the user passes onSkippedRequest', async () => {
+            const requestQueue = await RequestQueue.open();
+            const crawlerOnSkippedRequest = vitest.fn();
+            const userOnSkippedRequest = vitest.fn();
+
+            const requestsToAdd = Array.from({ length: 3 }, (_, i) => `http://example.com/${i + 1}`);
+
+            const crawler = new BasicCrawler({
+                requestQueue,
+                onSkippedRequest: crawlerOnSkippedRequest,
+                requestHandler: async (context) => {
+                    if (context.request.label) {
+                        return;
+                    }
+
+                    await context.enqueueLinks({
+                        urls: requestsToAdd,
+                        limit: 1,
+                        label: 'child',
+                        onSkippedRequest: userOnSkippedRequest,
+                    });
+                },
+            });
+
+            await crawler.run(['http://example.com']);
+
+            const skipped = [
+                { url: 'http://example.com/2', reason: 'enqueueLimit' },
+                { url: 'http://example.com/3', reason: 'enqueueLimit' },
+            ];
+
+            for (const mock of [crawlerOnSkippedRequest, userOnSkippedRequest]) {
+                expect(mock.mock.calls.map((call) => call[0]).sort((a, b) => a.url.localeCompare(b.url))).toEqual(
+                    skipped,
+                );
+            }
+        });
+
+        test('enqueueLinks should keep the crawler robots.txt file when passed an explicitly undefined robotsTxtFile', async () => {
+            const requestQueue = await RequestQueue.open();
+
+            const crawler = new (class MockedRobotsTxtCrawler extends BasicCrawler {
+                override async getRobotsTxtFileForUrl(_: string) {
+                    return RobotsTxtFile.from(
+                        'http://example.com/robots.txt',
+                        `User-agent: *
+                         Disallow: /no
+                        `,
+                    );
+                }
+            })({
+                requestQueue,
+                maxConcurrency: 1,
+                respectRobotsTxtFile: true,
+                requestHandler: async (context) => {
+                    if (context.request.label) {
+                        return;
+                    }
+
+                    await context.enqueueLinks({
+                        urls: ['http://example.com/yes', 'http://example.com/no'],
+                        robotsTxtFile: undefined,
+                        label: 'child',
+                    });
+                },
+            });
+
+            await crawler.run(['http://example.com/start']);
+
+            // The disallowed URL should never make it into the queue
+            expect(requestQueue.getTotalCount()).toBe(2);
+        });
+
+        test('enqueueLinks should keep the crawler user-agent when passed an explicitly undefined respectRobotsTxtFile', async () => {
+            const requestQueue = await RequestQueue.open();
+            const isAllowedSpy = vitest.fn((_url: string, _userAgent?: string) => true);
+
+            const crawler = new (class MockedRobotsTxtCrawler extends BasicCrawler {
+                override async getRobotsTxtFileForUrl(_: string) {
+                    return { isAllowed: isAllowedSpy } as unknown as RobotsTxtFile;
+                }
+            })({
+                requestQueue,
+                maxConcurrency: 1,
+                respectRobotsTxtFile: { userAgent: 'MyCrawler' },
+                requestHandler: async (context) => {
+                    if (context.request.label) {
+                        return;
+                    }
+
+                    await context.enqueueLinks({
+                        urls: ['http://example.com/child'],
+                        respectRobotsTxtFile: undefined,
+                        label: 'child',
+                    });
+                },
+            });
+
+            await crawler.run(['http://example.com/start']);
+
+            expect(isAllowedSpy).toHaveBeenCalledWith('http://example.com/child', 'MyCrawler');
+            // the crawler user-agent must not fall back to the `*` default
+            expect(isAllowedSpy.mock.calls.map(([, userAgent]) => userAgent)).not.toContain('*');
+        });
+
+        test('enqueueLinks should use the request queue from the options, and the crawler one when it is undefined', async () => {
+            const requestQueue = await RequestQueue.open();
+            const customQueue = await RequestQueue.open('custom-queue');
+
+            const crawler = new BasicCrawler({
+                requestQueue,
+                requestHandler: async (context) => {
+                    if (context.request.label) {
+                        return;
+                    }
+
+                    await context.enqueueLinks({
+                        urls: ['http://example.com/custom'],
+                        requestQueue: customQueue,
+                        label: 'child',
+                    });
+
+                    await context.enqueueLinks({
+                        urls: ['http://example.com/default'],
+                        requestQueue: undefined,
+                        label: 'child',
+                    });
+                },
+            });
+
+            await crawler.run(['http://example.com/start']);
+
+            expect(customQueue.getTotalCount()).toBe(1);
+            expect(requestQueue.getTotalCount()).toBe(2);
+        });
     });
 
     describe('addRequests input validation', () => {

test/core/crawlers/cheerio_crawler.test.ts

@@ -1411,6 +1411,22 @@ describe('CheerioCrawler', () => {
         expect(succeeded[0]).toEqual('Redirecting outside');
     });
 
+    test('enqueueLinks should not log an enqueueLinks limit when only maxRequestsPerCrawl clamps', async () => {
+        const crawler = new CheerioCrawler({
+            maxRequestsPerCrawl: 1,
+            requestHandler: async ({ enqueueLinks }) => {
+                await enqueueLinks({ strategy: EnqueueStrategy.All });
+            },
+        });
+
+        const infoSpy = vitest.spyOn(crawler.log, 'info');
+
+        await crawler.run([`${serverAddress}/special/html-type`]);
+
+        // The user passed no `limit`, so the skips must not be attributed to one
+        expect(infoSpy).not.toHaveBeenCalledWith(expect.stringContaining('Skipping URLs in the handler'));
+    });
+
     test('enqueueLinks should respect maxCrawlDepth', async () => {
         const succeeded: string[] = [];