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

isClientSafe DDP errors #8756

Merged
merged 3 commits into from Jun 21, 2017

Conversation

Projects
None yet
4 participants
@aldeed
Contributor

aldeed commented Jun 3, 2017

This is a possible implementation of something we discussed about a year ago here: https://forums.meteor.com/t/meteor-guide-methods/19662/34?u=aldeed

If any thrown error has isClientSafe set to true, it is cast into a Meteor.Error to be sent back to the client. I think an ideal implementation would not even reference Meteor.Error anywhere in the DDP code, but this is a simple, safe, backwards compatible solution for now.

The goal here is for NPM packages like simpl-schema to be able to throw validation or other errors and have them sent back to the client call.

One question: Should it do the same thing for any sanitizedError that has isClientSafe set?

Closes #7305

@stubailo

This comment has been minimized.

Contributor

stubailo commented Jun 3, 2017

I think a similar thing would be useful for GraphQL as well, do we think it might be nice to promote isClientSafe?

Might it be better instead of a flag to maybe have a version of the error that is client safe, as a field on the error object? Like it would have validation information but maybe not stack traces?

@zimme

This comment has been minimized.

Contributor

zimme commented Jun 4, 2017

@stubailo, I think that might be a good idea, it gives the package maintainers more control of the shape of the exception that would be forwarded to the client. But, doesn't the forwarded error need to be an instance of Meteor.Error for the meteor client to understand it?

@stubailo

This comment has been minimized.

Contributor

stubailo commented Jun 4, 2017

On the wire, it's just a JSON blob - so the client can make that into a Meteor.Error if it wants. I'm just suggesting a different server-side mechanism for picking out what data is sent over the wire about the error.

Meteor.Error on the server will also be able to do this by default, meaning people using it will end up with the exact same behavior as before.

We can promote this mechanism in the Apollo graphql-server package as well, because it also has a formatError step where we could pick out that field by default and return it.

@aldeed

This comment has been minimized.

Contributor

aldeed commented Jun 5, 2017

@stubailo I'm happy to adjust to whatever implementation people want. Just wanted to get something up. My only priority is removing the need for any dependencies, and still being able to pass back additional error details JSON, which SimpleSchema currently does using the details prop.

The sanitizedError prop is pretty much what you are proposing already, right? If we removed the requirement for it to be a Meteor.Error instance. So bigger picture, I see this being basically a generic specification for throwing client safe errors back across the wire. Something like:

"To specify an error that is safe to be sent back to the remote caller, set sanitizedError property on the error that your method throws. This error will be serialized and sent in the DDP message reply. It will then be reconstructed on the client as best as possible."

It could then attempt to generically serialize the error (standard error props + "own" props) rather than explicitly looking for "reason", "details", etc.

Let me know what you all are thinking, and I can update the PR. I just don't know how deep into the DDP spec I should make changes. It should definitely be possible to remove all Meteor.Error references and still be backwards compatible, though.

As an aside, mdg:validation-error could also be ported to an npm package following this new spec.

@benjamn

benjamn requested changes Jun 7, 2017 edited

After some discussion this morning in our triage meeting, I'm a fan of the isClientSafe property as a convention that can be respected by multiple interacting libraries/frameworks, given the limitations of instanceof when you don't have access to the constructor function.

To move forward with this PR:

  1. The Meteor.Error constructor should set this.isClientSafe = true, so that all Meteor.Error objects pass either test. This should also give us sanitizedError.isClientSafe for free.
  2. We should replace all error instanceof Meteor.Error checks with error && error.isClientSafe.
  3. The current implementation needs to change a bit to prevent accidentally wrapping Meteor.Error objects with new Meteor.Error objects.

With those issues addressed, I like this idea a lot!

@aldeed

This comment has been minimized.

Contributor

aldeed commented Jun 8, 2017

Thanks @benjamn, I'll work on those changes this weekend.

@hwillson hwillson referenced this pull request Jun 9, 2017

Closed

Generalize DDP error handling for non-Meteor errors #7305

0 of 3 tasks complete
@aldeed

This comment has been minimized.

Contributor

aldeed commented Jun 12, 2017

@benjamn I made the requested changes. You said to replace error instanceof Meteor.Error but I'm not sure if maybe it should do both checks? The thing that confuses me is how the dependency on the meteor package works. It is basically an implied dependency I guess and so I'm not sure whether it's possible for ddp-server package to be updated but not meteor, in which case isClientSafe would not be set on Meteor.Error.

if (exception.isClientSafe) {
if (!(exception instanceof Meteor.Error)) {
const originalMessage = exception.message;
exception = new Meteor.Error(exception.error, exception.reason, exception.details);

This comment has been minimized.

@benjamn

benjamn Jun 14, 2017

Member

Are we sure that these properties (exception.error, exception.reason, exception.details) will be defined (or safely undefined) for arbitrary exception objects that happen to use the exception.isClientSafe convention? Should Meteor.Error take in the whole exception object and copy its properties (e.g. .stack)?

This comment has been minimized.

@aldeed

aldeed Jun 14, 2017

Contributor

So in Meteor.Error constructor, it would if (error instanceof Error) then copy all "own" props plus message and stack to self?

I like that idea in general, but as I look at this a bit more, I'm not sure that casting to Meteor.Error here is actually necessary. Whatever this function returns is set to the error prop on the DDP payload, and stringifyDDP seems to just clone and send it. addType is not even used for Meteor.Error for EJSON, so it might just work with any error already?

However, I think the client does assume there will be error, reason, and details props in reconstructing the error on the other end.

This comment has been minimized.

@aldeed

aldeed Jun 14, 2017

Contributor

Relevant comment from errors.js:

// Note: The DDP server assumes that Meteor.Error EJSON-serializes as an object
// containing 'error' and optionally 'reason' and 'details'.
// The DDP client manually puts these into Meteor.Error objects. (We don't use
// EJSON.addType here because the type is determined by location in the
// protocol, not text on the wire.)

This comment has been minimized.

@aldeed

aldeed Jun 14, 2017

Contributor

Reconstruction on client would also need to do a more generic copying of properties I think. In livedata_connection.js:

if (_.has(msg, 'error')) {
      m.receiveResult(new Meteor.Error(
        msg.error.error, msg.error.reason,
        msg.error.details));
    } else {
      // msg.result may be undefined if the method didn't return a
      // value
      m.receiveResult(undefined, msg.result);
    }
@@ -1736,10 +1747,10 @@ var wrapInternalException = function (exception, context) {
// provided a "sanitized" version with more context than 500 Internal server
// error? Use that.
if (exception.sanitizedError) {
if (exception.sanitizedError instanceof Meteor.Error)
if (exception.sanitizedError.isClientSafe)

This comment has been minimized.

@benjamn

benjamn Jun 14, 2017

Member

To your question, I think just checking .isClientSafe (without also checking instanceof Meteor.Error) is fine, now that all Meteor.Error objects will have .isClientSafe set to true.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 14, 2017

I'm not sure whether it's possible for ddp-server package to be updated but not meteor, in which case isClientSafe would not be set on Meteor.Error.

We will be bumping at least the minor version of both packages in Meteor 1.5.1, so they should both be forced to update by the Meteor release. Note: we're only bumping the patch version of meteor (from 1.6.1 to 1.6.2), but I'll make sure we go with 1.7.0.

@aldeed aldeed referenced this pull request Jun 19, 2017

Closed

Unknown key in field #128

@benjamn benjamn merged commit a32a979 into meteor:devel Jun 21, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aldeed aldeed deleted the aldeed:client-safe-errors branch Jun 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment