akka / alpakka

Alpakka is a Reactive Enterprise Integration library for Java and Scala, based on Reactive Streams and Akka.
https://doc.akka.io/docs/alpakka/current/
Other
1.26k stars 645 forks source link

Google Cloud PubSub PubSubMessage.messageId should be optional #1786

Closed LGLO closed 5 years ago

LGLO commented 5 years ago

As per REST API reference: https://cloud.google.com/pubsub/docs/reference/rest/v1/PubsubMessage messageId property should not be set when publishing.

I understand it's a breaking change.

I think I can provide fix that will make PubSubMessage.messageId optional or introduce separate case class for publishing that doesn't have messageId and publishTime fields.

2m commented 5 years ago

In the generated code of the Alpakka Google Cloud Pub/Sub gRPC connector the logic is to check if the messageId string is not empty:

val __v = messageId
if (__v != "") {
  _output__.writeString(3, __v)
}

We could go with the same approach, where JsonWriter would not write out messageId if it is empty.

And then for nicer user API, we could add PubSubMessage.apply/create methods that do not take messageId or publishTime.

LGLO commented 5 years ago

According to API specs, "messageId" should never be present when publishing. PubSub could start rejecting messages, although it would go against robustness principle.

Maybe we can make this two step? In the first step I'll add functions to create a PubSubMessage without messageId and publishTime and remove these fields from JSON when publishing.

Then we could change messageId to Option[String] when some breaking changes are going in anyway? WDYT?

2m commented 5 years ago

Two step process sounds good. Then we can release non-breaking changes sooner, and API breaking changes will have to wait next major release.

LGLO commented 5 years ago

I've opened https://github.com/akka/alpakka/pull/1795 could you take a look? On Monday I'll re-learn MiMa and check if it breaks binary compatibility or not.

2m commented 5 years ago

Change looks good to me and MiMa.

When you will be tackling step 2, it could be done together with https://github.com/akka/alpakka/issues/1797 since it will break binary API as well.

LGLO commented 5 years ago

Thank you! I've updated @deprecated with 1.1.0.

ennru commented 5 years ago

Fixed with #1795