fix: reduced unnecessary GET /sketches request

Ref: #1849
Signed-off-by: Akos Kitta <[email protected]>

Author: Akos Kitta

arduino-ide-extension/src/browser/create/create-api.ts

@@ -62,26 +62,25 @@ export class CreateApi {
     url.searchParams.set('user_id', 'me');
     url.searchParams.set('limit', limit.toString());
     const headers = await this.headers();
-    const result: { sketches: Create.Sketch[] } = { sketches: [] };
-
-    let partialSketches: Create.Sketch[] = [];
+    const allSketches: Create.Sketch[] = [];
     let currentOffset = 0;
-    do {
+    while (true) {
       url.searchParams.set('offset', currentOffset.toString());
-      partialSketches = (
-        await this.run<{ sketches: Create.Sketch[] }>(url, {
-          method: 'GET',
-          headers,
-        })
-      ).sketches;
-      if (partialSketches.length !== 0) {
-        result.sketches = result.sketches.concat(partialSketches);
+      const { sketches } = await this.run<{ sketches: Create.Sketch[] }>(url, {
+        method: 'GET',
+        headers,
+      });
+      allSketches.push(...sketches);
+      if (sketches.length < limit) {
+        break;
       }
-      currentOffset = currentOffset + limit;
-    } while (partialSketches.length !== 0);
-
-    result.sketches.forEach((sketch) => this.sketchCache.addSketch(sketch));
-    return result.sketches;
+      currentOffset += limit;
+      // The create API doc show that there is `next` and `prev` pages, but it does not work
+      // https://api2.arduino.cc/create/docs#!/sketches95v2/sketches_v2_search
+      // IF sketchCount mod limit === 0, an extra fetch must happen to detect the end of the pagination.
+    }
+    allSketches.forEach((sketch) => this.sketchCache.addSketch(sketch));
+    return allSketches;
   }
 
   async createSketch(

arduino-ide-extension/src/test/browser/create-api.test.ts

@@ -1,4 +1,8 @@
-import { Container, ContainerModule } from '@theia/core/shared/inversify';
+import {
+  Container,
+  ContainerModule,
+  injectable,
+} from '@theia/core/shared/inversify';
 import { assert, expect } from 'chai';
 import fetch from 'cross-fetch';
 import { v4 } from 'uuid';
@@ -18,13 +22,14 @@ import queryString = require('query-string');
 const timeout = 60 * 1_000;
 
 describe('create-api', () => {
-  let createApi: CreateApi;
+  let createApi: TestCreateApi;
 
   before(async function () {
     this.timeout(timeout);
     try {
       const accessToken = await login();
-      createApi = createContainer(accessToken).get<CreateApi>(CreateApi);
+      createApi =
+        createContainer(accessToken).get<TestCreateApi>(TestCreateApi);
     } catch (err) {
       if (err instanceof LoginFailed) {
         return this.skip();
@@ -42,7 +47,7 @@ describe('create-api', () => {
     const container = new Container({ defaultScope: 'Singleton' });
     container.load(
       new ContainerModule((bind) => {
-        bind(CreateApi).toSelf().inSingletonScope();
+        bind(TestCreateApi).toSelf().inSingletonScope();
         bind(SketchCache).toSelf().inSingletonScope();
         bind(AuthenticationClientService).toConstantValue(<
           AuthenticationClientService
@@ -223,6 +228,90 @@ describe('create-api', () => {
     expect(findByName(otherName, sketches)).to.be.not.undefined;
   });
 
+  it('should not run unnecessary fetches when retrieving all sketches', async () => {
+    const content = 'void setup(){} void loop(){}';
+    const maxLimit = 50; // https://github.com/arduino/arduino-ide/pull/875
+    const sketchNames = [...Array(maxLimit - 1).keys()].map(() => v4());
+
+    // #region - sketch count < offset
+    await sketchNames
+      .map((name) => createApi.createSketch(toPosix(name), content))
+      .reduce(async (acc, curr) => {
+        await acc;
+        return curr;
+      }, Promise.resolve() as Promise<unknown>);
+
+    createApi.resetRequestRecording();
+    let sketches = await createApi.sketches();
+    let actualRecording = createApi.requestRecording.slice();
+
+    expect(sketches.length).to.be.equal(maxLimit - 1);
+    sketchNames.forEach(
+      (name) => expect(findByName(name, sketches)).to.be.not.undefined
+    );
+
+    expect(actualRecording.length).to.be.equal(1);
+    let getSketchesRequests = actualRecording.filter(
+      (description) =>
+        description.method === 'GET' &&
+        description.pathname === '/create/v2/sketches' &&
+        description.query &&
+        description.query.includes(`limit=${maxLimit}`)
+    );
+    expect(getSketchesRequests.length).to.be.equal(1);
+    // #endregion
+
+    // #region - sketch count === offset
+    let extraName = v4();
+    sketchNames.push(extraName);
+    await createApi.createSketch(toPosix(extraName), content);
+
+    createApi.resetRequestRecording();
+    sketches = await createApi.sketches();
+    actualRecording = createApi.requestRecording.slice();
+
+    expect(sketches.length).to.be.equal(maxLimit);
+    sketchNames.forEach(
+      (name) => expect(findByName(name, sketches)).to.be.not.undefined
+    );
+
+    expect(actualRecording.length).to.be.equal(2);
+    getSketchesRequests = actualRecording.filter(
+      (description) =>
+        description.method === 'GET' &&
+        description.pathname === '/create/v2/sketches' &&
+        description.query &&
+        description.query.includes(`limit=${maxLimit}`)
+    );
+    expect(getSketchesRequests.length).to.be.equal(2);
+    // #endregion
+
+    // #region - sketch count > offset
+    extraName = v4();
+    sketchNames.push(extraName);
+    await createApi.createSketch(toPosix(extraName), content);
+
+    createApi.resetRequestRecording();
+    sketches = await createApi.sketches();
+    actualRecording = createApi.requestRecording.slice();
+
+    expect(sketches.length).to.be.equal(maxLimit + 1);
+    sketchNames.forEach(
+      (name) => expect(findByName(name, sketches)).to.be.not.undefined
+    );
+
+    expect(actualRecording.length).to.be.equal(2);
+    getSketchesRequests = actualRecording.filter(
+      (description) =>
+        description.method === 'GET' &&
+        description.pathname === '/create/v2/sketches' &&
+        description.query &&
+        description.query.includes(`limit=${maxLimit}`)
+    );
+    expect(getSketchesRequests.length).to.be.equal(2);
+    // #endregion
+  });
+
   ['.', '-', '_'].map((char) => {
     it(`should create a new sketch with '${char}' in the sketch folder name although it's disallowed from the Create Editor`, async () => {
       const name = `sketch${char}`;
@@ -298,3 +387,44 @@ class LoginFailed extends Error {
     Object.setPrototypeOf(this, LoginFailed.prototype);
   }
 }
+
+@injectable()
+class TestCreateApi extends CreateApi {
+  private _recording: RequestDescription[] = [];
+
+  constructor() {
+    super();
+    const originalRun = this['run'];
+    this['run'] = (url, init, resultProvider) => {
+      this._recording.push(createRequestDescription(url, init));
+      return originalRun.bind(this)(url, init, resultProvider);
+    };
+  }
+
+  resetRequestRecording(): void {
+    this._recording = [];
+  }
+
+  get requestRecording(): RequestDescription[] {
+    return this._recording;
+  }
+}
+
+interface RequestDescription {
+  readonly origin: string;
+  readonly pathname: string;
+  readonly query?: string;
+
+  readonly method?: string | undefined;
+  readonly serializedBody?: string | undefined;
+}
+
+function createRequestDescription(
+  url: URL,
+  init?: RequestInit | undefined
+): RequestDescription {
+  const { origin, pathname, search: query } = url;
+  const method = init?.method;
+  const serializedBody = init?.body?.toString();
+  return { origin, pathname, query, method, serializedBody };
+}