Skip to content

fix: accumulate protobuf descriptors #192

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.

Sign up for GitHub

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

Merged
ennru merged 2 commits into from
Aug 9, 2021
Merged

fix: accumulate protobuf descriptors #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4887c59
chore: try eventing shopping cart in CI
ennru Aug 4, 2021
c64711e
Accumulate protobuf descriptors instead of using them in isolation.
franciscolopezsancho Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ jobs:

samples-tests:
machine: true
description: "Maven Samples Integreation Tests"
description: "Maven Samples Integration Tests"
steps:
- checkout-and-merge-to-main
- setup_sbt
Expand Down Expand Up @@ -219,6 +219,13 @@ jobs:
cd samples/replicatedentity-counter
echo "Running mvn with SDK version: '$SDK_VERSION'"
mvn -Dakkaserverless-sdk.version=$SDK_VERSION verify -Pit
- run:
name: Run integration tests for the Eventing Shopping Cart sample
command: |
cd samples/java-eventing-shopping-cart
echo "Running mvn with SDK version: '$SDK_VERSION'"
# TODO: enable integration tests with -Pit
mvn -Dakkaserverless-sdk.version=$SDK_VERSION verify
- save_deps_cache

publish:
Expand Down
1 change: 1 addition & 0 deletions RELEASING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
* git push --tags
2. CircleCI will automatically publish to Sonatype OSSRH (Maven Central) based on the tag.
3. If all seems well, create a [new release](https://github.com/akkaserverless-java-sdk/releases/new) for the new version.
4. Update the `akkaserverless-sdk.version` in the `samples/*/pom.xml` files to the released version.
41 changes: 30 additions & 11 deletions codegen/core/src/main/scala/com/lightbend/akkasls/codegen/DescriptorSet.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,8 @@ object DescriptorSet {
@SuppressWarnings(Array("org.wartremover.warts.Throw"))
def fileDescriptors(
file: File
): Either[CannotOpen, Iterable[Either[ReadFailure, Descriptors.FileDescriptor]]] =
Using[FileInputStream, Either[CannotOpen, Iterable[
Either[ReadFailure, Descriptors.FileDescriptor]
]]](
): Either[CannotOpen, Either[ReadFailure, Iterable[Descriptors.FileDescriptor]]] =
Copy link
Copy Markdown
Contributor

@raboof raboof Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now if we fail to read one file we throw away all results? That might be OK if we're sure we need all of them anyway

Copy link
Copy Markdown
Member Author

@ennru ennru Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We bail out on any error now, before it could live with certain descriptors failing.

Using[FileInputStream, Either[CannotOpen, Either[ReadFailure, Iterable[Descriptors.FileDescriptor]]]](
new FileInputStream(file)
) { fis =>
val registry = ExtensionRegistry.newInstance()
Expand All @@ -64,15 +62,12 @@ object DescriptorSet {
val descriptorProtos =
DescriptorProtos.FileDescriptorSet.parseFrom(fis, registry).getFileList.asScala

for (descriptorProto <- descriptorProtos)
yield try Right(Descriptors.FileDescriptor.buildFrom(descriptorProto, Array.empty, true))
catch {
case e: Descriptors.DescriptorValidationException =>
Left(CannotValidate(e))
}
val empty: Either[ReadFailure, Iterable[Descriptors.FileDescriptor]] =
Right(Array[Descriptors.FileDescriptor]())
descriptorProtos.foldLeft(empty)((acc, file) => accumulatedBuildFrom(acc, file))
} catch {
case e: IOException =>
List(Left(CannotRead(e)))
Left(CannotRead(e))
})
} match {
case Success(result) => result
Expand All @@ -83,4 +78,28 @@ object DescriptorSet {
private val descriptorslogger = Logger.getLogger(classOf[Descriptors].getName)
descriptorslogger.setLevel(Level.OFF); // Silence protobuf

/**
* This method accumulates `FileDescriptor`s to provide
* all the necessary dependencies for each call to FileDescriptor.buildFrom.
* Otherwise placeholders (mocked references) get created instead and
* these can't function as proper dependencies. Chiefly as imports.
*
* see allowUnknownDependencies per https://github.com/protocolbuffers/protobuf/blob/ae26a81918fa9e16f64ac27b5a2fb2b110b7aa1b/java/core/src/main/java/com/google/protobuf/Descriptors.java#L286
**/
private def accumulatedBuildFrom(
reads: Either[ReadFailure, Iterable[Descriptors.FileDescriptor]],
file: DescriptorProtos.FileDescriptorProto
): Either[ReadFailure, Iterable[Descriptors.FileDescriptor]] = {
reads match {
case Left(_) => reads
case Right(fileDescriptors) =>
try {
Right(fileDescriptors ++ List(Descriptors.FileDescriptor.buildFrom(file, fileDescriptors.toArray, true)))
Copy link
Copy Markdown
Contributor

@raboof raboof Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have to pass 'depended-upon' descriptors when parsing 'depending' descriptors - meaning the files must be accumulated in the 'right order'. How do we make sure they are in the right order?

Copy link
Copy Markdown
Member Author

@ennru ennru Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure they have to be in the right order, but maybe we should allowUnknownDependencies=false instead?

Copy link
Copy Markdown
Contributor

@raboof raboof Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure they have to be in the right order

Whatever descriptor is the first one will be called with an empty fileDescriptors array, so if some of them need things to be available in there then that'd mean the order matters?

I'm not sure they have to be in the right order, but maybe we should allowUnknownDependencies=false instead?

I'm not sure, I guess you could have unknown dependencies (like external proto annotations that we don't care about) that we could allow?

Copy link
Copy Markdown
Member Author

@ennru ennru Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is what I expected, but the API doesn't seem to allow us to first accumulate the descriptors and after that trigger linking.

I assume allowUnknownDependencies=false would make it fail when the order is wrong, but now the placeholders will be linked once the correct descriptor has been added to the list.

} catch {
case e: Descriptors.DescriptorValidationException =>
Left(CannotValidate(e))
}
}
}

}
10 changes: 7 additions & 3 deletions codegen/core/src/test/scala/com/lightbend/akkasls/codegen/DescriptorSetSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,13 @@ class DescriptorSetSuite extends munit.FunSuite {
val testFilesPath = Paths.get(getClass.getClassLoader.getResource("test-files").toURI)
val descriptorFile = testFilesPath.resolve("descriptor-sets/hello-1.0-SNAPSHOT.protobin").toFile
val result = DescriptorSet
.fileDescriptors(descriptorFile)
.flatMap(x => x.head.map(_.getServices.get(0).getFullName))
assertEquals(result, Right("com.lightbend.MyServiceEntity"))
.fileDescriptors(descriptorFile) match {
case Right(fileExists) =>
fileExists match {
case Right(descriptors) => descriptors.map(_.getServices.get(0).getFullName)
}
}
assertEquals(result, List("com.lightbend.MyServiceEntity"))
}

test("failed to open") {
Expand Down
53 changes: 26 additions & 27 deletions maven-java/akkaserverless-maven-plugin/src/main/java/com/akkaserverless/GenerateMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,36 +91,35 @@ public void execute() throws MojoExecutionException {
File protobufDescriptor = descriptorSetOutputDirectory.toPath().resolve(descriptorSetFileName).toFile();
if (protobufDescriptor.exists()) {
log.info("Inspecting proto file descriptor for entity generation...");
Either<DescriptorSet.CannotOpen, Iterable<Either<DescriptorSet.ReadFailure, Descriptors.FileDescriptor>>> descriptors = DescriptorSet
Either<DescriptorSet.CannotOpen, Either<DescriptorSet.ReadFailure, Iterable<Descriptors.FileDescriptor>>> descriptors = DescriptorSet
.fileDescriptors(protobufDescriptor);
if (descriptors.isRight()) {
Iterable<FileDescriptor> fileDescriptors = descriptors.right().get().map(descriptor -> {
if (descriptor.isRight()) {
return descriptor.right().get();
} else {
Either<DescriptorSet.ReadFailure, Iterable<Descriptors.FileDescriptor>> protoFile = descriptors.right().get();
if (protoFile.isRight()){
Iterable<FileDescriptor> fileDescriptors = protoFile.right().get();
ModelBuilder.Model model = ModelBuilder.introspectProtobufClasses(fileDescriptors, log);
log.debug("Model: " + model);
Iterable<Path> generated = SourceGenerator.generate(
model,
sourceDirectory.toPath(),
testSourceDirectory.toPath(),
integrationTestSourceDirectory.toPath(),
generatedSourceDirectory.toPath(),
mainClass,
log);
Path absBaseDir = baseDir.toPath().toAbsolutePath();
generated.foreach(p -> {
log.info("Generated: " + absBaseDir.relativize(p.toAbsolutePath()));
return null;
});

project.addCompileSourceRoot(generatedSourceDirectory.toString());

} else {
throw new RuntimeException(new MojoExecutionException(
"There was a problem building the file descriptor from its protobuf: "
+ descriptor.left().get().toString()));
}
});
ModelBuilder.Model model = ModelBuilder.introspectProtobufClasses(fileDescriptors, log);
log.debug("Model: " + model);
Iterable<Path> generated = SourceGenerator.generate(
model,
sourceDirectory.toPath(),
testSourceDirectory.toPath(),
integrationTestSourceDirectory.toPath(),
generatedSourceDirectory.toPath(),
mainClass,
log);
Path absBaseDir = baseDir.toPath().toAbsolutePath();
generated.foreach(p -> {
log.info("Generated: " + absBaseDir.relativize(p.toAbsolutePath()));
return null;
});

project.addCompileSourceRoot(generatedSourceDirectory.toString());

"There was a problem building the file descriptor from its protobuf: "
+ descriptors.left().get().toString()));
}
} else {
throw new MojoExecutionException("There was a problem opening the protobuf descriptor file",
descriptors.left().get().e());
Expand Down
2 changes: 1 addition & 1 deletion samples/java-customer-registry/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<akkasls.dockerTag>${project.version}</akkasls.dockerTag>
<akkasls.mainClass>customer.Main</akkasls.mainClass>
<jdk.target>11</jdk.target>
<akkaserverless-sdk.version>0.7.0-beta.15</akkaserverless-sdk.version>
<akkaserverless-sdk.version>0.7.0-beta.18</akkaserverless-sdk.version>
<akka-grpc.version>1.1.1</akka-grpc.version>
</properties>

Expand Down
14 changes: 7 additions & 7 deletions samples/java-eventing-shopping-cart/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@

<name>Shopping Cart with Eventing</name>


<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<!-- TODO Update to your own Docker repository or Docker Hub scope -->
<akkasls.dockerImage>my-docker-repo/${project.artifactId}</akkasls.dockerImage>
<akkasls.dockerTag>${project.version}</akkasls.dockerTag>
<akkasls.mainClass>shopping.Main</akkasls.mainClass>
<dockerImage>gcr.io/akkaserverless-public/samples-java-eventing-shopping-cart</dockerImage>
<dockerTag>${project.version}</dockerTag>
<mainClass>shopping.Main</mainClass>

<jdk.target>11</jdk.target>
<akkaserverless-sdk.version>0.7.0-beta.15</akkaserverless-sdk.version>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<akkaserverless-sdk.version>0.7.0-beta.18</akkaserverless-sdk.version>
<akka-grpc.version>1.1.1</akka-grpc.version>
</properties>

Expand Down
2 changes: 1 addition & 1 deletion samples/java-eventsourced-shopping-cart/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<jdk.target>11</jdk.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<akkaserverless-sdk.version>0.7.0-beta.15</akkaserverless-sdk.version>
<akkaserverless-sdk.version>0.7.0-beta.18</akkaserverless-sdk.version>
<akka-grpc.version>1.1.1</akka-grpc.version>
</properties>

Expand Down
2 changes: 1 addition & 1 deletion samples/java-valueentity-shopping-cart/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<jdk.target>11</jdk.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<akkaserverless-sdk.version>0.7.0-beta.15</akkaserverless-sdk.version>
<akkaserverless-sdk.version>0.7.0-beta.18</akkaserverless-sdk.version>
<akka-grpc.version>1.1.1</akka-grpc.version>
</properties>

Expand Down
2 changes: 1 addition & 1 deletion samples/replicatedentity-counter/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<jdk.target>11</jdk.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<akkaserverless-sdk.version>0.7.0-beta.15</akkaserverless-sdk.version>
<akkaserverless-sdk.version>0.7.0-beta.18</akkaserverless-sdk.version>
<akka-grpc.version>1.1.1</akka-grpc.version>
</properties>

Expand Down
2 changes: 1 addition & 1 deletion samples/valueentity-counter/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
<jdk.target>11</jdk.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<akkaserverless-sdk.version>0.7.0-beta.15</akkaserverless-sdk.version>
<akkaserverless-sdk.version>0.7.0-beta.18</akkaserverless-sdk.version>
<akka-grpc.version>1.1.1</akka-grpc.version>
</properties>

Expand Down