googleapis / sdk-platform-java Goto Github PK

take gapic-generator repo as example.

Steps to reproduce

1. Attempt to generate gapic client for storage/v2
2. Receive compile error

   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/CryptoKeyName.java:[173,3] method does not override or implement a method from a supertype
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/CryptoKeyName.java:[175,11] incomparable types: com.google.storage.v2.Object and com.google.storage.v2.CryptoKeyName
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/CryptoKeyName.java:[178,33] incomparable types: java.lang.Class<capture#1 of ? extends com.google.storage.v2.CryptoKeyName> and java.lang.Class<capture#2 of ? extends com.google.storage.v2.Object>
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/CryptoKeyName.java:[179,45] incompatible types: com.google.storage.v2.Object cannot be converted to com.google.storage.v2.CryptoKeyName
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/BucketName.java:[135,3] method does not override or implement a method from a supertype
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/BucketName.java:[137,11] incomparable types: com.google.storage.v2.Object and com.google.storage.v2.BucketName
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/BucketName.java:[140,33] incomparable types: java.lang.Class<capture#3 of ? extends com.google.storage.v2.BucketName> and java.lang.Class<capture#4 of ? extends com.google.storage.v2.Object>
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/BucketName.java:[141,39] incompatible types: com.google.storage.v2.Object cannot be converted to com.google.storage.v2.BucketName
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/ProjectName.java:[125,3] method does not override or implement a method from a supertype
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/ProjectName.java:[127,11] incomparable types: com.google.storage.v2.Object and com.google.storage.v2.ProjectName
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/ProjectName.java:[130,33] incomparable types: java.lang.Class<capture#5 of ? extends com.google.storage.v2.ProjectName> and java.lang.Class<capture#6 of ? extends com.google.storage.v2.Object>
   [ERROR] .../src/github.com/googleapis/java-storage/proto-google-cloud-storage-v2/src/main/java/com/google/storage/v2/ProjectName.java:[131,41] incompatible types: com.google.storage.v2.Object cannot be converted to com.google.storage.v2.ProjectName
   

BucketName, CryptoKeyName and ProjectName all implemented ResourceName and follow a different code generation path than the other proto classes. This different generation path does not use fully qualified class names for java.lang classes. This particular scenario is manifesting because GCS generate a class com.google.storage.v1.Object which takes resolution precedence to java.lang.Object and prevents java.lang.Object#equals(java.lang.Object) from being implemented.

As a temporary work around I'm manually applying a search and replace which can be seen here: https://github.com/googleapis/java-storage/blob/d6acdab54582852b3c14e152a12ea2f923ff85ef/synth.py#L40-L58

The following release PRs may have failed:

• #681

The following release PRs may have failed:

• #692

Affects : 1.58.2

ServerStream currently doesn't allow to call the iterator() method more than once. However some tool that consumes this iterable may want to check the characteristics. E.g the reactor project (webflux) which is now checking the characteristics of the iterator via Iterable.spliterator(). However this has the side effect of invoking the default method which invokes the iterator() method, thus setting the consumed boolean to true.

FluxIterable

	static <T> boolean checkFinite(Iterable<T> iterable) {
		return iterable instanceof Collection || iterable.spliterator().hasCharacteristics(Spliterator.SIZED);
	}

Iterable

    default Spliterator<T> spliterator() {
        return Spliterators.spliteratorUnknownSize(iterator(), 0);
    }

While this can be worked around for this particular use case, it would be nice if the ServerStream provide basic support for spliterators.

I was just testing a change to build_gen to introduce the Location pkg deps when the mixin is present and noticed in the generated output that MockLocations.java and MockLocationsImpl.java. I'm not sure that these should be generated since there isn't a Locations client generated, but I will let you @miraleung decide.

Only message is currently supported, but cause may need to be passed into the constructor as well.

We need a centralized name conflict resolver that would generalize #803 and #760.

Our libraries, though compiled on Java 8 will need to support the Java 7 API for a bit longer.

https://github.com/googleapis/java-servicedirectory/pull/244/checks?check_run_id=1485724473

This will enable expressions like throw e.

The current approach pulls the version from the jar manifest. Unfortunately this will break if the application builds an uber jar (the version will come from the uber jar rather then the generated client or gax). A more robust approach would to follow grpc's approach and embed it in a class or to use the maven-resources-plugin. I recently worked around this issue in the bigtable client:
googleapis/java-bigtable#451

In ads API, we use resource names which contain ~ to delimit multiple IDs in a path segment. For example:

message AdGroupAd {
  option (google.api.resource) = {
    type: "googleads.googleapis.com/AdGroupAd"
    pattern: "customers/{customer_id}/adGroupAds/{ad_group_id}~{ad_id}"
  };

The current microgenerator implementation doesn't appear to support this, and instead just generates a single param in its place.

Link to protos: http://shortn/_zIPrXkqkoU

For instance, see TryCatchStatement's setTryBody.

Context here.

In regards of dropping support of Java 7, the backport classes brought by threeten are no longer needed. In an effort to deprecate the usage of threeten classes, we will follow a deprecation plan involving threeten classes being marked as @ObsoleteApi, while the newly introduced java.time classes will be the recommended ones. See #1872 for more info

Gax uses a pattern of abstract <B extends Builder<SettingsT, B>> B toBuilder(); in a base class, expecting a subclass to override it with a concrete return type.

This causes warnings like:

BigtableTableAdminStubSettings.java:[764,43] build() in com.google.cloud.bigtable.admin.v2.stub.BigtableTableAdminStubSettings.Builder overrides <B>build() in com.google.api.gax.rpc.StubSettings.Builder
  return type requires unchecked conversion from com.google.cloud.bigtable.admin.v2.stub.BigtableTableAdminStubSettings to com.google.api.gax.rpc.StubSettings<B>

It might be cleaner remove the abstract toBuilder() method from the base classes and trust that the subclasses will implement that method

https://github.com/googleapis/gapic-generator-java/blob/master/src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java#L209

The generated close() method in the client class should not be final, so that clients can override it as needed.

Context:
The Google Photos Library API client library, extends the generated client library and wraps some additional functionality in its main client class PhotosLibraryClient. In particular, it includes upload functionality that is handled via an internal BackgroundResource that must be cleaned up after use.

Unfortunately, the close() method included in the autogenerated client class (InternalPhotosLibraryClient) is marked as final, which means that we are not able to override it and implement our own close() method.
As proposed in google/java-photoslibrary#40 we are considering manually removing the final keyword from the generated method so that it can be overridden in our client class. Ideally this is something that can be done at the Java microgenerator level, so that we don't have to remove the keyword when the client library is regenerated.

googleapis/gax-java#1238 introduced a new feature to the ApiCallContext that allows end users to supply RetrySetttings to be used for a single method invocation. The examples and documentation in the PR are generic, and show how to create an entirely new RetrySettings instance to supply via withRetrySettings, but there is API specific documentation that ggj could generate that would greatly help users. We should generate a single, client-level example of configuring RetrySettings via the ApiCallContext in the GAPIC layer that utilizes that GAPIC's StubSettings class as a base RetrySettings to modify. An example usage is as follows:

LanguageServiceClient client = LanguageServiceClient.create();

// In order to adjust the RetrySettings on a per-call basis, modify the default settings
// and supply it via the ApiCallContext.
RetrySettings retrySettings =
  client
    .getSettings()
      .analyzeSentimentSettings()
      .getRetrySettings()
        .toBuilder()
          .setTotalTimeout(Duration.ofSeconds(10L))
          .builder();

ApiCallContext callContext = GrpcCallContext.createDefault().withRetrySettings(retrySettings);

client.analyzeSentimentCallable().call(request, callContext);

Note: The spacing is probably off there.

A similar existing example in LanguageServiceStubSettings.

I'm thinking it would be generated in this client-level doc comment block somewhere.

cc: @miraleung

This seems to be happening only in a very limited scope of the paginated <smth>Callable() methods.

The incode-samples for paginated methods seem missing request creation logic. This is how they look now

 // AUTO-GENERATED DOCUMENTATION AND METHOD.
  /**
   * Lists DeidentifyTemplates. See https://cloud.google.com/dlp/docs/creating-templates-deid to
   * learn more.
   *
   * <p>Sample code:
   *
   * <pre>{@code
   * try (DlpServiceClient dlpServiceClient = DlpServiceClient.create()) {
   *   while (true) {
   *     ListDeidentifyTemplatesResponse response =
   *         dlpServiceClient.listDeidentifyTemplatesCallable().call(request);
   *     for (DeidentifyTemplate element : response.getResponsesList()) {
   *       // doThingsWith(element);
   *     }
   *     String nextPageToken = response.getNextPageToken();
   *     if (!Strings.isNullOrEmpty(nextPageToken)) {
   *       request = request.toBuilder().setPageToken(nextPageToken).build();
   *     } else {
   *       break;
   *     }
   *   }
   * }
   * }</pre>
   */
  public final UnaryCallable<ListDeidentifyTemplatesRequest, ListDeidentifyTemplatesResponse>
      listDeidentifyTemplatesCallable() {
    return stub.listDeidentifyTemplatesCallable();
  }

Notice that the request is not defined when first used in the example above.
Also there is an extra } right at the end (before </pre> element).

This is how they used to be:

 // AUTO-GENERATED DOCUMENTATION AND METHOD
  /**
   * Lists DeidentifyTemplates. See https://cloud.google.com/dlp/docs/creating-templates-deid to
   * learn more.
   *
   * <p>Sample code:
   *
   * <pre><code>
   * try (DlpServiceClient dlpServiceClient = DlpServiceClient.create()) {
   *   OrganizationLocationName parent = OrganizationLocationName.of("[ORGANIZATION]", "[LOCATION]");
   *   ListDeidentifyTemplatesRequest request = ListDeidentifyTemplatesRequest.newBuilder()
   *     .setParent(parent.toString())
   *     .build();
   *   while (true) {
   *     ListDeidentifyTemplatesResponse response = dlpServiceClient.listDeidentifyTemplatesCallable().call(request);
   *     for (DeidentifyTemplate element : response.getDeidentifyTemplatesList()) {
   *       // doThingsWith(element);
   *     }
   *     String nextPageToken = response.getNextPageToken();
   *     if (!Strings.isNullOrEmpty(nextPageToken)) {
   *       request = request.toBuilder().setPageToken(nextPageToken).build();
   *     } else {
   *       break;
   *     }
   *   }
   * }
   * </code></pre>
   */
  public final UnaryCallable<ListDeidentifyTemplatesRequest, ListDeidentifyTemplatesResponse>
      listDeidentifyTemplatesCallable() {
    return stub.listDeidentifyTemplatesCallable();
  }

The request is defined and initialized before being used.

Comment from @miraleung in #73
Could you please take a look at VariableExpr to ensure we can never do something like null x, and add a test case? Same for MethodDefinition (e.g. it is invalid to do public null close()). and anywhere else where Object types may apply. Feel free to do those in separate PRs.

googleapis/java-cloudbuild#343

see: codecov/patch — 77.14% of diff hit (target 81.69%)

Currently generated stubs delegate close() to BackgroundResourceAggregation#shutdown(). Which will effectively end up calling channel.shutdown() followed immediately by executor.shutdown(). This can result in RPCs hanging indefinitely.

Steps leading to a hang:

future = stub.someCallable().futureCall(); // start rpc
// prevents new RPCs from starting, but existing RPCs can continue
// executor is shutdown
stub.close();
// grpc receives a response, but doesn't have executor to notify the future
// so this will hang indefinitely
future.get()

To avoid this situation, stub.close() should call close() on background resources, which wait for the channel to terminate before shutting down the executor.

See googleapis/java-bigtable#916 for more details

GAPIC generation on the googleads/v5 protos fails with a NPE when encountering an LRO with response_type: "google.protobuf.Empty".

Specifically, when GAPIC generation gets to googleads/v5/services/batch_job_service.proto:RunBatchJob, the following exception is thrown:

Exception in thread "main" java.lang.NullPointerException: LRO response message Empty not found on method RunBatchJob
	at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:895)
	at com.google.api.generator.gapic.protoparser.Parser.parseLro(Parser.java:384)
	at com.google.api.generator.gapic.protoparser.Parser.parseMethods(Parser.java:331)
	at com.google.api.generator.gapic.protoparser.Parser.lambda$parseService$0(Parser.java:196)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:948)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
	at com.google.api.generator.gapic.protoparser.Parser.parseService(Parser.java:199)
	at com.google.api.generator.gapic.protoparser.Parser.parseServices(Parser.java:132)
	at com.google.api.generator.gapic.protoparser.Parser.parse(Parser.java:103)
	at com.google.api.generator.gapic.Generator.generateGapic(Generator.java:29)
	at com.google.api.generator.Main.main(Main.java:35)

per @lesv we should replace FindBugs with SpotBugs

StreamingCallSettings are ignored when creating a bidiStreamingCallable: https://github.com/googleapis/gax-java/blob/5fd82c7058bf780dacbf0127cc5b4f74fd4a9800/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcCallableFactory.java#L231-L248

See: https://github.com/googleapis/google-cloud-java/issues/6051

The user wishes to use a custom name resolver which is supported by gRPC (see example)

Example: https://googleapis.github.io/google-cloud-java/google-api-grpc/apidocs/com/google/cloud/speech/v1/RecognitionAudioOrBuilder.html#getUri--

URI that points to a file that contains audio data bytes as specified in
`RecognitionConfig`. The file must not be compressed (for example, gzip).
Currently, only Google Cloud Storage URIs are
supported, which must be specified in the following format:
`gs://bucket_name/object_name` (other URI formats return
[google.rpc.Code.INVALID_ARGUMENT][google.rpc.Code.INVALID_ARGUMENT]). For more information, see
[Request URIs](https://cloud.google.com/storage/docs/reference-uris).

Code that glues ClientSettings to the ClientContext is generated by gapic generator making it difficult to test that configuration options make it to the transport layer.

Can we generate a test client where we can mock out the transport for testing purposes?

Calls to get auth tokens should have their own trace span to surface issues with slowness issues with auth.

This is needed for #804 and #792.

For gapic service rpcs:

option deprecated = true

Example: proto client

Expected:

• The method should be annotationed @Deprecated
• The javadoc should contain @deprecated with a note to replacement (if possible)

Filing an issue so we can more easily track this across APIs.

Interceptors currently have to be specified on an implementation of TransportChannelProvider, e.g. InstantiatingGrpcChannelProvider.

What do you think about adding methods to TransportChannelProvider to set interceptors? The idea being to write unit tests which replacing InstantiatingGrpcChannelProvider with LocalChannelProvider, and still being able to test interceptors all through the stack? We might need some convenience methods for test asserts in LocalChannelProvider as well.

Here are the current implementations (in GAX):

LocalChannelProvider (com.google.api.gax.grpc.testing)
InstantiatingGrpcChannelProvider (com.google.api.gax.grpc)
FixedTransportChannelProvider (com.google.api.gax.rpc)
InstantiatingHttpJsonChannelProvider (com.google.api.gax.httpjson)

LocalChannelProvider and InstantiatingGrpcChannelProvider can both implement these methods.

FixedTransportChannelProvider should probably return false for needsInterceptors() and throw on withInterceptors().

InstantiatingHttpJsonChannelProvider currently doesn't support interceptors, and I didn't see any support drilling down to HttpRequestRunnable. Maybe should do the same as above for FixedTransportChannelProvider?

The comment Builder classes currently break the expected immutability pattern. That is, Builder setFoo() should not alter the class, but return a newly-constructed Builder object with the desired changes applied.

@xiaozhenliu-gg5 FYI

googleapis/synthtool#1151

It looks like the setIamPolicy method's sample generated for the Javadoc is alternating between CryptoKeyVersionName and DatabaseName

Java codegen for list methods should not generate null checks like this one for proto fields https://github.com/googleapis/java-pubsublite/blob/da98b0f8212d33ace2e3f9d034f7a923ef05c43a/google-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/v1/stub/AdminServiceStubSettings.java#L187

This is a hard blocker for pulling these libraries into the google codebase which has a compiler error for null checks on protobuf fields. Per https://developers.google.com/protocol-buffers/docs/reference/java-generated, java protobuf fields will never return null.

Environment details

• Programming language: Java
• OS: All
• Language runtime version: All
• Package version: All

Steps to reproduce

1. Write a java client library with a list request
2. Codegen adds redundant check

Currently, InstantiatingGrpcChannelProvider endpoints must be host:port strings. This is a request to support target strings, e.g. that can be resolved using custom name resolvers. Implementation-wise, it seems that allowing a target string to be passed through to ManagedChannelBuilder.forTarget() instead of forAddress() would be sufficient.

Primary use cases are for testing.

If a grpc_service_config is not supplied at generation time the generator throws an error. Instead, the library should still be generated, but without default retry/timeout settings.