From fc14f7c8412d3008add20b2cd23a9d7e8faefbb2 Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Wed, 12 Apr 2023 19:16:23 +0200 Subject: [PATCH] fix: reduced unnecessary `GET /sketches` request Ref: #1849 Signed-off-by: Akos Kitta --- .../src/browser/create/create-api.ts | 33 ++++--- .../src/test/browser/create-api.test.ts | 95 ++++++++++++++++++- 2 files changed, 107 insertions(+), 21 deletions(-) diff --git a/arduino-ide-extension/src/browser/create/create-api.ts b/arduino-ide-extension/src/browser/create/create-api.ts index b2aac2b02..beebf0a1e 100644 --- a/arduino-ide-extension/src/browser/create/create-api.ts +++ b/arduino-ide-extension/src/browser/create/create-api.ts @@ -86,26 +86,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( diff --git a/arduino-ide-extension/src/test/browser/create-api.test.ts b/arduino-ide-extension/src/test/browser/create-api.test.ts index a18609cb1..13bd8686e 100644 --- a/arduino-ide-extension/src/test/browser/create-api.test.ts +++ b/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 { posix } from 'path'; @@ -19,13 +23,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 = + createContainer(accessToken).get(TestCreateApi); } catch (err) { if (err instanceof LoginFailed) { return this.skip(); @@ -43,7 +48,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 @@ -224,6 +229,47 @@ describe('create-api', () => { expect(findByName(otherName, sketches)).to.be.not.undefined; }); + [ + [-1, 1], + [0, 2], + [1, 2], + ].forEach(([diff, expected]) => + it(`should not run unnecessary fetches when retrieving all sketches (sketch count ${ + diff < 0 ? '<' : diff > 0 ? '>' : '=' + } limit)`, async () => { + const content = 'void setup(){} void loop(){}'; + const maxLimit = 50; // https://github.com/arduino/arduino-ide/pull/875 + const sketchCount = maxLimit + diff; + const sketchNames = [...Array(sketchCount).keys()].map(() => v4()); + + await sketchNames + .map((name) => createApi.createSketch(toPosix(name), content)) + .reduce(async (acc, curr) => { + await acc; + return curr; + }, Promise.resolve() as Promise); + + createApi.resetRequestRecording(); + const sketches = await createApi.sketches(); + const allRequests = createApi.requestRecording.slice(); + + expect(sketches.length).to.be.equal(sketchCount); + sketchNames.forEach( + (name) => expect(findByName(name, sketches)).to.be.not.undefined + ); + + expect(allRequests.length).to.be.equal(expected); + const getSketchesRequests = allRequests.filter( + (description) => + description.method === 'GET' && + description.pathname === '/create/v2/sketches' && + description.query && + description.query.includes(`limit=${maxLimit}`) + ); + expect(getSketchesRequests.length).to.be.equal(expected); + }) + ); + ['.', '-', '_'].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}`; @@ -332,3 +378,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 }; +}