thenativeweb / node-cqrs-domain

Node-cqrs-domain is a node.js module based on nodeEventStore that. It can be very useful as domain component if you work with (d)ddd, cqrs, eventdenormalizer, host, etc.
http://cqrs.js.org/pages/domain.html
MIT License
269 stars 57 forks source link

How to handle errors thrown from defineCommand / defineEvent handlers? #152

Open imissyouso opened 4 years ago

imissyouso commented 4 years ago

Hi! For example:

export = domain.defineEvent({},  (data, aggregate) => {
    aggregate.set('balance', data.balance);
    aggregate.set('value', data.value);

    throw new FakeError(); // unhandled, something went wrong or TypeError
});

how to handle such type of errors?

Approach listed below doesn't help.

    async handle(data: object) {
        return new Promise((resolve, reject) => {
            try {
                this.domain.handle(data, (err) => {
                    if (err) {
                        reject(err);
                    } else {
                        resolve();
                    }
                });
            } catch (e) {
                reject(e);
            }
        });
    }

This problem is similar for defineViewBuilder of eventdenormalizer. Can't catch errors there too.

another a little question: Why custom command handler is so bad?

Command Handler (Be careful!!!)
Is your use case not solvable without a custom command handling? Sagas? Micro-Services?

what if I need to make external operation like 3rd party API http call and generate event with resulted data without saving to eventstore. This event then can be handled by sagas to generate command to update required aggregate.

adrai commented 4 years ago

These type of errors should theoretically be catched via normal, error argument in the handle callback or as uncaughtException, i.e:

process.on('uncaughtException', function (err) {
})

Can you try to update to v2.14.78 ?

adrai commented 4 years ago

Regarding the custom command handler, you are free to use it and to call external operations, etc... But I would suggest to solve this via a dedicated saga, which will react on an "preparation"-event, do an external api call and generate the "complete"-command

imissyouso commented 4 years ago

These type of errors should theoretically be catched via normal, error argument in the handle callback or as uncaughtException, i.e:

process.on('uncaughtException', function (err) {
})

Can you try to update to v2.14.78 ?

thank you, now it works. I think we should move list of errors which produces commandRejected events into domain options, because now it is hardcoded in createCommandRejectedEvent of domain.js

    if (err instanceof ValidationError || err instanceof BusinessRuleError ||
        err instanceof AggregateDestroyedError || err instanceof AggregateConcurrencyError ||
        err instanceof DuplicateCommandError) {
      dotty.put(evt, this.definitions.event.payload, {
        command: cmd,
        reason: {
          name: err.name,
          message: err.message,
          more: err.more
        }
      });

      if (!!this.definitions.event.revision && dotty.get(evt, this.definitions.event.revision) === undefined && err.more && err.more.aggregateRevision > -1) {
        dotty.put(evt, this.definitions.event.revision, err.more.aggregateRevision);
      }
    } else {
      evt = null;
    }

in reason of this, if error occurred in commandHandler (for example TypeError), we can't rollback our Saga and as result end user will see the hanged operation. I think we should do rollbacks where it possible even if error was made by programmer on code level.

adrai commented 4 years ago

When Designing this piece of code I discussed this with other persons. The decision was to always “punish” programmer errors this way, but feel free to suggest a PR.

imissyouso commented 4 years ago

About

process.on('uncaughtException', function (err) {
})

I understand that we can use it in emergency cases if by some reason callback handler was not executed with error object in first argument. But this situation does not allow us to log this error by our OpenTracing server (Jaeger) because to close current trace we need to pass trace id to error handler of Jaeger. In uncaughtException we do not have access to such trace ID because Error object does not contains it itself.

I made this note to explain why we should try to pass error in handler callback in all cases where it possible and leave handling by uncaughtException only for unpredicted errors, for example on library level. This regards all libs not only Domain.

dmitry-textmagic commented 4 years ago

But then in case of the exception inside command handler, we have to deal with unfinished (not completed nor rolled back) saga and manually rollback actions performed before this exception. On the other hand, imagine that command handler catch any exceptions and thrown them as, let me say, CommandHandlerError and this error type would have listed in the code @imissyouso noted above (with ValidationError, BusinessRuleError etc). Then "commandRejected" event will be produced in a natural way, as it was ValidationError or BusinessRuleError, and the saga would be rolled back.

I understand that having an unhandled exception in command is an unusual situation, but now we have to catch it and do produce "commandRejected" manually, and it looks kinda strange to me because other types of exceptions produce it by themselves.

imissyouso commented 4 years ago
When Designing this piece of code I discussed this with other persons.
The decision was to always “punish” programmer errors this way, but feel free to suggest a PR.

the strange way to punish programmers especially if we are talking about highload systems where in reason of programmer error we will have to rollback all hanged sagas manually - sounds bad for business :)

adrai commented 4 years ago

Always happy for a PR.

imissyouso commented 4 years ago

Always happy for a PR.

currently we are at stage of investigating and collecting potential issues before the real start of development. As soon as we will do it wait PR from us :)