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

Fix for error(s) swallowing #151

Closed ssipos90 closed 4 years ago

ssipos90 commented 4 years ago

The bit of code replaced was replacing all the errors of an alternative JSON schema statement (anyOf, oneOf) with it's sub-errors, effectively removing other errors in the list, although this was an edge case.

I'd better explain with an example.

For context, we're adding some items on a map and each item can be either a pin or a polygon. Naturally, there's an updateItemLocation command; it has only one field in the payload, a geojson Point or Polygon which we've implemented using oneOf.

If you try to send a polygon that's not valid (eg: 3 coordinates instead of 4), the library fails with something like /payload/location => does not match any schemas, falling on that case (line 49), pulling the first sub-error sub-errors as the main errors, which, in turn, don't have any other sub-errors, making errors = null, (I know it's a bit hard to wrap your head around the issue). The issue is that you lose some context information that's inside the initial error and consistency is lost (our script extracts from error.more the field and message to build something simple for our API).

The behavior is not consistent, if you have some additional field that's before in the payload (JSON props are ordered alphabetically), then the condition will never be true. However, it gets worse if you have a field after your alternative field error (some xyz field) and that one errors too, that error is removed from the payload.

My patch would iterate through all errors, and if any is matching the condition, that error in particular will be replaced by it's first sub-error, keeping the original reason for this code valid.

Didn't find any tests to update.

adrai commented 4 years ago

LGTM

nanov commented 4 years ago

LGTM as well. A good catch as well. Getting merged.