Repository: compiler-explorer/compiler-explorer
Pull request #7090 (2024-11-12 14:02:52)

Separate list and load actions for source API
This is the first PR in API consistency with more to come:

1. The source API urls have been properly defined with `/source/:source/list` and `/source/:source/load/:language/:filename`
   - These used to be two API endpoints mushed together into one `handle()` function. They are now separate
   - While it may appear from the tests that this is an API breakage, there's currently no way to have the source API spit out responses for stuff like `/source/moose/load/Grunkle`.
2. The frontend code calling the list api now has shared types
3. Code has been migrated to `/lib/handlers/api/source.ts`
    - I've moved code here, because I would like to separate the stuff that's under the API and the stuff that isn't (e.g., healthcheck)
    - The source endpoint is currently not under /api, but I believe it makes sense to move it.
4. GitHub is terrible at showing the diff, but the `browser.ts` file has been removed, since it's never actually used. Our frontend only calls `/source/builtin/...` and it never had any entries. Besides, the type used on the frontend for the locally stored stuff is different...

mattgodbolt: > The source endpoint is currently not under /api, but I believe it makes sense to move it.

Agreed here: we are paying for my terrible original design here. Anything to fix it is welcomed! We _may_ have folks relying on the old endpoints so we will need some redirect/equivalent thing though (at least for the interirm)
junlarsen: > > The source endpoint is currently not under /api, but I believe it makes sense to move it.
> 
> Agreed here: we are paying for my terrible original design here. Anything to fix it is welcomed! We _may_ have folks relying on the old endpoints so we will need some redirect/equivalent thing though (at least for the interirm)

Agreed, this patch doesn't change the paths itself, but if we do move them to /api we'll add a fallback

Patch:
diff --git a/app.ts b/app.ts
index 92e17a29d..d466b6a17 100755
--- a/app.ts
+++ b/app.ts
@@ -57,11 +57,11 @@ import {startWineInit} from './lib/exec.js';
 import {RemoteExecutionQuery} from './lib/execution/execution-query.js';
 import {initHostSpecialties} from './lib/execution/execution-triple.js';
 import {startExecutionWorkerThread} from './lib/execution/sqs-execution-queue.js';
+import {SourceController} from './lib/handlers/api/source-controller.js';
 import {CompileHandler} from './lib/handlers/compile.js';
 import * as healthCheck from './lib/handlers/health-check.js';
 import {NoScriptHandler} from './lib/handlers/noscript.js';
 import {RouteAPI, ShortLinkMetaData} from './lib/handlers/route-api.js';
-import {SourceHandler} from './lib/handlers/source.js';
 import {languages as allLanguages} from './lib/languages.js';
 import {logger, logToLoki, logToPapertrail, makeLogStream, suppressConsoleLog} from './lib/logger.js';
 import {setupMetricsServer} from './lib/metrics-server.js';
@@ -335,9 +335,13 @@ const httpRoot = urljoin(ceProps('httpRoot', '/'), '/');
 const staticUrl = ceProps<string | undefined>('staticUrl');
 const staticRoot = urljoin(staticUrl || urljoin(httpRoot, 'static'), '/');
 
-function staticHeaders(res: express.Response) {
+/**
+ * Sets appropriate headers for static resources. This is based on the staticMaxAgeSecs property from the
+ * compiler-explorer.properties file.
+ */
+export function addStaticHeaders(res: express.Response) {
     if (staticMaxAgeSecs) {
-        res.setHeader('Cache-Control', 'public, max-age=' + staticMaxAgeSecs + ', must-revalidate');
+        res.setHeader('Cache-Control', `public, max-age=${staticMaxAgeSecs}, must-revalidate`);
     }
 }
 
@@ -550,7 +554,7 @@ async function main() {
     const compileHandler = new CompileHandler(compilationEnvironment, awsProps);
     const storageType = getStorageTypeByKey(storageSolution);
     const storageHandler = new storageType(httpRoot, compilerProps, awsProps);
-    const sourceHandler = new SourceHandler(sources, staticHeaders);
+    const sourceHandler = new SourceController(sources);
     const compilerFinder = new CompilerFinder(compileHandler, compilerProps, awsProps, defArgs, clientOptionsHandler);
 
     logger.info('=======================================');
@@ -615,7 +619,7 @@ async function main() {
         defArgs,
         renderConfig,
         renderGoldenLayout,
-        staticHeaders,
+        staticHeaders: addStaticHeaders,
         contentPolicyHeader,
     };
 
@@ -736,7 +740,7 @@ async function main() {
         req: express.Request,
         res: express.Response,
     ) {
-        staticHeaders(res);
+        addStaticHeaders(res);
         contentPolicyHeader(res);
 
         const embedded = req.query.embedded === 'true' ? true : false;
@@ -757,7 +761,7 @@ async function main() {
     }
 
     const embeddedHandler = function (req: express.Request, res: express.Response) {
-        staticHeaders(res);
+        addStaticHeaders(res);
         contentPolicyHeader(res);
         res.render(
             'embed',
@@ -802,7 +806,7 @@ async function main() {
         )
         .use(compression())
         .get('/', (req, res) => {
-            staticHeaders(res);
+            addStaticHeaders(res);
             contentPolicyHeader(res);
             res.render(
                 'index',
@@ -819,7 +823,7 @@ async function main() {
         // legacy. not a 301 to prevent any redirect loops between old e links and embed.html
         .get('/embed.html', embeddedHandler)
         .get('/embed-ro', (req, res) => {
-            staticHeaders(res);
+            addStaticHeaders(res);
             contentPolicyHeader(res);
             res.render(
                 'embed',
@@ -834,22 +838,22 @@ async function main() {
             );
         })
         .get('/robots.txt', (req, res) => {
-            staticHeaders(res);
+            addStaticHeaders(res);
             res.end('User-agent: *\nSitemap: https://godbolt.org/sitemap.xml\nDisallow:');
         })
         .get('/sitemap.xml', (req, res) => {
-            staticHeaders(res);
+            addStaticHeaders(res);
             res.set('Content-Type', 'application/xml');
             res.render('sitemap');
         })
         .use(sFavicon(utils.resolvePathFromAppRoot('static/favicons', getFaviconFilename())))
         .get('/client-options.js', (req, res) => {
-            staticHeaders(res);
+            addStaticHeaders(res);
             res.set('Content-Type', 'application/javascript');
             res.end(`window.compilerExplorerOptions = ${clientOptionsHandler.getJSON()};`);
         })
         .use('/bits/:bits(\\w+).html', (req, res) => {
-            staticHeaders(res);
+            addStaticHeaders(res);
             contentPolicyHeader(res);
             res.render(
                 `bits/${sanitize(req.params.bits)}`,
@@ -863,7 +867,8 @@ async function main() {
             );
         })
         .use(bodyParser.json({limit: ceProps('bodyParserLimit', maxUploadSize)}))
-        .use('/source', sourceHandler.handle.bind(sourceHandler))
+        .use('/source/:source/list', sourceHandler.listEntries.bind(sourceHandler))
+        .use('/source/:source/load/:language/:filename', sourceHandler.loadEntry.bind(sourceHandler))
         .get('/g/:id', oldGoogleUrlHandler)
         // Deprecated old route for this -- TODO remove in late 2021
         .post('/shortener', routeApi.apiHandler.shortener.handle.bind(routeApi.apiHandler.shortener));
diff --git a/lib/handlers/source.ts b/lib/handlers/api/source-controller.ts
similarity index 54%
rename from lib/handlers/source.ts
rename to lib/handlers/api/source-controller.ts
index b9e064913..065689401 100644
--- a/lib/handlers/source.ts
+++ b/lib/handlers/api/source-controller.ts
@@ -1,4 +1,4 @@
-// Copyright (c) 2017, Compiler Explorer Authors
+// Copyright (c) 2024, Compiler Explorer Authors
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
@@ -25,46 +25,42 @@
 import express from 'express';
 import _ from 'underscore';
 
-import {Source} from '../sources/index.js';
+import {addStaticHeaders} from '../../../app.js';
+import {Source} from '../../../types/source.interfaces.js';
 
-// TODO(supergrecko): Maybe provide a more elegant way to do this instead of accessing keys?
-const ALLOWED_ACTIONS = new Set(['list', 'load']);
+export class SourceController {
+    public constructor(private readonly sources: Source[]) {}
 
-export class SourceHandler {
-    public constructor(
-        private fileSources: Source[],
-        private addStaticHeaders: (res: express.Response) => void,
-    ) {}
-
-    private getSourceForHandler(handler: string): Source | null {
-        const records = _.indexBy(this.fileSources, 'urlpart');
-        return records[handler] || null;
-    }
-
-    private getActionForSource(source: Source, action: string): ((...args: unknown[]) => Promise<unknown>) | null {
-        return ALLOWED_ACTIONS.has(action) ? (source as any)[action] : null;
-    }
-
-    public handle(req: express.Request, res: express.Response, next: express.NextFunction): void {
-        // Split URLs with the scheme /source/browser/list into the source and the action to perform
-        const [_, handler, action, ...rest] = req.url.split('/');
-        const source = this.getSourceForHandler(handler);
+    /**
+     * Handle request to `/source/<source>/list` endpoint
+     */
+    public async listEntries(req: express.Request, res: express.Response) {
+        const source = this.getSourceForHandler(req.params.source);
         if (source === null) {
-            next();
+            res.sendStatus(404);
             return;
         }
-        const callback = this.getActionForSource(source, action);
-        if (callback === null) {
-            next();
+        const entries = await source.list();
+        addStaticHeaders(res);
+        res.json(entries);
+    }
+
+    /**
+     * Handle request to `/source/<source>/load/<language>/<filename>` endpoint
+     */
+    public async loadEntry(req: express.Request, res: express.Response) {
+        const source = this.getSourceForHandler(req.params.source);
+        if (source === null) {
+            res.sendStatus(404);
             return;
         }
-        callback(...rest)
-            .then(response => {
-                this.addStaticHeaders(res);
-                res.send(response);
-            })
-            .catch(err => {
-                res.send({err: err});
-            });
+        const entry = await source.load(req.params.language, req.params.filename);
+        addStaticHeaders(res);
+        res.json(entry);
+    }
+
+    private getSourceForHandler(handler: string): Source | null {
+        const records = _.indexBy(this.sources, 'urlpart');
+        return records[handler] || null;
     }
 }
diff --git a/lib/options-handler.ts b/lib/options-handler.ts
index 3a9fcc7aa..d29b520e0 100755
--- a/lib/options-handler.ts
+++ b/lib/options-handler.ts
@@ -32,12 +32,12 @@ import _ from 'underscore';
 import {AppDefaultArguments} from '../app.js';
 import {CompilerInfo} from '../types/compiler.interfaces.js';
 import type {LanguageKey} from '../types/languages.interfaces.js';
+import type {Source} from '../types/source.interfaces.js';
 import type {ToolTypeKey} from '../types/tool.interfaces.js';
 
 import {logger} from './logger.js';
 import type {PropertyGetter, PropertyValue} from './properties.interfaces.js';
 import {CompilerProps} from './properties.js';
-import {Source} from './sources/index.js';
 import {BaseTool, getToolTypeByKey} from './tooling/index.js';
 import {asSafeVer, getHash, splitArguments, splitIntoArray} from './utils.js';
 
diff --git a/lib/sources/builtin.ts b/lib/sources/builtin.ts
index 73531b0c0..00d5b073c 100644
--- a/lib/sources/builtin.ts
+++ b/lib/sources/builtin.ts
@@ -26,10 +26,9 @@ import fs from 'fs';
 import fsp from 'fs/promises';
 import path from 'path';
 
+import type {Source, SourceApiEntry, SourceEntry} from '../../types/source.interfaces.js';
 import * as props from '../properties.js';
 
-import type {Source, SourceEntry} from './index.js';
-
 const EXAMPLES_PATH = props.get('builtin', 'sourcePath', './examples/') as string;
 const NAME_SUBSTUTION_PATTERN = new RegExp('_', 'g');
 const ALL_EXAMPLES: SourceEntry[] = fs.readdirSync(EXAMPLES_PATH).flatMap(folder => {
@@ -62,7 +61,7 @@ export const builtin: Source = {
             return {file: 'Could not read file'};
         }
     },
-    async list(): Promise<Omit<SourceEntry, 'path'>[]> {
+    async list(): Promise<SourceApiEntry[]> {
         return ALL_EXAMPLES.map(e => ({
             lang: e.lang,
             name: e.name,
diff --git a/lib/sources/index.ts b/lib/sources/index.ts
index 47325edc8..4bbbcd96d 100644
--- a/lib/sources/index.ts
+++ b/lib/sources/index.ts
@@ -22,25 +22,6 @@
 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.
 
-import {browser} from './browser.js';
 import {builtin} from './builtin.js';
 
-export interface SourceEntry {
-    /** The file name of the source file */
-    file: string;
-    /** The programming language the source file is written in */
-    lang: string;
-    /** The "nice" name of the source file, replacing _ with spaces */
-    name: string;
-    /** The path to the source file */
-    path: string;
-}
-
-export interface Source {
-    name: string;
-    urlpart: string;
-    list(): Promise<Omit<SourceEntry, 'path'>[]>;
-    load(language: string, filename: string): Promise<{file: string}>;
-}
-
-export const sources = [browser, builtin];
+export const sources = [builtin];
diff --git a/static/widgets/load-save.ts b/static/widgets/load-save.ts
index a80464b75..4ef87e336 100644
--- a/static/widgets/load-save.ts
+++ b/static/widgets/load-save.ts
@@ -30,6 +30,7 @@ import {Language} from '../../types/languages.interfaces.js';
 import {unwrap, unwrapString} from '../assert.js';
 import {escapeHTML} from '../../shared/common-utils.js';
 import {localStorage} from '../local.js';
+import {SourceApiEntry} from '../../types/source.interfaces';
 
 const history = require('../history');
 
@@ -71,8 +72,8 @@ export class LoadSave {
         localStorage.set('files', JSON.stringify(files));
     }
 
-    private async fetchBuiltins(): Promise<Record<string, any>[]> {
-        return new Promise<Record<string, any>[]>(resolve => {
+    private async fetchBuiltins(): Promise<SourceApiEntry[]> {
+        return new Promise(resolve => {
             $.getJSON(window.location.origin + this.base + 'source/builtin/list', resolve);
         });
     }
@@ -118,7 +119,7 @@ export class LoadSave {
 
     private async populateBuiltins() {
         const builtins = (await this.fetchBuiltins()).filter(entry => this.currentLanguage?.id === entry.lang);
-        return LoadSave.populate(
+        LoadSave.populate(
             unwrap(this.modal).find('.examples'),
             builtins.map(elem => {
                 return {
diff --git a/lib/sources/browser.ts b/types/source.interfaces.ts
similarity index 71%
rename from lib/sources/browser.ts
rename to types/source.interfaces.ts
index 4ad7933eb..b3b3b382b 100644
--- a/lib/sources/browser.ts
+++ b/types/source.interfaces.ts
@@ -1,4 +1,4 @@
-// Copyright (c) 2012, Compiler Explorer Authors
+// Copyright (c) 2024, Compiler Explorer Authors
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
@@ -22,19 +22,22 @@
 // ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 // POSSIBILITY OF SUCH DAMAGE.
 
-import type {Source, SourceEntry} from './index.js';
+export interface SourceEntry {
+    /** The file name of the source file */
+    file: string;
+    /** The programming language the source file is written in */
+    lang: string;
+    /** The "nice" name of the source file, replacing _ with spaces */
+    name: string;
+    /** The path to the source file */
+    path: string;
+}
 
-// This is a fake plugin. All of the functionality is in the browser code.
-export const browser: Source = {
-    name: 'Browser',
-    urlpart: 'browser',
-    list(): Promise<Omit<SourceEntry, 'path'>[]> {
-        return Promise.resolve([]);
-    },
-    load(language: string, filename: string): Promise<{file: string}> {
-        return Promise.resolve({file: ''});
-    },
-};
+export interface Source {
+    name: string;
+    urlpart: string;
+    list(): Promise<SourceApiEntry[]>;
+    load(language: string, filename: string): Promise<{file: string}>;
+}
 
-export const name = 'Browser';
-export const urlpart = 'browser';
+export type SourceApiEntry = Pick<SourceEntry, 'lang' | 'name' | 'file'>;


Test patch:
diff --git a/test/handlers/source-tests.ts b/test/handlers/source-tests.ts
index ef5106730..a8708a1a7 100644
--- a/test/handlers/source-tests.ts
+++ b/test/handlers/source-tests.ts
@@ -26,35 +26,40 @@ import express from 'express';
 import request from 'supertest';
 import {describe, expect, it} from 'vitest';
 
-import {SourceHandler} from '../../lib/handlers/source.js';
+import {SourceController} from '../../lib/handlers/api/source-controller.js';
 
 describe('Sources', () => {
     const app = express();
-    const handler = new SourceHandler(
-        [
-            {
-                name: 'moose',
-                urlpart: 'moose',
-                list: async () => [{file: 'file', lang: 'lang', name: 'name'}],
-                load: name => Promise.resolve({file: `File called ${name}`}),
-            },
-        ],
-        res => res.setHeader('Yibble', 'boing'),
-    );
-    app.use('/source', handler.handle.bind(handler));
+    const handler = new SourceController([
+        {
+            name: 'moose',
+            urlpart: 'moose',
+            list: async () => [{file: 'file', lang: 'lang', name: 'Grunkle'}],
+            load: (lang, name) => Promise.resolve({file: `File called ${name} in ${lang}`}),
+        },
+    ]);
+    app.use('/source/:source/list', handler.listEntries.bind(handler));
+    app.use('/source/:source/load/:language/:filename', handler.loadEntry.bind(handler));
 
     it('should list', async () => {
-        const res = await request(app)
+        await request(app)
             .get('/source/moose/list')
             .expect('Content-Type', /json/)
-            .expect(200, [{file: 'file', lang: 'lang', name: 'name'}]);
-        expect(res.headers['yibble']).toEqual('boing');
+            .expect(200, [{file: 'file', lang: 'lang', name: 'Grunkle'}]);
     });
+
     it('should fetch files', async () => {
+        await request(app)
+            .get('/source/moose/load/lang/Grunkle')
+            .expect('Content-Type', /json/)
+            .expect(200, {file: 'File called Grunkle in lang'});
+    });
+
+    it('should have a max-age cache control header', async () => {
         const res = await request(app)
-            .get('/source/moose/load/Grunkle')
+            .get('/source/moose/load/lang/Grunkle')
             .expect('Content-Type', /json/)
-            .expect(200, {file: 'File called Grunkle'});
-        expect(res.headers['yibble']).toEqual('boing');
+            .expect(200, {file: 'File called Grunkle in lang'});
+        expect(res.headers['cache-control']).toContain('public, max-age=');
     });
 });