Migrate to Express [*****] #7877

paulmelnikow · 2022-04-22T16:42:34Z

There's surely a bit of cleanup to do here, but thought it worth getting some 👀 on this.

I'd welcome help updating and improving the documentation, though that could also be done in a follow-on PR.

An area I've not yet addressed is query parameters. Scoutcamp used to turn the params into numbers, which I imagine Express does not do. It means we probably can simplify places where we've got Joi.alternatives().try(Joi.string(), Joi.number()) in the query param schemas.

There are surely bugs in this, and edge cases. I removed a few tests in legacy-request-handler.spec.js which we may want to rewrite elsewhere.

We should test this out (maybe even on Fly), in particular things like suggest and token acceptance.

I would be surprised if performance were a problem, though we may want to experiment with this under load to make sure.

Closes #3328
Closes #3368
Closes #4948

shields-ci · 2022-04-22T16:43:48Z

	Warnings
⚠️	📚 Remember to ensure any changes to `config.private` in `services/github/auth/acceptor.spec.js` are reflected in the server secrets documentation
⚠️	📚 Remember to ensure any changes to `config.private` in `services/github/auth/acceptor.js` are reflected in the server secrets documentation
⚠️	📚 Remember to ensure any changes to `config.private` in `services/github/github-constellation.js` are reflected in the server secrets documentation

	Messages
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS against fc68b88

lgtm-com · 2022-04-22T16:46:56Z

This pull request introduces 1 alert when merging 78b886c into 560d267 - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

paulmelnikow · 2022-04-22T16:55:43Z

badge-maker/lib/make-badge.js

@@ -24,22 +23,6 @@ module.exports = function makeBadge({
  label = `${label}`.trim()
  message = `${message}`.trim()

-  // This ought to be the responsibility of the server, not `makeBadge`.
-  if (format === 'json') {


I've argued before that this functionality doesn't belong in the badge library as the particular JSON schema we generate here (with name and value) is a legacy quirk of shields.io, and not something we particularly want users to replace elsewhere. For the moment I went ahead and moved this functionality to the server. It's now handled by makeJsonBadge() which is invoked from a conditional in the request handler in base.js.

If we keep this we'll probably want to add normalizeColor to the package's public interface, or alternatively a normalizeBadgeData() function, and then we could start dogfooding the public makeBadge().

I haven't read over the code for this bit at all yet. I agree with the objective. Has anything significant has changed since #4980 in terms of the implementation, specifically #4980 (comment) ? I will have a look through it. If a slightly different configuration of not eating our own dogfood is necessary to get off scoutcamp, this shouldn't be a blocker but until we do #4947 and #4949 I suspect all we can really do is move from one undesirable implementation to another. I do accept #4947 and #4949 have been open for 2 years and none of us are actively working on them.
As a note, we may want to consider documenting it as a non-BC change for badge-maker, even though format='json' is undocumented. The next badge-maker will be a major release anyway due to dropping node 10.

paulmelnikow · 2022-04-22T16:56:36Z

badge-maker/lib/make-badge.spec.js

 describe('The badge generator', function () {
-  describe('color test', function () {


Mostly, but not completely, these duplicate tests in color.spec.js. We may want to go through them line by line and see if there are any here we should port over. With the format parameter removed, this behavior can no longer be tested here.

paulmelnikow · 2022-04-22T17:05:06Z

badge-maker/lib/make-badge.spec.js

    })

    it('should replace undefined svg badge style with "flat"', function () {
-      const jsonBadgeWithUnknownStyle = makeBadge({


paulmelnikow · 2022-04-22T17:05:15Z

badge-maker/lib/make-badge.spec.js

-  })
-
-  describe('JSON', function () {
-    it('should produce the expected JSON', function () {


Remove JSON tests

paulmelnikow · 2022-04-22T17:05:46Z

badge-maker/lib/make-badge.spec.js

          style: 'social',
        })
      )
-        .to.include('""')
+        .to.include('></text>')


SVG equivalent

paulmelnikow · 2022-04-22T17:21:35Z

core/server/server.spec.js

@@ -112,7 +110,9 @@ describe('The server', function () {
        `${baseUrl}:fruit-apple-green.json`
      )
      expect(statusCode).to.equal(200)
-      expect(headers['content-type']).to.equal('application/json')
+      expect(headers['content-type']).to.equal(
+        'application/json; charset=utf-8'


Charset added by Express

paulmelnikow · 2022-04-22T17:21:46Z

core/server/server.spec.js

      expect(body)
        .to.satisfy(isSvg)
        .and.to.include('410')
        .and.to.include('jpg no longer available')
    })
-
-    it('should return cors header for the request', async function () {


This duplicates a test a bit higher up in this context

paulmelnikow · 2022-04-22T17:23:07Z

doc/code-walkthrough.md

+    [`core/server/server.js`][core/server/server], is based on [Express][].
+    It creates an http server, sets up helpers for token persistence and
+    monitoring. Then it loads all the services, injecting dependencies as it
+    asks each one to register its route with the Express app.


Made a rough effort to update this, though it may make sense for someone to take a pass through later. This whole process is much more direct than before.

paulmelnikow · 2022-04-22T17:23:42Z

package.json

@@ -25,7 +25,6 @@
    "@fontsource/lekton": "^4.5.6",
    "@renovate/pep440": "^1.0.0",
    "@sentry/node": "^6.19.6",
-    "@shields_io/camp": "^18.1.1",


💁🏼 🎩

paulmelnikow · 2022-04-22T17:24:43Z

services/github/auth/acceptor.js

-      return end('GitHub OAuth authentication failed to provide a code.')
-    }
+  app.post('/github-auth/done', multer().none(), async (req, res) => {
+    const code = (req.body ?? {}).code


This hasn't been tested yet

paulmelnikow · 2022-04-22T17:42:46Z

In the first service test run there are some interesting failures.

One issue is that the JSON badge no longer strips leading and trailing whitespace from label and message. That could be added to makeJsonBadge… or maybe we should just fix the services.

paulmelnikow · 2022-04-22T22:20:43Z

core/base-service/route.spec.js

    })
  })

  context('No named params are declared', function () {
    const { regex, captureNames } = prepareRoute({
      base: 'foo',
-      format: '(?:[^/]+)',
+      format: '(?:[^/]+?)',


To correctly detect .svg and .json as intended, this regex needs to be lazy.

paulmelnikow · 2022-04-22T22:24:00Z

services/suggest.js

-function setRoutes(allowedOrigin, githubApiProvider, server) {
-  server.ajax.on('suggest/v1', (data, end, ask) => {
+function setRoutes(allowedOrigin, githubApiProvider, app) {
+  app.get('/[$]suggest/v1', (req, res) => {


The Scoutcamp AJAX routes implicitly started with $. For some reason the dollar needs to be escaped for Express.

paulmelnikow · 2022-04-22T22:25:44Z

services/suggest.js

    if (origin) {
      let host
      try {
        host = new URL(origin).hostname
      } catch (e) {
-        ask.res.setHeader('Access-Control-Allow-Origin', 'null')
-        end({ err: 'Disallowed' })
+        res.setHeader('Access-Control-Allow-Origin', 'null')


As I read this I wonder if it works, especially since we're setting Access-Control-Allow-Origin: * in server.js.

chris48s · 2022-04-24T17:58:56Z

Thanks for taking this on. This aspect of the codebase and the reliance on ScoutCamp is one of the biggest pain points of this codebase so it is really good to be making some progress on this :)

I haven't done a full review of this, but I have checked it out locally and skimmed the diff.

The main issue I hit is if I run npm start on a checkout of this branch, it throws

Failed to load plugin 'graphql' declared in 'BaseConfig': Cannot find module 'ws'

and won't start the frontend, so I've only been able to test the badge server. Any idea what is happening there?

There were a few bits I wanted to specifically check as possible edge cases:

Badges with regex routes e.g:
- services/travis/travis-build.service.js
- services/wercker/wercker.service.js
- services/requires/requires.service.js
- services/static-badge/static-badge.service.js
  Those all seem to work fine 👍
.png redirect to raster proxy. Also works 👍
/endpoint route conflict - not tested yet as I couldn't start the frontend.

I'll try and have a more detailed look at this next week.

chris48s · 2022-04-24T17:59:49Z

core/server/server.js

@@ -306,87 +297,78 @@ class Server {

  // See https://www.viget.com/articles/heroku-cloudflare-the-right-way/
  requireCloudflare() {
-    // Set `req.ip`, which is expected by `cloudflareMiddleware()`. This is set
-    // by Express but not Scoutcamp.


We'll definitely need to add back the special case for fly.io before we can merge/deploy this. It definitely doesn't work with the default setup.
On the heroku case, I'm slightly in 2 minds whether to just bin that on the basis we're not using it or keep it on the basis people who are self-hosting might be relying on it. In any case, we added this in #5712 because we deployed to Heroku using the default behaviour of cloudflareMiddleware() and it didn't work.

chris48s · 2022-04-24T18:00:33Z

core/server/server.js

+    app.use(
+      express.static(this.config.public.documentRoot, {
+        // Since express's `maxAge` parameter sets `Cache-Control: public`, set
+        // the headers manually insetad.
+        cacheControl: false,
+        setHeaders: res =>
+          res.setHeader('Cache-Control', 'max-age=300, s-maxage=300'),
+      })
+    )


Is this replacing staticMaxAge?
badges/sc@dc89a3a

chris48s · 2022-04-24T18:01:05Z

core/base-service/legacy-request-handler.js

-    const serverResponsive = setTimeout(() => {
-      serverUnresponsive = true
-      ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
-      const badgeData = coalesceBadge(
-        filteredQueryParams,
-        { label: 'vendor', message: 'unresponsive' },
-        {}
-      )
-      const svg = makeBadge(badgeData)
-      const extension = (match.slice(-1)[0] || '.svg').replace(/^\./, '')
-      setCacheHeadersOnResponse(ask.res)
-      makeSend(extension, ask.res, end)(svg)
-    }, 25000)


I guess we've just totally dropped this in favour of express this.server.setTimeout() ?

chris48s · 2022-04-24T18:02:34Z

badge-maker/lib/make-badge.js

@@ -24,22 +23,6 @@ module.exports = function makeBadge({
  label = `${label}`.trim()
  message = `${message}`.trim()

-  // This ought to be the responsibility of the server, not `makeBadge`.
-  if (format === 'json') {


I haven't read over the code for this bit at all yet. I agree with the objective. Has anything significant has changed since #4980 in terms of the implementation, specifically #4980 (comment) ? I will have a look through it. If a slightly different configuration of not eating our own dogfood is necessary to get off scoutcamp, this shouldn't be a blocker but until we do #4947 and #4949 I suspect all we can really do is move from one undesirable implementation to another. I do accept #4947 and #4949 have been open for 2 years and none of us are actively working on them.
As a note, we may want to consider documenting it as a non-BC change for badge-maker, even though format='json' is undocumented. The next badge-maker will be a major release anyway due to dropping node 10.

chris48s

Did another slightly more detailed pass over this and left some more comments

chris48s · 2022-05-05T08:08:24Z

core/base-service/base-static.js

+          next()
+        }
+      },
+      this.makeExpressHandler(serviceContext, serviceConfig)


Is dropping the early return in the 304 case intended here?
It looks like we are sticking with early return in core/base-service/redirector.js

chris48s · 2022-05-05T08:09:23Z

core/base-service/legacy-request-handler.spec.js

-        )
-      }
-
-      it('should set the expires header to current time + defaultCacheLengthSeconds', async function () {


I think we probably do want to replicate at lest some of these tests, particularly for the cache headers. What level do you think would make most sense to test this at with the revised architecture?

chris48s · 2022-05-05T08:09:53Z

core/base-service/redirector.js

-        let urlSuffix = ask.uri.search || ''
+        let urlSuffix = url.parse(req.url).search ?? '' // eslint-disable-line node/no-deprecated-api


Can we do this with let urlSuffix = new url.URL(req.url).search instead of introducing a deprecated call here? I think that should be the same.

chris48s · 2022-05-05T08:10:16Z

core/server/server.spec.js

-      // TODO It would be nice if this were 404 or 410.
-      expect(statusCode).to.equal(200)
+      expect(statusCode).to.equal(410)


chris48s · 2022-05-05T08:10:41Z

core/server/server.js


-    await new Promise(resolve => camp.on('listening', () => resolve()))
+    this.server.setTimeout(this.config.public.requestTimeoutSeconds * 1000)


If we're going to move to just dropping the connection on timeout, I think we can just get rid of the requestTimeoutMaxAgeSeconds setting as it won't do anything

requestTimeoutSeconds is an optional setting (for self hosting users). We should handle the case where it is not set.

paulmelnikow · 2022-05-05T18:58:46Z

Thanks for the comments. Will nudge this forward when I can!

PyvesB · 2023-12-31T11:00:01Z

Given that there hasn't been any activity for a while, I'm going to close this pull request. Feel free to ping us or reopen it if you get a chance to look into it again. 😉

If others think that repository level projects would be an option and want to pick this up, you can simply fetch the commits by running git fetch origin pull/7877/head:pr-7877. Change the implementation to target repository level projects and open a new pull request with a co-authored trailer included in the commit message to give attribution to the original author.

chris48s · 2023-12-31T16:25:11Z

It is still my ambition to pick this up and finish it at some point, but agree that there is limited value in keeping the PR hanging around

paulmelnikow added 17 commits April 16, 2022 23:29

Begin to replace scoutcamp with express

6d72fd6

Core tests passing

7c071e3

Move one test

6e6cec9

Fix / remove obsolete tests

c3097aa

Renames

aeebfaa

Remove obsolete code

cf6b5b1

Remove more obsolete code

15043df

Finish rename

903bef2

Fixes

aa046cb

More tests passing

be9d490

More tests passing

8cbc8cb

More fixes

e8f59a2

Progress / cleanup

6d77534

Update integration tests

b4f7ec3

Clean diff

00df3e1

Update docs and suggest test

b19ca20

Merge branch 'master' into express

78b886c

paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Apr 22, 2022

paulmelnikow added 4 commits April 22, 2022 12:49

Clean up acceptor test

18c76e3

Clean lint

9d6b3e0

Clean up makeBadge tests

25ab480

Don't alias this

416ef39

paulmelnikow commented Apr 22, 2022

View reviewed changes

paulmelnikow marked this pull request as ready for review April 22, 2022 17:27

paulmelnikow changed the title ~~Migrate to Express~~ Migrate to Express [*****] Apr 22, 2022

This was referenced Apr 22, 2022

Stringify and trim whitespace from a few services [gem swagger githubissues githubmilestone opencollective] #7880

Merged

Modernizing the request code #3368

Open

This was referenced Apr 22, 2022

Modernizing the server #3328

Open

Move JSON output out of badge-maker and into server #4948

Open

paulmelnikow added 2 commits April 22, 2022 15:09

Wee bit of cleanup

1761aab

Merge branch 'master' into express

fc13e8e

paulmelnikow commented Apr 22, 2022

View reviewed changes

Tweak style

ca6ae88

paulmelnikow mentioned this pull request Apr 22, 2022

Stringify [githublanguagecount] message #7881

Merged

Merge branch 'master' into express

fc68b88

chris48s reviewed Apr 24, 2022

View reviewed changes

chris48s mentioned this pull request May 1, 2022

Total Lines of Code gives HTTP 408 periodically #7889

Closed

chris48s reviewed May 5, 2022

View reviewed changes

PyvesB closed this Dec 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Express [*****] #7877

Migrate to Express [*****] #7877

paulmelnikow commented Apr 22, 2022 •

edited

Loading

shields-ci commented Apr 22, 2022 •

edited

Loading

lgtm-com bot commented Apr 22, 2022

paulmelnikow Apr 22, 2022

chris48s Apr 24, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow commented Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

paulmelnikow Apr 22, 2022

chris48s commented Apr 24, 2022

chris48s Apr 24, 2022

chris48s Apr 24, 2022

chris48s Apr 24, 2022

chris48s Apr 24, 2022

chris48s left a comment

chris48s May 5, 2022

chris48s May 5, 2022

chris48s May 5, 2022

chris48s May 5, 2022

chris48s May 5, 2022

paulmelnikow commented May 5, 2022

PyvesB commented Dec 31, 2023

chris48s commented Dec 31, 2023

		describe('The badge generator', function () {
		describe('color test', function () {

		let urlSuffix = ask.uri.search \|\| ''
		let urlSuffix = url.parse(req.url).search ?? '' // eslint-disable-line node/no-deprecated-api


		await new Promise(resolve => camp.on('listening', () => resolve()))
		this.server.setTimeout(this.config.public.requestTimeoutSeconds * 1000)

Migrate to Express [*****] #7877

Migrate to Express [*****] #7877

Conversation

paulmelnikow commented Apr 22, 2022 • edited Loading

shields-ci commented Apr 22, 2022 • edited Loading

lgtm-com bot commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris48s commented Apr 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris48s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented May 5, 2022

PyvesB commented Dec 31, 2023

chris48s commented Dec 31, 2023

paulmelnikow commented Apr 22, 2022 •

edited

Loading

shields-ci commented Apr 22, 2022 •

edited

Loading