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

Replace numeric error codes with strings #309

Merged
merged 6 commits into from
Nov 20, 2019
Merged

Replace numeric error codes with strings #309

merged 6 commits into from
Nov 20, 2019

Conversation

alecgibson
Copy link
Collaborator

Fixes #287
Fixes #198

This is a breaking change that moves us from using numeric error codes
to string-based codes similar to Node.

The motivation for this is:

  • more descriptive error codes
  • stop implying that errors can be broadly classified together (in the
    same way that HTTP errors might)

This change also exposes these codes on ShareDBError.code, so that
they are easily discoverable, and can be referenced by consumers for
programmatic error handling.

@coveralls
Copy link

coveralls commented Sep 13, 2019

Coverage Status

Coverage increased (+0.1%) to 96.804% when pulling c0e9e0b on error-codes into 9effa65 on master.

@alecgibson
Copy link
Collaborator Author

I've done this PR in a relatively "dumb" way, just blindly swapping out numeric codes for strings as I saw fit. While we're here, I think it would be good to think about:

  • specificity: some of our errors are very specific, and some are very broad; do we want/need this to be consistent? I usually think that the message component can be used to add any extra context, but there may be cases where an error that sounds generic may want to be handled in a different way programmatically, where it would want a different error code.
    • I've notably reduced specificity for all the "not implemented" messages, because having different codes felt redundant. If this is wrong, let me know.
    • Are there other places to reduce specificity? What about:
      • CREATE_DATA_MUST_BE_AN_OBJECT
      • DEL_MUST_BE_TRUE
      • DOCUMENT_ALREADY_CREATED vs DOCUMENT_CREATED_REMOTELY
      • SRC_AND_SEQ_MUST_BE_SET_TOGETHER, INVALID_SRC, INVALID_SEQ
      • others?
    • we may need to increase specificity on the INVALID_VERSION error? We use this to refer to the doc/op/snapshot version. Do we need different errors for each of these? If not, is there a better way to name this code (to differentiate it from eg INVALID_PROTOCOL_VERSION)
  • naming: this is the hard part!
    • All the constant keys are missing the ERR_ prefix to make consumer code a bit neater and less overloaded than ShareDBError.code.ERR_... (it's clear it's an error code already, I think). Is this okay?
    • Do we want a policy on frontloading? For example, should we try to always have <subject>_<state>? So CLIENT_ID_INVALID rather than INVALID_CLIENT_ID? I personally think that this format helps to remove some noise when looking at the error codes; helps to prevent duplicate codes; makes it a bit clearer what can be wrong with a particular thing. I thought I'd wait for opinions before actually doing this
  • error code overlaps - some of the error codes were overlapping (notably the 4XXX series). I didn't do a find-replace, so there shouldn't be any breakages, but you can never be too careful
  • default code: I think the only other slightly controversial thing I've done is add a default code to the ShareDBError class. It seemed like a sensible thing to do.

lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
@alecgibson
Copy link
Collaborator Author

As a rough rule of thumb, let's group object formatting issues under single codes, but for any user input (eg DEL_MUST_BE_TRUE), let's have specific error codes.

lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
@alecgibson
Copy link
Collaborator Author

Let's go ahead with <subject>_<state>, but subject might be the act of doing something eg apply

lib/submit-request.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
lib/error.js Outdated Show resolved Hide resolved
lib/ot.js Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@alecgibson alecgibson left a comment

Choose a reason for hiding this comment

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

We should also add something to the README that talks about errors a consumer is likely to run into, and what do about them:

  • ERR_OP_SUBMIT_REJECTED (normal - for telling client to roll back)
  • ERR_OP_ALREADY_SUBMITTED (normal - the middleware abuses it to stop all further execution by looking like an error, but then the agent just swallows it because it's non-fatal)
  • ERR_SUBMIT_TRANSFORM_OPS_NOT_FOUND
  • ERR_MAX_SUBMIT_RETRIES_EXCEEDED
  • ERR_DOC_ALREADY_CREATED
  • ERR_DOC_ALREADY_DELETED
  • ERR_DOC_TYPE_NOT_RECOGNISED
    (Advanced use:)
  • ERR_OP_NOT_ALLOWED_IN_PROJECTION
  • ERR_DEFAULT_TYPE_MISMATCH

Alec Gibson added 3 commits October 31, 2019 10:42
Fixes #287
Fixes #198

This is a breaking change that moves us from using numeric error codes
to string-based codes [similar to Node][1].

The motivation for this is:

  - more descriptive error codes
  - stop implying that errors can be broadly classified together (in the
    same way that HTTP errors might)

This change also exposes these codes on `ShareDBError.code`, so that
they are easily discoverable, and can be referenced by consumers for
programmatic error handling.

[1]: https://nodejs.org/api/errors.html#nodejs-error-codes
This change aims to reword our new error codes according to review
comments. The main changes are:

  - Prefix all code keys with `ERR_`
  - Rename to follow the <subject>_<state> format
  - Tweak some error code specificity
  - Provide a bit more context with some error messages
  - Add documentation for common errors
  - Americanize spelling
  - Add a special error for invalid Milestone DB arguments
@alecgibson
Copy link
Collaborator Author

While we're here, let's also replace all of our anonymous error objects with instances of an actual Error. We should create our own Error class that extends the base, but with its own type for instanceof checking. We can base this loosely on the MDN example.

In short we should:

  • set the name property
  • try to use captureStackTrace if available
  • otherwise just construct a new Error and copy the stack from that

lib/milestone-db/memory.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Alec Gibson added 2 commits November 14, 2019 09:57
This change drops our dependence on `make-error`, and instead implements
our own simple `ShareDBError` class, which extends `Error`.

We also wrap all anonymous error objects in this `ShareDBError`, so that
its type can be checked by consumers.
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.

[1.0.0] Switch error codes from numbers to strings Error code documentation is out-of-date
4 participants