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

implement RFC7807 #65

Closed
wants to merge 2 commits into from
Closed

implement RFC7807 #65

wants to merge 2 commits into from

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Feb 22, 2022

This is a PR for #64

This implements an RFC7807 conform toRFC7807 method to Fastify Error.

The only thing which is slightly deviating from RFC7807 is, that I dont put the attributes of the additional Error-Details on the root of the Error-Object. RFC7807 suggests, that if you have errors, you should put them into the root like this:

 {
  "type": "https://example.net/validation-error",
  "title": "Your request parameters didn't validate.",
  "invalid-params": [ {
                        "name": "age",
                        "reason": "must be a positive integer"
                      },
                      {
                        "name": "color",
                        "reason": "must be 'green', 'red' or 'blue'"}
                    ]
  }

I dont want that we have the necessity to enforce that some words are reserved, as we have e.g. already type and title in the root of the Object, so I implemented it by saying all additional errors should be in an details attribute.

But of course I could implement it like RFC7807 suggests, and put the attributes on the root of the Object, if you request it.

What we should then discuss is, if it is good or bad, that I implemented details as an Array. I also tend to implement the details as an Object.

Looking for your feedback

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 22, 2022

On second thought, I think details should be an Object and not an Array.

title: this.name,
status: this.statusCode,
detail: this.message,
instance: instance || '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the place where the error in the fastify route happens. Like you have an ForbiddenError in GET /users/1234 then the instance would be /users/1234

}
FastifyError.prototype[Symbol.toStringTag] = 'Error'

FastifyError.prototype.toString = function () {
return `${this.name} [${this.code}]: ${this.message}`
}

FastifyError.prototype.toRFC7807 = function (instance, details) {
return {
type: this.uriReference || 'about:blank',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the URI reference should be taken from the request itself and defined at the top for all errors of the same kind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I called it uriReference because according to RFC7807 it is called type. But that would be imho confusing.

https://datatracker.ietf.org/doc/html/rfc7807#section-3.1

A URI reference [RFC3986] that identifies the
     * problem type.  This specification encourages that, when
     * dereferenced, it provide human-readable documentation for the
     * problem type (e.g., using HTML [W3C.REC-html5-20141028]).
     * When this member is not present, its value is assumed to be
     * "about:blank".

So e.g. we create a FastifyError with code 'FST_ERR_BAD_URL' then the uriReference would be https://www.fastify.io/docs/latest/Reference/Errors/#fst_err_bad_url

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats why this is an parameter of the createError function and not on the toRFC7807. On the toRFC7807 I use the instance parameter to provide uri from the request.

I wanted to ensure that we have no potential backwards breaking, but I could put instance and details as parameters of the actual Error. So toRFC7807 would just this.instance and this.details instead using them from toRFC7807.
Maybe that is better, as we would have those values at the time we throw the Error?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 22, 2022

@mcollina

Do you prefer it more like this?

'use strict'

const { inherits, format } = require('util')

function createError (code, message, statusCode = 500, Base = Error, uriReference) {
  if (!code) throw new Error('Fastify error code must not be empty')
  if (!message) throw new Error('Fastify error message must not be empty')

  code = code.toUpperCase()

  function FastifyError (a, b, c, instance, details) {
    if (!new.target) {
      return new FastifyError(a, b, c, instance, details)
    }
    Error.captureStackTrace(this, FastifyError)
    this.name = 'FastifyError'
    this.code = code

    // more performant than spread (...) operator
    if (a && b && c) {
      this.message = format(message, a, b, c)
    } else if (a && b) {
      this.message = format(message, a, b)
    } else if (a) {
      this.message = format(message, a)
    } else {
      this.message = message
    }

    this.statusCode = statusCode || undefined
    this.uriReference = uriReference || undefined
    this.instance = instance || undefined
    this.details = details || undefined
  }
  FastifyError.prototype[Symbol.toStringTag] = 'Error'

  FastifyError.prototype.toString = function () {
    return `${this.name} [${this.code}]: ${this.message}`
  }

  FastifyError.prototype.toRFC7807 = function (instance, details) {
    return {
      type: this.uriReference || 'about:blank',
      title: this.name,
      status: this.statusCode,
      detail: this.message,
      instance: this.instance || instance || '',
      code: this.code,
      details: this.details || details || {}
    }
  }

  inherits(FastifyError, Base)

  return FastifyError
}

module.exports = createError

@jsumners
Copy link
Member

I think this RFC is too tightly coupled to application implementations to be implemented generically.

@zekth
Copy link
Member

zekth commented Feb 22, 2022

I think this RFC is too tightly coupled to application implementations to be implemented generically.

I'd say it's not good to be implemented generically by default indeed. On the other hand there is some aspects that are quite appealing for some of our use cases. Like the parsing of the AJV errors for bad requests.

Shouldn't this functionnality be another function? Like createRFCerror or something?

@Uzlopak Uzlopak closed this Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants