ETT-1128: Return a 404 or 400 for certain errors

[aelkiss]

Most errors in babel are handled via the 'assertion' functionality, which ultimately raises an exception. This middleware looks at the error and if the error matches a specific pattern chooses to respond with 404 instead of 500. Although this is kind of

Although it would be preferable if knowledge about the error could be closer to the actual problem (and HTTPExceptions has a mechanism for communicating this via an exception object), many of the places the errors are raised have no knowledge of the HTTP context at all, and it would probably be more disruptive to change places that might provoke an error (and figure out how to do it in a way that comes through) than to do this post-processing.

See the ticket ETT-1128 for a variety of other approaches I tried that didn't end up working out.

The playwright tests fail, because the middleware is not configured if HT_DEV is set. One option could be disabling the stack trace functionality entirely in development, or we could not have the tests. I'm not sure if there's a middle ground.

[aelkiss]

Consensus seems to be that the stack trace pages are of somewhat limited utility.

We could comment out the stack trace middleware & re-enable locally if we needed it for specific purposes.

[aelkiss]

I think this should be good to go, based on our discussion this morning.

We could probably add some more playwright tests based on some of the errors I added (esp. with 400 errors, other apps, etc) but I don't think they necessarily need to be comprehensive so long as they cover the functionality in general.

[aelkiss]

I'm having issues getting HTHTTPExceptions to work as expected with imgsrv. I end up getting an unstyled 500 error rather than the styled error page. Maybe the exception info isn't making it through?

[aelkiss]

After doing some debugging it looks like rethrow is getting set, but I'm not sure where/why.

[aelkiss]

I might just take the rethrow functionality out, since we aren't using it.

[aelkiss]

@moseshll @carylwyatt I think this should be ready for any review you want to do at this point.

[moseshll]

Added a couple minor comments. This conforms to my expectations in all other respects. APPROVE

[moseshll]

I know this is copied from HTErrorDocument.pm but in both cases I would be curious to know the purpose of this substitution. Is it still needed?

[carylwyatt]

@aelkiss Locally, when I add some nonsense to pt's ID query param, I'm getting a 404 in the browser console and I get the correct "Not Found" title in the browser tab, but it's a blank screen. Turns out these templates are linking to js/css assets from the assets directory and not the cache-busted/manifest versions, so they're not loading locally (we don't build those locally). 😵‍💫

babel/mdp-web/production_404.html

Lines 8 to 9 in 973ce33

	<link rel="stylesheet" href="/common/firebird/dist/assets/index.css" />
	<script type="module" src="/common/firebird/dist/assets/index.js"></script>

While this works in production, it's not how we normally load firebird assets. I reworked what we do in skeleton.xsl and catalog layout.tpl to work for these error html pages:

 <script>
      async function loadFirebirdAssets() {
        function addScript(options) {
          let scriptEl = document.createElement('script');
          if (options.crossOrigin) {
            scriptEl.crossOrigin = options.crossOrigin;
          }
          if (options.type) {
            scriptEl.type = options.type;
          }
          scriptEl.src = options.href;
          document.head.appendChild(scriptEl);
        }

        function addStylesheet(options) {
          let linkEl = document.createElement('link');
          linkEl.rel = 'stylesheet';
          linkEl.href = options.href;
          document.head.appendChild(linkEl);
        }

        try {
          const response = await fetch('/common/firebird/dist/manifest.json');
          const manifest = await response.json();

          const assets = {
            stylesheet: '/common/firebird/dist/' + manifest['index.css'].file,
            script: '/common/firebird/dist/' + manifest['index.html'].file,
          };

          addStylesheet({ href: assets.stylesheet });
          addScript({ href: assets.script, type: 'module' });
        } catch (err) {
          console.error('Failed to load firebird assets:', err);
        }
      }

      loadFirebirdAssets();

      // in case any of the links and scripts fail
      setTimeout(function () {
        document.body.style.visibility = 'visible';
        document.body.style.opacity = '1';
      }, 1500);
    </script>

I can make this update on the error pages in mdp-web and push (to this branch or another one) if this makes sense to you!

[aelkiss]

@carylwyatt Yes if you can update those error pages here let's go ahead and do that -- thanks!

[carylwyatt]

@aelkiss I see that you specify production_404 and production_500 but we had some other error pages and I have no idea if/when those are used, so I just updated those, too. Staged on dev-3 and it's loading the manifest assets now, so this looks good to go: https://dev-3.babel.hathitrust.org/cgi/pt?id=not.page

Edit to add: I only updated the scripts in the <head> tag, but my linter must've updated the formatting in the rest of the document because it looks like a lot of changes.