moleculerjs / moleculer

:rocket: Progressive microservices framework for Node.js
https://moleculer.services/
MIT License
6.15k stars 583 forks source link

Parameter validation for events (like actions) #621

Closed erfanium closed 4 years ago

erfanium commented 4 years ago

Event parameters may be unexpected and It creates problems for me. I manually validate the parameters but it can get better if Moleculer has built-in validator (like actions)

something like this (pseudo-code):

module.exports = {
  name: 'payment',
  events: {
    'order.created': {
      // Register handler to the "other" group instead of "payment" group.
      group: 'other',
      params: {
         id: 'string',
         pid: 'string',
         opt: {
            name: 'string'
         }
      },
      handler(payload) {
        // ...
      },
    },
  },
};

Then If parameters were not valid, ignore calling handler function

icebob commented 4 years ago

Good idea. But I think, the ignoring calling is not a good solution because it can cause problems, when you don't know why your event handler is not called. So would be throw error, but it won't go back to the caller, but will be printed to the console.

ccampanale commented 4 years ago

Interesting this should come up because this is partially in line with something I'm working on for my team called "Published Events".

In this case, I would argue what the OP is suggesting is essentially an "Anticorruption Layer" (as discussed by Evans in Domain Driven Design) which is certainly not a bad idea. Of course, as @icebob stated, events should never go back to the caller but rather printed to the console; "Discarded event payload for event '${topic}' which did not adhere to the interface" or something to that effect.

While the current implementation of events is nice I would argue what is missing - especially when tasked with extending a system with new services/features - is knowledge about events services intend to emit and their topics; the ability for a service to define the interface (a parameter validation schema) for an event that it intends to emit. I've been toying around with an implementation that does this, uses Fastest Validator for the param schema, and provides defined "Event Models" in the service metadata. Additionally, a custom REPL command uses the service metadata to show in REPL which services are emitting which events along with their "contracts".

My implementation is still very young and is quite opinionated as I'm trying to take my team in the direction of AVTO (Actor Verb Target Object) based event structures but something that was built into the Moleculer core would be quite nice. The library - as our internal code does - could provide a sort of EventModelFactory which would work similar to how one would define Mongoose models for documents with the difference being that the resulting model would actually generate concrete event objects which could then be emitted/broadcasted:

Imperatively:

const {PublishedEventFactory} = require('moleculer')
const MyEvent = PublishedEventFactory.getModel({
  'my.event', // topic
  {
    id: 'string',
    pid: 'string',
    opt: {
      name: 'string'
    }
  } // payload interface (validation schema)
  // optional broker if factory is not already configured with broker;
  // PublishedFactory.setBroker(this.broker)
)
const anEvent = MyEvent.create({})
// provided payloads which do not adhere to the interface would throw an error here
anEvent.emit()
// or for broadcasting
anEvent.broadcast()
// these would essentially be calls to the encapsulated broker.emit() and 
// broker.broadcast() supplying the concrete event as the payload

Declaritively via the ServiceSchema:

module.exports = {
  name: 'myservice',
  publishedEvents: {
    'MyServiceEvent': {
      topic: 'event.fired',
      params: {
         id: 'string',
         pid: 'string',
         opt: {
            name: 'string'
         }
      },
    },
  },
  actions: {
    generateEvent: {
      handler() {
        // get the model from the factory encapsulated in the service and 
        // preconfigured with the broker
        const testEvent = this.getEventModel('MyServiceEvent');
        const payload = {
          id: 'foo',
          pid: 'bar',
          opt: {
            name: 'baz'
          }
        };
        const event = testEvent.create(payload);
        event.emit();
      }
    }
  },
};

Thoughts?

intech commented 4 years ago

In addition to outputting errors to the console, it would be nice to have a handler that can be declared and caught in each service at the hook or middleware level.

erfanium commented 4 years ago

@ccampanale Good idea, but i think this implementation is inconsistent with the usual Moleculer service structure I think it's better to save events model in a separate file (like mongoose models), and when we want to use it, simply import it and use it with this.emit or this.broadcast methods like this:

MyService/eventsModel.js

const { EventModel } = require("moleculer");

exports.myEvent = new EventModel("my.event", {
   id: "string",
   pid: "string",
   opt: {
      name: "string"
   }
});

MyService/myService.js

const { myEvent } = require("./eventsModel");

module.exports = {
   name: "myService",
   actions: {
      myAction(ctx) {
         // code...
         ctx.emit(myEvent, {
            id: "5",
            pid: 6, // bad type
            opt: { name: "erf" }
         });  // It throws an error duo payload

         // or 

         ctx.broadcast(myEvent, {
            id: "5",
            pid: 6,
            opt: { name: "erf" }
         }); // like emit behavior
      }
   }
};

Edit: Also we can use it as event parameter validator in listeners

const { myEvent } = require("./eventsModel");

module.exports = {
   name: "anotherService",
   events: {
      'my.event': {
         event: myEvent,  // I think using 'params' key can be confusing
         handler(payload){
            // ...somecode
         }

      }
   }
};

Edit 2 is this way (using the event model), defining Event Name is waste, cause we can use that Event Model like this:

const { myEvent } = require("./eventsModel");

module.exports = {
   name: "anotherService",
   events: [{
      event: myEvent,
      handler(payload) {
         // ...somecode
      }
   }]
};

any comments?

icebob commented 4 years ago

Nice suggestions, but at first, I will add params validation to the events on the listener side. It can be done with a few code lines. After that, we can discuss event models which one fits best Moleculer concepts.

@intech In 0.14 the event handlers errors are passed to the errorHandler() broker options, so you can catch them. On the other hand, I'm planning to implement hooks in events like action hooks. But after the stable 0.14 because it is the highest priority task.

ccampanale commented 4 years ago

@erfanium
Thanks! I do agree regarding the inconsistency with the rest of the framework and absolutely think that more thought needs to go into this. That said, the examples you provided would be breaking changes to the broker .emit() and .broadcast() interfaces (unless I'm missing something) which would be nice to avoid. Additionally, I would argue that the broker is not emitting/broadcasting the model itself but rather the payload created by the model post-validation. I'm sure there is a middle-ground here though where the broker can stay the same and this new EventModel would not feel terribly out of place.

If we can define events in separate files (something I'm 100% on-board for) and not on the service schema but in a way that still allows for the association of that event to a specific service, I believe this would be just fine. We also have to consider the situation where multiple service are defined in a single codebase and ran using single broker. So long as the event can be associated to only the service(s) which may emit/broadcast the event the solution should be sound. It's possible the solution here is to do what my team is doing and ultimately provide the model's schema on the service metadata object manually but core support - especially from a REPL perspective - would be quite nice. (As I stated previously, viewing event listeners from REPL perspective is informative from an operational perspective but leaves a lot to be desired from a development perspective.)

I really like your example of using the same event model on the consumer side for validation of event payloads for listeners! While I'm all for a "separation of concerns" (event consumption vs. production) I feel like this approach would offer a lot of flexibility; ultimately, these event models are being created for the purpose of payload validation regardless of whether the event is being consumed or produced.

@icebob That makes perfect sense. However, consider that both problems may be more alike than they appear on the surface. Whether events are coming or going, they are still "events". In Moleculer these are simply payloads of plain old JavaScript objects but what better way to validate "any" object than with a model of sorts? Given @erfanium's suggestion, it should be possible to address both of these problems with the same solution. Of course, support for how these may be published throughout the system - an underlying part of my personal suggestion - could come as an afterthought to the EventModel.

In any event (lol), I like the momentum here! :)

erfanium commented 4 years ago

@ccampanale thanks for your opinion. I may not understand exacly what you mean. However... in the current Moleculer service structure, All services are free to listen/emit any events (of curse in listener side you can see event service source). so if you have suggestions for this structure, it should be discussed in another issue.

of course, in EventModel we can select that which services can emit that event, but EventModel is just an optional module and it doesn't create real limits.

new EventModel({
  name: 'myEvent',
  params: {
    id: 'string',
    pid: 'string',
    opt: {
      name: 'string',
    },
  },
  pubServices: ['myService'],
});

the best solution is event channels, I relay like it, but it needs big breaking changes.

...the examples you provided would be breaking changes to the broker .emit() and .broadcast() interfaces.

No, it isn't a breaking change, we can check the type of the first argument of broker.emit() or broker.broadcast() , if it was String, so it's simple event emit (that exists now) or if it was an instance of EventModel, so it's event model.

@icebob I can help you with coding if you need

ccampanale commented 4 years ago

No, I think we're on the same page. Also, I'm not aiming for creating limits exactly. When I stated that "the event can be associated to only the service(s) which may emit/broadcast the event" I only mean that the ability to associate the event to a service should be prescriptive. Furthermore, the reason for the association is not about limiting which service can use the event model to create or filter events of that type but for information purposes as I feel it would be quite useful to see which services can emit particular events from the REPL console.

The pubServices option on an EventModel feels out of place to me personally. I would argue that it's not the responsibility of the model to track which services publish events of that type. Rather, optionally providing EventModel objects on a service schema in such a way that informs the service that the model either represents a class of event payloads which may be emitted/broadcasted or acts as a filter (anticorruption layer) for event listeners seems much more appropriate. Let the service decide what to do with the model and that information as it would ultimately end up needing to find it's way into the registry for published events.

No, it isn't a breaking change, we can check the type of the first argument of broker.emit() or broker.broadcast() , if it was String, so it's simple event emit (that exists now) or if it was an instance of EventModel, so it's event model.

You're absolutely correct! I completely forgot about the eventName param as the first argument for some reason. 🤦‍♂

erfanium commented 4 years ago

I updated this: https://github.com/moleculerjs/moleculer/issues/621#issuecomment-558799763

@ccampanale I think I understood what you mean You want to know which services can emit or broadcast particular events. it's ok? We can define them in service schema, like this:

const { myEvent } = require("./EventsModel");

module.exports = {
   name: "myService",
   publishedEvents: [myEvent],
   actions: {
      myAction(ctx) {
         // code...
         ctx.emit(myEvent, {
            id: "5",
            pid: 6,
            opt: { name: "erf" }
         });
      }
   }
};

so you can see which events are listened or published by service and (B) only services can emit a particular event that they have added that eventModel to their publishedEvents array/set (/B)

also, we can add an optional key to EventModel called 'restrict', to enable/disable (B) behavior

ccampanale commented 4 years ago

@erfanium Precisely! This is essentially what I'm building locally but the published events data ends up in the service metadata. Being built into the Moleculer Core would be much cleaner IMO.

Additionally, as you've pointed out, using these same event models for event payload validation (an anti-corruption layer) would be a win-win.

Also, while I don't personally need to restrict the event by service I think this could be a useful feature especially in a shared library or monolithic architectures.

ujwal-setlur commented 4 years ago

@ccampanale any plans on open-sourcing your code into a library?

icebob commented 4 years ago

Released: https://github.com/moleculerjs/moleculer/releases/tag/v0.14.0-rc1

ccampanale commented 4 years ago

@ujwal-setlur The code I was working on was very alpha and I have decided to take things in a different direction. Reason one is that there will be core support for this idea in Moleculer (this issue) which, given the small size of my team, would be best to adopt rather than to reinvent. Additionally - reason two - since the platform my team supports is primarily in AWS I've written a package with services that leverage the AWS EventBridge and Schema Registry for processing events, inferring schemas from them, and exposing the schemas for developers through our API and broker REPL commands. I would love to open source the later in the future but it's a very opinionated component in our architecture and build on our internal microservice chassis for deploying Moleculer services so I would need to "generalize" the code base (not impossible, just takes time). :P