realm / realm-js

Realm is a mobile database: an alternative to SQLite & key-value stores
https://realm.io
Apache License 2.0
5.69k stars 563 forks source link

Align errors across Realm JS and Realm Web #3476

Open kraenhansen opened 3 years ago

kraenhansen commented 3 years ago

Goals

It would be great if the errors thrown would align on properties exposed and values used. This would enable sharing code which catches and handles errors regardless of the weather or not the underlying implementation is using Realm JS or Realm Web, but more importantly shared documentation of error handling across Realm JS and Realm Web.

Expected Results

Errors thrown, should be instances of Error (such that it records and expose a stacktrace) and also expose the server's error code, message and the HTTP status code (and perhaps URL and method to help debugging).

Actual Results

Realm Web

When a request to the server fails in Realm Web a MongoDBRealmError is thrown (exposing the servers application error code and message, alongside the HTTP status code, url and method). At the time of writing this, the codes are not enums with JSDoc strings.

Realm JS

In Realm JS, Realm Object Store will call callbacks with instances of AppError and these will be transformed in to plain-ol' JavaScript objects. Which is not ideal as they don't record a stacktrace. And does not expose the status and application error codes to the user.

Steps to Reproduce & Code Sample (Realm JS)

package.json

{
  "name": "realm-js-error-test",
  "version": "0.1.0",
  "private": true,
  "main": "index.js",
  "scripts": {
    "test": "node index.js"
  },
  "dependencies": {
    "realm": "^10.1.1"
  }
}

index.js

const Realm = require("realm");
const assert = require("assert");

async function run() {
  try {
    const app = new Realm.App("invalid-app-id");
    const credentials = Realm.Credentials.anonymous();
    const user = await app.logIn(credentials);
  } catch (err) {
    assert(err instanceof Error, 'Expected err to be an actual Error');
  }
}

run().then(() => {
  console.log("All done ...");
}, err => {
  console.error(err);
  process.exit(1);
});

Version of Realm and Tooling

emragins commented 3 years ago

Just a thought that from a security perspective, it's not always ideal for the client to get the stack trace from the server. This typically falls into the bucket of exposing information that the client does not need to know, and I've spent many hours on other projects writing wrappers to avoid leaking such details.

This isn't to say I don't see the appeal, but showing the stack trace might actually end up creating more work on others who don't want that information leaked. It's not at all uncommon to have to check server logs for more information.

Please note, I'm extremely new to Realm and just getting back into javascript after years, so I recognize this comment might be off-base from ignorance. However, I don't think my ignorance on that front discounts the point made from years of experience writing back-end integration code and dealing with strict security reviews.

kraenhansen commented 3 years ago

@emragins I appreciate the heads-up. Technically we consider both Realm JS and Realm Web client-side SDKs (although both can be used from a Node.js process) and I do think an error should contain a stack property. If a developer choose to send that along to their users in a server response or send only the message (which shouldn't contain a stack) is their choice (IMO).