search

msemys / esjc

EventStore Java Client
MIT License
108 stars 27 forks source link

esjc needs to tolerate empty EventRecords #25

Closed Yspadadden closed 7 years ago

Yspadadden commented 7 years ago

There is the possibility that the eventstore returns empty (invalid) EventRecords. See issue. According to @gregoryyoung, the client has to be able to handle this.

may give a different behaviour in the .net client where we recognize it is bad and throw it away

This might be solved by changing the fields of an EventRecord in the protobuf specs to be optional. I don't think the individual events can be ignored on protobuf level, as the code that causes the exception is autogenerated.

Here is an excerpt from the error that occurs client side:

java.util.concurrent.ExecutionException: com.google.protobuf.InvalidProtocolBufferException: Message missing required fields: events[123].event, events[125].event, events[132].event, events[134].event, events[142].event, events[144].event, events[149].event, events[150].event at java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:357) at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1895) at com.github.msemys.esjc.subscription.AllCatchUpSubscription.readEventsTill(AllCatchUpSubscription.java:36) at com.github.msemys.esjc.CatchUpSubscription.lambda$runSubscription$1(CatchUpSubscription.java:161) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745) Caused by: com.google.protobuf.InvalidProtocolBufferException: Message missing required fields: events[123].event, events[125].event, events[132].event, events[134].event, events[142].event, events[144].event, events[149].event, events[150].event at com.google.protobuf.UninitializedMessageException.asInvalidProtocolBufferException(UninitializedMessageException.java:81) at com.google.protobuf.AbstractParser.checkMessageInitialized(AbstractParser.java:71) at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:168)

Please find attached a demo java app that reproduces this error reliably. The data.zip must be loaded into the test eventstore first.

data.zip EventStoreDemo.zip

gregoryyoung commented 7 years ago

There should not need to be a protobuf change. The .net client seems to handle this using the same protobuf spec.

Yspadadden commented 7 years ago

As far as I can see, the code that throws the exception and is ultimately the problem is auto generated using the google protobuf toolkit. A quick look at the documentation did not reveal any options to disable the validation or auto remove invalid elements. Might be a difference in the tool that was used to generate the .net code.

Anyway, I also don't like to meddle with the protobuf definitions. Maybe @msemys has a better idea.

msemys commented 7 years ago

I did some debuging on what is: events[123], events[125], events[132], events[134], events[142], events[144], events[149], events[150] in ReadAllEventsForwardCompleted message.

It is link events, but without required event field:

123 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2604} "link {\n event_stream_id: "$streams"\n event_number: 1\n event_id: "Hf7\204\315b\234I\247`\036\216\277\3548\365"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@ab2b56fa-f46f-451b-ad08-08e33b796833"\n metadata: "{\"$v\":\"1:- 1:1:3\",\"$c\":43520,\"$p\":43520,\"$causedBy\":\"77a3d080-9c66-9d41-b0f0-798b4251949e\"}"\n created: 636305151797985030\n created_epoch: 1494918379798\n}\ncommit_position: 43940\nprepare_position: 43940\n"

125 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2606} "link {\n event_stream_id: "$et-TestDataObject"\n event_number: 1\n event_id: "I\002\333)R\234\252D\247c\003k\'\237tS"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@ab2b56fa-f46f-451b-ad08-08e33b796833"\n metadata: "{\"$v\":\"4:- 1:1:3\",\"$c\":43520,\"$p\":43520,\"$causedBy\":\"77a3d080-9c66-9d41-b0f0-798b4251949e\"}"\n created: 636305151798002150\n created_epoch: 1494918379800\n}\ncommit_position: 44314\nprepare_position: 44314\n"

132 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2613} "link {\n event_stream_id: "$et-$metadata"\n event_number: 0\n event_id: "\220c{=R\373cF\227\336\270\251\362\223\314"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@ab2b56fa-f46f-451b-ad08-08e33b796833"\n metadata: "{\"$v\":\"4:- 1:1:3\",\"$c\":45434,\"$p\":45434,\"$causedBy\":\"68c15897-d9e1-dd49-ab05-35c9410d39a8\"}"\n created: 636305151798464430\n created_epoch: 1494918379846\n}\ncommit_position: 45953\nprepare_position: 45953\n"

134 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2615} "link {\n event_stream_id: "$ce-ab2b56fa"\n event_number: 0\n event_id: "p\242\321~\373\005\"A\275\325\024\016\364\'\261\233"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@ab2b56fa-f46f-451b-ad08-08e33b796833"\n metadata: "{\"$v\":\"3:- 1:1:3\",\"$c\":43520,\"$p\":43520,\"$o\":\"ab2b56fa-f46f-451b-ad08-08e33b796833\",\"$causedBy\":\"77a3d080-9c66-9d41-b0f0-798b4251949e\"}"\n created: 636305151798627950\n created_epoch: 1494918379862\n}\ncommit_position: 46326\nprepare_position: 46326\n"

142 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2623} "link {\n event_stream_id: "$streams"\n event_number: 3\n event_id: "\330\363r\362\252\327 J\263\v\r$\254\261\377\'"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@2005e065-77e9-40b3-8e24-bbab436f7e31"\n metadata: "{\"$v\":\"1:- 1:1:3\",\"$c\":49378,\"$p\":49378,\"$causedBy\":\"adbd6f54-c50c-6f4c-9b2c-a880b1bc7bac\"}"\n created: 636305151803789190\n created_epoch: 1494918380378\n}\ncommit_position: 49798\nprepare_position: 49798\n"

144 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2625} "link {\n event_stream_id: "$et-TestDataObject"\n event_number: 3\n event_id: "\201\363\357>R7\023F\256N4EB\310nl"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@2005e065-77e9-40b3-8e24-bbab436f7e31"\n metadata: "{\"$v\":\"4:- 1:1:3\",\"$c\":49378,\"$p\":49378,\"$causedBy\":\"adbd6f54-c50c-6f4c-9b2c-a880b1bc7bac\"}"\n created: 636305151803790110\n created_epoch: 1494918380379\n}\ncommit_position: 50172\nprepare_position: 50172\n"

149 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2630} "link {\n event_stream_id: "$et-$metadata"\n event_number: 1\n event_id: "\300\324\214\237\002\003\354J\266\345|9\334\2073\321"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@2005e065-77e9-40b3-8e24-bbab436f7e31"\n metadata: "{\"$v\":\"4:- 1:1:3\",\"$c\":50406,\"$p\":50406,\"$causedBy\":\"4a25a018-523a-7642-b3e9-b811f0ae2528\"}"\n created: 636305151803833190\n created_epoch: 1494918380383\n}\ncommit_position: 51156\nprepare_position: 51156\n"

150 = {com.github.msemys.esjc.proto.EventStoreClientMessages$ResolvedEvent@2631} "link {\n event_stream_id: "$ce-2005e065"\n event_number: 0\n event_id: "\202\037>UN\315\215I\253n\253\217\267i\027W"\n event_type: "$>"\n data_content_type: 0\n metadata_content_type: 0\n data: "0@2005e065-77e9-40b3-8e24-bbab436f7e31"\n metadata: "{\"$v\":\"3:- 1:1:3\",\"$c\":49378,\"$p\":49378,\"$o\":\"2005e065-77e9-40b3-8e24-bbab436f7e31\",\"$causedBy\":\"adbd6f54-c50c-6f4c-9b2c-a880b1bc7bac\"}"\n created: 636305151803843210\n created_epoch: 1494918380384\n}\ncommit_position: 51385\nprepare_position: 51385\n"

Ignoring whole ReadAllEventsForwardCompleted message, that contains some invalid ResolvedEvent's would be too danger, because we will skip all the events in this batch (that contains valid events as well).

I think that the better option is to update the protobuf spec: https://github.com/msemys/esjc/blob/master/src/main/protobuf/EventStoreClientMessages.proto#L43 -> optional EventRecord event = 1;

More over, nullable 'event' field of ResolvedEvent is expected and allowed in both clients (escj and .net) :)

Konfuzzyus commented 7 years ago

I agree, the proto spec should define that field optional, considering how it is used.

gregoryyoung commented 7 years ago

Why does the .net client not have a problem?

On Wed, May 17, 2017 at 12:50 PM, Christian Meyer notifications@github.com wrote:

I agree, the proto spec should define that field optional, considering how it is used.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/msemys/esjc/issues/25#issuecomment-302066738, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXRWi7Rmt0KikpCeqiPm6zkKxdbD1Atks5r6t8jgaJpZM4NcR3N .

-- Studying for the Turing test

Konfuzzyus commented 7 years ago

If I understand the .net client correctly, https://github.com/EventStore/EventStore/blob/release-v4.0.2/src/EventStore.ClientAPI/Transport.Tcp/ProtobufExtensions.cs#L27 catches the exception and just substitutes a default value for the field that failed to deserialize.

gregoryyoung commented 7 years ago

Ah makes sense

On Wed, May 17, 2017 at 2:35 PM, Christian Meyer notifications@github.com wrote:

If I understand the .net client correctly, https://github.com/EventStore/ EventStore/blob/release-v4.0.2/src/EventStore.ClientAPI/Transport.Tcp/ ProtobufExtensions.cs#L27 catches the exception and just substitutes a default value for the field that failed to deserialize.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/msemys/esjc/issues/25#issuecomment-302091782, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXRWhK1Z3ET3Wn3ZaepQTOpEiVMsBafks5r6veJgaJpZM4NcR3N .

-- Studying for the Turing test

msemys commented 7 years ago

It is possible to do a bit similar behavior like in the .net client - catch InvalidProtocolBufferException here https://github.com/msemys/esjc/blob/master/src/main/java/com/github/msemys/esjc/operation/AbstractOperation.java#L72 and take unfinished message (successfully parsed but invalid) from exception - InvalidProtocolBufferException.getUnfinishedMessage().

...but particularly in this case I would prefere to update the protobuf spec, because server does not return event field and the client logic allows nullable field.