Skip to content
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

RFC: Remove payload from Message #70

Closed
enumag opened this Issue Aug 6, 2018 · 35 comments

Comments

Projects
None yet
6 participants
@enumag
Copy link
Contributor

enumag commented Aug 6, 2018

As requested on Gitter, I'm creating an issue with a BC breaking change I'd like to suggest for prooph v8.

In my setup all messages are simple value objects with properties and methods. They don't hold an array of values internally, nor they are able to convert themselves to an array. Such responsibilities belong to MessageConverter::convertToArray() and MessageFactory::createMessageFromArray().

Because of that I'd like to remove these methods in Prooph v8:

  • Message::payload()
  • DomainMessage::setPayload()
  • DomainMessage::fromArray()
  • DomainMessage::toArray()

fromArray and toArray don't need any replacement in my opinion - MessageConverter and MessageFactory have that covered.

payload and setPayload should go into separate interface named PayloadMessage or MessageWithPayload. Default implementations of MessageConverter and MessageFactory would trow an exception for messages not implementing PayloadMessage - the user needs to implement their own array conversion logic for no-payload messages.

I've already made the necessary changes in PDO event store: prooph/pdo-event-store#153

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 6, 2018

Mh... MessageConverter uses toArray, see https://github.com/prooph/common/blob/master/src/Messaging/NoOpMessageConverter.php#L29. There are lots of affected places, and hence this change would be not so small as it might seem.

The MessageFactory used the fromArray, too, see https://github.com/prooph/common/blob/master/src/Messaging/FQCNMessageFactory.php#L51

@enumag How can a default message converter and factory look like, if there is no payload method and no toArray/fromArray can get called? I don't want to force everyone to implement this by themselves.

@codeliner what are your thoughts? is it worth it?

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 6, 2018

Something like this I think. Those who are using payload would not be forced to implement anything. Just their messages would need to implement the new PayloadMessage interface but they already have those methods implemented.

final class NoOpMessageConverter implements MessageConverter
{
    public function convertToArray(Message $message): array
    {
        Assertion::isInstanceOf($message, DomainMessage::class);
        Assertion::isInstanceOf($message, PayloadMessage::class);

        return [
            'message_name' => $message->messageName(),
            'uuid' => $message->uuid()->toString(),
            'payload' => $message->payload(),
            'metadata' => $message->metadata(),
            'created_at' => $message->createdAt(),
        ];
    }
}

class FQCNMessageFactory implements MessageFactory
{
    public function createMessageFromArray(string $messageName, array $messageData): Message
    {
        if (! class_exists($messageName)) {
            throw new \UnexpectedValueException('Given message name is not a valid class: ' . (string) $messageName);
        }
        if (! is_subclass_of($messageName, DomainMessage::class)) {
            throw new \UnexpectedValueException(sprintf(
                'Message class %s is not a sub class of %s',
                $messageName,
                DomainMessage::class
            ));
        }
        if (! is_subclass_of($messageName, PayloadMessage::class)) {
            throw new \UnexpectedValueException(sprintf(
                'Message class %s is not a sub class of %s',
                $messageName,
                PayloadMessage::class
            ));
        }

        $message = $messageName::fromData(
            $messageData['message_name'] ?? $messageName,
            isset($messageData['uuid']) ? Uuid::fromString($messageData['uuid']) : Uuid::uuid4(),
            $messageData['created_at'] ?? new DateTimeImmutable('now', new DateTimeZone('UTC')),
        );

        if (isset($messageData['metadata'])) {
            $message = $message->withMetadata($messageData['metadata']);
        }

        $message->setPayload($messageData['payload']);

        return $message;
    }
}

abstract class DomainMessage implements Message
{
    // ...

    public static function fromData(string $messageName, UuidInterdace $uuid, \DateTimeImmutable $createdAt): DomainMessage
    {
        $messageRef = new \ReflectionClass(get_called_class());
        /** @var $message DomainMessage */
        $message = $messageRef->newInstanceWithoutConstructor();

        $message->uuid = $uuid;
        $message->messageName = $messageName;
        $message->createdAt = $messageData['created_at'];

        return $message;
    }
}
@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 6, 2018

I have a different idea that should work for me and is a less of a BC break:

  • separate the properties and some of the methods (eg the withMetadata) from DomainMessage to a new trait
  • move payload() from Message to DomainMessage

I still think that DomainMessaage fromArray/toArray should be deprecated in favor of MessageConverter and MessageFactory but with this idea it's pretty much a separate issue which can be considered independently.

@codeliner

This comment has been minimized.

Copy link
Member

codeliner commented Aug 7, 2018

I'm a big payload fan. It's like the body of a http request.

But I do agree that it should be the task of MessageConverter and MessageFactory to convert from/to array including message payload.

The thing is, without payload the message interface feels incomplete. Again, it's like removing ServerRequestInterface::getBody() from PSR-7
so we decided to keep payload in the interface and encourage people to get used to it ;)

It became a common approach to create value objects insight message getter methods like shown in prooph-do: https://github.com/prooph/proophessor-do/blob/master/src/Model/Todo/Command/PostTodo.php#L38

This approach works really really well. And if you ask me everybody should use it. But again, your request is valid and one can ask why MessageConverter and MessageFactory exist.

The simple answer is: Because the two work with the entire message object and covert it. Payload is just one property and the return type of its getter method is an array.

The conflict is, that there is no way to inject a custom payload serializer into a message other than invoking a static method of the serializer in Message::payload(). And that's definitely against OOP best practice so I cannot suggest doing this instead. 🤔

move payload() from Message to DomainMessage

Not my favorite. We use Message interface with custom implementations in some projects. So we would recognize the missing payload method.

I'm really not sure if we should do that. On the one hand it would make the concept of MessageConverter/Factory more complete but on the other hand it could cause a lot of trouble in existing projects relying on that interface.

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 8, 2018

It became a common approach to create value objects insight message getter methods like shown in prooph-do
This approach works really really well. And if you ask me everybody should use it.

I have to disagree here. We tried that at first but to say it didn't work very well would be an understatement.

The first problem is the assertions in setPayload. Since some commands are directly connected to the API we can't use these assertions because we would only get the first error instead of all of them. I really don't want to duplicate the validation somewhere else. Prooph-based application needs a lot of writing already so I need to simplify as many things as possible.

The second problem is that the getter methods are a real pain to write. With normal objects, IDE can just generate the getters for you but it can't generate these getters. When some form (and by extension the command connected to it) has a few dozen fields, writing all the getters gets really painful really fast.

Third problem is null values handling. It's very easy to forget that some value is nullable when writing the getters. The getter would need to look like for example return isset($this->payload['until_date']) ? new DateTimeImmutable($this->payload['until_date']) : null;. If you forget the ternary then in the better case you get a fatal error in production and in the worse case the problem is silently ignored giving you some random data instead (such as current date instead of null). After fixing about 10 problems like that every week I was forced to admit that the payload approach simply doesn't work.

So yeah, I had a very bad experience with payload. I'd like to hear how you approach it to get around these issues but even if you somehow have an ultimate solution I'm very unlikely to switch back to payloads. Right now payloads feel like a newbie trap to me and I could not recommend it even to my enemy. These problems should be addressed in the docs in my opinion.


Anyway all of that is a bit off topic. I understand your viewpoint so I'd like to suggest a compromise - remove DomainMessage::fromArray/toArray in favor of Converter/Factory, but keep payload as is. The default implementations would use the payload of course but custom implementations might not. At the same time payload and setPayload methods should not be used anywhere in prooph libraries other than the default Converter/Factory.

@codeliner

This comment has been minimized.

Copy link
Member

codeliner commented Aug 8, 2018

thx for the detailed pain report :D It's interesting. I had very bad experiences with serializers in the past and therefor try to avoid them. Much simpler to map from/to value objects directly. But ok, it does not work for you.

The compromise is not bad. Let's see if we can do that. And I'll also think about payload again ...

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 8, 2018

Should I prepare a PR for the compromise suggestion?

I'd also like to hear from you how you get around the problems I mentioned about payload. Either you solved them somehow, never ran into them or don't find them as painful.

@fritz-gerneth

This comment has been minimized.

Copy link

fritz-gerneth commented Aug 9, 2018

I aggree with @enumag on this topic for the same reasons. In my opinion the current array-notion of a message is a storage detail (or bus detail) which a message should not be aware of itself. For us this is mostly tedious copy and paste with some quick unit tests to make sure all possible combinations work (again mostly copy and paste). Both tasks are error-prone though.

Truethfully I am not a big fan of keeping the payload method in the interface either. For any custom implementation this method would not be implementable, leaving a non-implemented / working method on the class (e.g. any automatic object serializer would not be able to be called from within this method). This maybe should go into the PayloadConstructable interface instead?

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 10, 2018

As per private discussion with @codeliner we (@prooph/components-core) would like to propose this:

a) Remove payload() method from Message Interface
b) Add another PayloadMessage Interface that extends Message Interface and has additional payload() method
c) Leave payload() method on DomainMessage implementation and implement PayloadMessage

That means, if you don't want to have the payload() method, you have to create your own base classes and implement your own message factory / converter classes.

What do you think?

@prolic prolic added the BC break label Aug 10, 2018

@prolic prolic changed the title BC break request - remove payload from Message RFC: Remove payload from Message Aug 10, 2018

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 10, 2018

@prolic Works for me but I'd like to also have a class/trait with the non-payload related properties and methods from DomainMessage (such as metadata). I can implement it on my side of course if you don't want it, I just thought it might help other people too.

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 10, 2018

@codeliner By the way as for serializers I do have to admit that our Converter/Factory are using some PHP reflection magic which is not very pretty. However dispite the code being a bit of a black magic, we never had any problem with these classes.

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 10, 2018

@enumag wanna provide a PR?

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 10, 2018

I'll be on vacation for a week now. If there is no PR when I return, I'll provide one.

@fritz-gerneth

This comment has been minimized.

Copy link

fritz-gerneth commented Aug 10, 2018

Should this PayloadMessage interface maybe merged with PayloadConstructable?

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 10, 2018

I was thinking about this today again and suddenly I had two insights. It's not yet worked out really, however I want to share it here, maybe you guys have some thoughts about it as well.

  1. I looked at the message interface again and thought about what is really necessary. And actually nothing is. For example the service bus can work with everything, it does not have to be an instance of message. So why not split up the interface completely? MessageWithName, MessageWithMetadata, UniqueMessage (uuid), PayloadMessage, MessageWithTimestamp, ...

This would allow for maximum flexibility, plugins can do type checks for specific features they are interested in (or none at all). Again, just an idea, not really thought through.

  1. I was thinking about the payload itself and the message converter again. The converter gives an array from an object. Ok, but how can an array be sent over wire? Not really. That's why we json encode it afterwards (see every single code that handles that, f.e. message producers or event store implementation). We are never really interested in an array, but in a string (or bytes to be more precise). The string could also be xml or some bytes of google protobuf. So actually the converter should return bytes and the factory create an object from bytes. This way we push json encode, xml creation, protobuf generation, whatever to the message converter. Nice thing about google protobuf support: very compact message for transfer over wire (if you need an example, check event store client).
@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 11, 2018

In case we would split up message interface completely, see: https://github.com/prooph/event-sourcing/blob/develop/src/Aggregate/AggregateRootTranslator.php#L49-L53. We would need a way to do this (maybe typehint on MessageWithMetadata).

@fritz-gerneth

This comment has been minimized.

Copy link

fritz-gerneth commented Aug 13, 2018

On 1: Interface composition only:
Not sure if this might be overkill. But I certainly like the idea. Different parts of an application sre only interested in certain aspects on an object. Creating dedicated interfaces is also a good way to hide other aspects of an object.
Interfaces I would see here are

  1. MessageName
  2. MessageMetadata
  3. UniqueMessage
  4. MessagePayload
  5. MessageDate
  6. MessageType (?, should maybe only be an interface instead, see below)

There is a problem with this granular approach though: you cannot type-hint to a mixture of those interfaces without introducing helper-interfaces or checking for multiple interfaces within the method. Former looses type-hinting and is tedious to write, first defeats the purpose of this separation in the first place and might not always be possible / desireable.

Also, I would suggest to remove the DomainMessage from the inheritance hierachy for most parts. There are simply some Commands / Events / Queriers that are not part of my domain but only of technical nature (is the DomainEvent actually required?):

MessageName ------\        / --- Command
MessageDate ------|--------| --- Query 
MessageType ------|        \ --- Event ---- DomainEvent -+-- AggregateChanged
UniqueMessage ----/                                      |
                                                         |
MessageMetadata -----------------------------------------/ 
MessagePayload

Thinking of different use cases:

  • (Any) bus does only care for the MessageName
    • Any other message type can also be dispatched
  • Event-Store requires AggregateChanged
  • Other message types can omit apsects not relevant for them
@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 13, 2018

@fritz-gerneth only a small correction regarding this:

Event-Store requires AggregateChanged

This is not true for what we are doing here. The new prooph/common lib will be used by the new major release of prooph/event-sourcing (and prooph/micro). prooph/event-store v7 is not affected at all. prooph/event-store v8 is in the pipeline and will be a 100% rewrite.

In case you're wondering:

@fritz-gerneth

This comment has been minimized.

Copy link

fritz-gerneth commented Aug 13, 2018

With that in mind, a second thought would be to completely remove the MessageType interface + metadata + payload interfaces are not relevant either:

MessageName ------\        / --- Command
MessageDate ------|--------| --- Query 
UniqueMessage ----/        \ --- Event
                      
MessageMetadata
MessagePayload

which would bring up the next question: would be need those Command / Query / Event interfaces at all? As you already suggested those are only relevant for the de/serializing messages. Take the v8 event store as an example: does it really care how the EventData is created? Should it not be the job of a message converter/factory to then create any event and cast it to an EventData object (and vice versa)? As long as the message converter can convert the event, the store does not care at all about whichever interfaces the event is implementing, right? That would suggest to remove any interface requirements and push the responsibility completely to the message converters. Of course a default implementation of Events and converts still might provide them for ease of use.

This would of course lead down a trecherous path where anything can be an event / command / query (as long as a converter can convert it). The bus probably would at least need the MessageName to dispatch it accordingly.

Part of this would suggest to merge & rename the message converter / factory to a single EventDataHydrator or alike (e.g. to Any input <-> EventData).

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 15, 2018

ping @codeliner @Ocramius - your thoughts?

@Ocramius

This comment has been minimized.

Copy link

Ocramius commented Aug 15, 2018

I didn't read the entire thread, nor will get to it: if this is about using an external serialiser that can work with @var fields and reflection, please go ahead :-)

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 15, 2018

@Ocramius just read this comment: #70 (comment)

@Ocramius

This comment has been minimized.

Copy link

Ocramius commented Aug 15, 2018

I looked at the message interface again and thought about what is really necessary. And actually nothing is. For example the service bus can work with everything, it does not have to be an instance of message. So why not split up the interface completely? MessageWithName, MessageWithMetadata, UniqueMessage (uuid), PayloadMessage, MessageWithTimestamp, ...

Could really be one interface aggregating the others. I don't think there is a strong advantage here, unless there is a practical use-case for messages that implement one interface but explicitly NOT an implementation of another.

I was thinking about the payload itself and the message converter again. The converter gives an array from an object. Ok, but how can an array be sent over wire? Not really. That's why we json encode it afterwards (see every single code that handles that, f.e. message producers or event store implementation). We are never really interested in an array, but in a string (or bytes to be more precise). The string could also be xml or some bytes of google protobuf. So actually the converter should return bytes and the factory create an object from bytes. This way we push json encode, xml creation, protobuf generation, whatever to the message converter. Nice thing about google protobuf support: very compact message for transfer over wire (if you need an example, check event store client).

My tip is getting rid of all this manual stuff: be extremely aggressive with reflection and just estract the heck of objects, recursively. Making people work with strings is possibly even worse, IMO: less control to be given to consumers here with out-of-the-box defaults. Yes, if people want protobufs, add an interface to do something like class MyMessage implements YesIKnowWhatIAmDoingPleaseLetMeSerializeStuffOnMyOwn.

@oqq

This comment has been minimized.

Copy link
Member

oqq commented Aug 17, 2018

How about to get rid of any interface for messages?

They are currently part of any command, event and query I have build in my projects. But to be honest, I hate this inheritance from any third party abstract class like "DomainMessage" or any interface implementation.

Commands are mostly independent when no message broker is in use. Also queries don't need any interface at all.

An event has to be storable. So all what we need is something like an EventSerializer or a chain of serializer, which knows how to serialize an event object.

By the way.. Many prooph components require implementation of the Message interfaces without testing the type of the objects at all.

@fritz-gerneth

This comment has been minimized.

Copy link

fritz-gerneth commented Aug 17, 2018

An event has to be storable.

Not every event has to be storable.

But ultimatively this is what @prolic suggested. Only certain use cases require some interfaces (or would make it easier). But the entire hierachy, up to DomainMessage would only provide a easy to use default implementation.

@oqq

This comment has been minimized.

Copy link
Member

oqq commented Aug 17, 2018

Not every event has to be storable.
Of course. I thought of events for 'stored state'. But there are also events which are not related to this issue.

So what we need is an interface for something like a "MessageReader", which could provide necessary values required from some prooph components for a message.

I would also like it to be more strict in prooph components. So if any method requires a Message as argument, than it should get one. e.g the EventStore should always get an DomainEvent object. If a user would implement such a component, but has only messages without any implementation detail of current interfaces / abstract classes, than he has to use a MessageConverter.

interface DomainEventConverter 
{
    /** @var mixed $event **/
    public function convertEvent($event): DomainEvent;
}

And of course, we could provide some default implementations. Something like fancy Hydrators. (just kidding ;)

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 19, 2018

@prolic To be honest I don't see much value in spliting the interface completely. It would complicate a lot of code where you could no longer use a Message typehint but would have to use instanceof multiple times to throw an exception if one of the interfaces needed in that context is missing. The name, date, type and uuid are a necessity for every message. Or do you have a use case where it would be good to NOT implement one of these interfaces? The only things that could be separated are the metadata and payload in my opinion.

I'm not sure about your idea about converting message <-> json instead of message <-> array <-> json. Even if the converter would return string, it would internally still use the array and most likely depend on two other classes to do the message <-> array and array <-> json conversions. Hiding the array from view sounds good to me at first glance but I wonder if it won't cause problems later on. One thing that comes to mind is that when you change an event class and need to update all such events in event store accordingly then it might be better to work with arrays for the migration.

Next can you explain that EventData class from prooph v8? I'm completely confused about the isJson property.

Looks like @Ocramius is suggesting exactly what I'm doing - serialization using reflection and @var annotations. Personally I'd refrain from going that way in prooph until PHP has typed properties (@var annotations are a pain to deal with) but I if you want to go that way, I can help for sure.

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 19, 2018

@enumag In EventData the isJson property tell, that the data is a json encoded string. So whenever isJson is set to true, you can expect data to be in json format, if it's false, data can be a simple string, xml, protobuf, whatever

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 19, 2018

@prolic Shouldn't it be a $format property then? Indicating what format the data is with json being one possible value? Doesn't make sense to have a special boolean property only for json.

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 19, 2018

If you need to know the format yourself, put it into metadata. Json is special for the event store, because it can handle it within the projections.

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 20, 2018

Ok, I understand in that case.

Have you decided how you want to resolve this issue? Or do you need more feedback about some aspects?

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 20, 2018

@enumag I had some discussion with @codeliner and we are going to make an announcement soon, you'll probably be surprised :)

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Aug 20, 2018

Is it an announcement just regarding this issue or something bigger? If it is something bigger, where should I await the announcement? :-)

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 20, 2018

It's bigger - will be published first in chat.

@prolic

This comment has been minimized.

Copy link
Member

prolic commented Aug 20, 2018

This library will receive support until December 31, 2019 and will then be deprecated.

For further information see the official announcement here: https://www.sasaprolic.com/2018/08/the-future-of-prooph-components.html

@prolic prolic closed this Aug 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.