-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix: Do not special handle protobuf payloads for replication #1212
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,6 @@ package akka.projection.grpc.internal | |
import scala.collection.concurrent.TrieMap | ||
import scala.collection.immutable | ||
import scala.util.Try | ||
|
||
import akka.actor.typed.ActorSystem | ||
import akka.actor.typed.scaladsl.LoggerOps | ||
import akka.annotation.InternalApi | ||
|
@@ -23,6 +22,7 @@ import com.google.protobuf.Descriptors | |
import com.google.protobuf.GeneratedMessageV3 | ||
import com.google.protobuf.Message | ||
import com.google.protobuf.Parser | ||
import com.google.protobuf.UnsafeByteOperations | ||
import com.google.protobuf.any.{ Any => ScalaPbAny } | ||
import com.google.protobuf.{ Any => JavaPbAny } | ||
import com.google.protobuf.{ Any => PbAny } | ||
|
@@ -99,15 +99,45 @@ import scalapb.options.Scalapb | |
} | ||
} | ||
|
||
def akkaSerializationTypeUrl(serializerId: Int, manifest: String): String = { | ||
if (manifest.isEmpty) s"$AkkaSerializationTypeUrlPrefix$serializerId" | ||
else | ||
s"$AkkaSerializationTypeUrlPrefix$serializerId$AkkaTypeUrlManifestSeparator$manifest" | ||
} | ||
|
||
def akkaSerializerIdAndManifestFromTypeUrl(typeUrl: String): (Int, String) = { | ||
val idAndManifest = | ||
typeUrl.substring(AkkaSerializationTypeUrlPrefix.length) | ||
val i = idAndManifest.indexOf(AkkaTypeUrlManifestSeparator) | ||
if (i == -1) | ||
idAndManifest.toInt -> "" | ||
else | ||
idAndManifest.substring(0, i).toInt -> idAndManifest.substring(i + 1) | ||
} | ||
|
||
} | ||
|
||
/** | ||
* INTERNAL API | ||
*/ | ||
@InternalApi private[akka] class ProtoAnySerialization( | ||
@InternalApi | ||
private[akka] trait AkkaProjectionGrpcSerialization { | ||
def serialize(event: Any): ScalaPbAny | ||
def deserialize(scalaPbAny: ScalaPbAny): Any | ||
def toSerializedEvent(scalaPbAny: ScalaPbAny): Option[SerializedEvent] | ||
} | ||
|
||
/** | ||
* Primarily intended for direct usage of grpc projections, where there may be a public (protobuf) protocol published | ||
* by the producing service. | ||
* | ||
* INTERNAL API | ||
*/ | ||
@InternalApi private[akka] final class ProtoAnySerialization( | ||
system: ActorSystem[_], | ||
descriptors: immutable.Seq[Descriptors.FileDescriptor], | ||
prefer: ProtoAnySerialization.Prefer) { | ||
prefer: ProtoAnySerialization.Prefer) | ||
extends AkkaProjectionGrpcSerialization { | ||
import ProtoAnySerialization._ | ||
|
||
private val serialization = SerializationExtension(system.classicSystem) | ||
|
@@ -138,7 +168,7 @@ import scalapb.options.Scalapb | |
def this(system: ActorSystem[_]) = | ||
this(system, descriptors = Nil, ProtoAnySerialization.Prefer.Scala) | ||
|
||
def serialize(event: Any): ScalaPbAny = { | ||
override def serialize(event: Any): ScalaPbAny = { | ||
event match { | ||
case scalaPbAny: ScalaPbAny if scalaPbAny.typeUrl.startsWith(GoogleTypeUrlPrefix) => | ||
ScalaPbAny(ProtoAnyTypeUrl, scalaPbAny.toByteString) | ||
|
@@ -153,15 +183,15 @@ import scalapb.options.Scalapb | |
case other => | ||
// fallback to Akka serialization | ||
val otherAnyRef = other.asInstanceOf[AnyRef] | ||
val bytes = serialization.serialize(otherAnyRef).get | ||
val serializer = serialization.findSerializerFor(otherAnyRef) | ||
val bytes = serializer.toBinary(otherAnyRef) | ||
val manifest = Serializers.manifestFor(serializer, otherAnyRef) | ||
val id = serializer.identifier | ||
ScalaPbAny(akkaSerializationTypeUrl(id, manifest), ByteString.copyFrom(bytes)) | ||
ScalaPbAny(akkaSerializationTypeUrl(id, manifest), UnsafeByteOperations.unsafeWrap(bytes)) | ||
} | ||
} | ||
|
||
def deserialize(scalaPbAny: ScalaPbAny): Any = { | ||
override def deserialize(scalaPbAny: ScalaPbAny): Any = { | ||
val typeUrl = scalaPbAny.typeUrl | ||
if (typeUrl == ProtoAnyTypeUrl) { | ||
if (prefer == Prefer.Scala) | ||
|
@@ -172,6 +202,7 @@ import scalapb.options.Scalapb | |
decodeMessage(scalaPbAny) | ||
} else if (typeUrl.startsWith(AkkaSerializationTypeUrlPrefix)) { | ||
val (id, manifest) = akkaSerializerIdAndManifestFromTypeUrl(typeUrl) | ||
// FIXME could potentially optimize to use byte buffer here instead of copy to byte array | ||
serialization.deserialize(scalaPbAny.value.toByteArray, id, manifest).get | ||
} else if (prefer == Prefer.Scala) { | ||
// when custom typeUrl | ||
|
@@ -182,7 +213,7 @@ import scalapb.options.Scalapb | |
} | ||
} | ||
|
||
def toSerializedEvent(scalaPbAny: ScalaPbAny): Option[SerializedEvent] = { | ||
override def toSerializedEvent(scalaPbAny: ScalaPbAny): Option[SerializedEvent] = { | ||
// see corresponding typeUrl cases in `deserialize` | ||
|
||
val typeUrl = scalaPbAny.typeUrl | ||
|
@@ -224,22 +255,6 @@ import scalapb.options.Scalapb | |
} | ||
} | ||
|
||
private def akkaSerializationTypeUrl(serializerId: Int, manifest: String): String = { | ||
if (manifest.isEmpty) s"$AkkaSerializationTypeUrlPrefix$serializerId" | ||
else | ||
s"$AkkaSerializationTypeUrlPrefix$serializerId$AkkaTypeUrlManifestSeparator$manifest" | ||
} | ||
|
||
private def akkaSerializerIdAndManifestFromTypeUrl(typeUrl: String): (Int, String) = { | ||
val idAndManifest = | ||
typeUrl.substring(AkkaSerializationTypeUrlPrefix.length) | ||
val i = idAndManifest.indexOf(AkkaTypeUrlManifestSeparator) | ||
if (i == -1) | ||
idAndManifest.toInt -> "" | ||
else | ||
idAndManifest.substring(0, i).toInt -> idAndManifest.substring(i + 1) | ||
} | ||
|
||
private def strippedFileName(fileName: String) = | ||
fileName.split(Array('/', '\\')).last.stripSuffix(".proto") | ||
|
||
|
@@ -451,3 +466,46 @@ import scalapb.options.Scalapb | |
} | ||
|
||
} | ||
|
||
/** | ||
* Primarily intended for replication where there is no public protocol | ||
* | ||
* INTERNAL API | ||
*/ | ||
@InternalApi | ||
private[akka] final class DelegateToAkkaSerialization(system: ActorSystem[_]) extends AkkaProjectionGrpcSerialization { | ||
import ProtoAnySerialization._ | ||
private val serialization = SerializationExtension(system.classicSystem) | ||
|
||
override def serialize(event: Any): ScalaPbAny = { | ||
val anyRefEvent = event.asInstanceOf[AnyRef] | ||
val serializer = serialization.findSerializerFor(anyRefEvent) | ||
val bytes = serializer.toBinary(anyRefEvent) | ||
val manifest = Serializers.manifestFor(serializer, anyRefEvent) | ||
val id = serializer.identifier | ||
ScalaPbAny(akkaSerializationTypeUrl(id, manifest), UnsafeByteOperations.unsafeWrap(bytes)) | ||
} | ||
|
||
override def deserialize(event: ScalaPbAny): Any = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked in detail at this PR, but could we keep deserialization side as it was? If we serialize with AkkaSerializationTypeUrlPrefix that would still be deserialized correctly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that if we can have no special casing that is better and less trickery to try to understand, all messages are fed to the configured akka serializer. The any-passthrough is such a weird special case that it is very unlikely that anybody else than us ourselves is doing that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
val typeUrl = event.typeUrl | ||
if (typeUrl.startsWith(AkkaSerializationTypeUrlPrefix)) { | ||
val (id, manifest) = akkaSerializerIdAndManifestFromTypeUrl(typeUrl) | ||
// FIXME could potentially optimize to use byte buffer here instead of copy to byte array | ||
serialization.deserialize(event.value.toByteArray, id, manifest).get | ||
} else { | ||
throw new IllegalArgumentException( | ||
s"Got event with type url: [${typeUrl}] but only type urls with Akka serializer prefix ($AkkaSerializationTypeUrlPrefix) supported") | ||
} | ||
} | ||
|
||
override def toSerializedEvent(event: ScalaPbAny): Option[SerializedEvent] = { | ||
val typeUrl = event.typeUrl | ||
if (typeUrl.startsWith(AkkaSerializationTypeUrlPrefix)) { | ||
val (id, manifest) = akkaSerializerIdAndManifestFromTypeUrl(typeUrl) | ||
Some(new SerializedEvent(event.value.toByteArray, id, manifest)) | ||
} else { | ||
// We don't try to optimize this case. One level of indirection too much, and probably not a common case. | ||
None | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This apply is the same as the other apply above, aside from the serialization. Where is the other used? Do we need both? Is it even so that we can look at
replicationSettings.isDefined
to select serializer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified to a single internal apply factory now.