Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify handling of error responses in page endpoints #5633

Closed
Conduitry opened this issue Jul 20, 2022 · 4 comments
Closed

Unify handling of error responses in page endpoints #5633

Conduitry opened this issue Jul 20, 2022 · 4 comments
Labels
feature request New feature or request
Milestone

Comments

@Conduitry
Copy link
Member

Describe the problem

The handling of error responses from page endpoints is currently a bit confusing and unflexible.

If a GET page endpoint returns a 4xx or 5xx response, then any props returned in its body are ignored and the nearest __error.svelte page is rendered. If body is an Error instance, then this (along with its enumerable properties) are available to that page.

If a non-GET page endpoint returns a 4xx or 5xx response, then any props returned in its body argument are passed to the regular (non-error) page, merged with the GET page endpoint body props, if any. If body is an Error instance, then (I believe) the nearest __error.svelte page is rendered instead and is sent this error.

Describe the proposed solution

I think it would be nice, whether the user is on a GET or non-GET endpoint, to use the response from the body - whether it's a POJO or an Error - to determine whether to render the regular page or the nearest __error.svelte page, respectively.

I'm not sure what other fallout of this might be.

I'm not sure what we want to do if a page endpoint returns a 4xx/5xx status but no body at all. Create a new Error() whose message is the appropriate description for that HTTP status code?

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

@Conduitry Conduitry added this to the 1.0 milestone Jul 20, 2022
@Conduitry Conduitry added the feature request New feature or request label Jul 20, 2022
@Conduitry
Copy link
Member Author

Some open questions I just thought of:

Would we want to continue to merge the body of the non-GET and the GET response in all cases?

How do we want to deal with competing status values from the non-GET and the GET response?

Is calling the GET handler as part of a non-GET request too much magic after all? Is there a more explicit way to deal with this, where we can avoid the above questions, but which isn't a huge hassle to use?

@composite
Copy link

composite commented Aug 4, 2022

I think it would be nice to have case of rendering regular page or error page. such as:

  • If page component has load function, render this page. otherwise, render nearest error page.
  • If load function ran successfully, render this page. or render nearest error page on error.

for ALL METHODs(GET or non-GET) request with page endpoint routes.

@richiejp
Copy link

richiejp commented Aug 6, 2022

I'm pretty confused about how to handle the many potential errors which have appropriate HTTP status codes other than 500. It would help if GET and other requests were consistent at least. However I'm not sure that gets to the root of the problem.

I'm not sure I grasp the current situation, so I'll say what I am doing now. Currently I throw my own exception types with HTTP status code attached.

// $lib/error.ts
export class Http extends Error {
  status: number = 500;

  constructor(message: string, status?: number) {
    super(message);

    if (status)
      this.status = status;
  }
}

// In /blogs/[user]
import error from '$lib/error';

export const GET: RequestHandler = error.wrap(async (ev) => {
  const { params } = ev;
  const user_id = await db.get('user', params.user);

  if (!user_id)
    throw new error.Http(`Could not find user nickname: ${params.user}`, 404);

...

The above tries to get a page like /blogs/[user]/ and if the user doesn't exist then we bail out with 404. Of course I could just return 404 in this case, but often errors happen in the middle of some library function and I don't want to pass an error all the way back up the call chain. Not because I dislike that style of error handling, it's just not consistent with a lot of JS code that does use exceptions. Also if it weren't a GET request then we need to pass an error as the body anyway.

Anyway, note the error.wrap function above, that looks like below

export class Span extends Http {
  span: string;

  constructor(message: string, status: number, span: string) {
    super(message, status);

    this.span = span;
  }
}

export function to_resp(event: RequestEvent, error: Http | Error):
RequestHandlerOutput<ResponseBody>
{
  let resp;

  if (error instanceof Http) {
    resp = {
      status: error.status,
      body: new Span(error.message, error.status, event.locals.span.id)
    };
  } else {
    resp = {
      status: 500,
      body: new Span(error.message, 500, event.locals.span.id)
    };
  }

  return resp;
}

interface AsyncRequestHandler<
  P extends Record<string, string> = Record<string, string>
> extends RequestHandler<P, ResponseBody> {
  (event: RequestEvent<P>): Promise<RequestHandlerOutput<ResponseBody>>;
}

export function wrap<
  P extends Record<string, string>
> (fn: AsyncRequestHandler<P>): AsyncRequestHandler<P>
{
  return async (event) => {
    return fn(event).catch((error) => {
      return to_resp(event, error);
    });
  };
}

This catches any exceptions (or a promise rejection for some other reason), then augments the Error object with a 'span' ID so that it can be displayed to the end user to help with technical support.

Because SvelteKit discards any props I have to put it in the error. That's OK I suppose (well maybe not), the problem is being forced to wrap the endpoints in this way. I'm not sure if that is necessary, but the handleError hook isn't able to modify the response AFAICT. Meanwhile in handle the resolve callback returns a Response which I think is too late in the chain. Unless I am mistaken, the error page has already been rendered by this point.

Changing the status inside the __error.svelte load function seems to signal that an error happened actually in the load function which is not what I want. I suppose this shows that the HTTP status code should never be used to indicate that an internal error happened. The status code is just for the client's information.

Setting the body to an error object might also not be the best idea. As the error object gets serialised and sent to the client. The stack is removed, but that's not necessarily the only sensitive information. Perhaps, when there is an uncaught error (or error is set on RequestHandlerOutput or LoadOutput), we want a hook to intercept it and decide if we want to render a different page and what props and status to explicitly send to that page.

@dummdidumm
Copy link
Member

dummdidumm commented Aug 19, 2022

With the API rework this behavior has changed:

Page endpoints (now +page.server.js):

  • load (previously GET) provides the throw error(..) helper to throw exceptions, they trigger the error page handling. If you return something from the load function, the result is treated as a success and it's passed to the Svelte page (if you return Error it will probably return something unexpected because that's not a POJO
  • POST works the same when you throw error(..), in case of validation errors you return { error: .. } from it which is then shown alongside the data from the load function

Standalone endpoints (now +server.js):

  • if you throw error(..) or return a 50x or whatever-non-20x response, that doesn't trigger error page handling at all. These endpoints are separate to the page, so if you visit them directly, they are not in any way embedded into the layouts etc. You should call these from fetch in your client

Therefore I think the things in here are outdated, and the new behavior is consistent, so I'm closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants