feat: add support for Proto Columns DDL

[harshachinta]

This PR adds feature support for Proto Columns DDL with supporting integration tests.

[generated-files-bot]

Warning: This pull request is touching the following templated files:

• google-cloud-spanner/src/main/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClient.java
• google-cloud-spanner/src/test/java/com/google/cloud/spanner/admin/database/v1/DatabaseAdminClientTest.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/CreateDatabaseRequest.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/CreateDatabaseRequestOrBuilder.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/GetDatabaseDdlResponse.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/GetDatabaseDdlResponseOrBuilder.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/SpannerDatabaseAdminProto.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/UpdateDatabaseDdlRequest.java
• proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/UpdateDatabaseDdlRequestOrBuilder.java
• proto-google-cloud-spanner-admin-database-v1/src/main/proto/google/spanner/admin/database/v1/spanner_database_admin.proto

[gauravpurohit06]

It looks like the method only support reading files from Project Resources. If yes please clarify it in java doc.

[harshachinta]

Added in java doc

[Shua1]

Is Project Resources a good default?

[harshachinta]

Yes. Having the resources with in the class path can help users provide a relative file path as input and Project resources directory is a good place to maintain all these files.

[olavloite]

While I understand the argument that having the proto descriptors in the resources directory of your project makes sense, I'm not sure that this will work as expected in all cases. This code does not live in the project of the end user, but in the client library. In an end user project, the client library will be one of many dependencies packaged as a jar. Will this always include the resources of directories of the end user project? What if that project uses multiple class loaders? I'm afraid that this could cause unexpected behavior.

[harshachinta]

@olavloite
My understanding here is class loader can find a resource if it is there in the classpath and resources directory would always be a part of the classpath. I tried this with a small demo application and it works fine.
However, I am not aware of the behaviour if there are any custom/multiple class loaders. I would need some time to analyse the behaviour in this case.
In the interest of time, I am planning to remove this overload that accepts a file path as argument and take it separately in another PR.
Can you please let me know your views here?

[olavloite]

I think that removing this for now is a good choice. People can easily use resources from their class path using the overload that takes an InputStream by calling getResourceAsStream() themselves.

[harshachinta]

Removed the overload changes.

[gauravpurohit06]

LGTM

[Shua1]

Is Project Resources a good default?

[Shua1]

Do we have a precedence of reading file on user's behalf? What about other Google Cloud client libraries. Are we reading files in the recommended way in case there is one?

[harshachinta]

User's can read the file on their end and pass in the byte[] or InputStream as input to the setProtoDescriptors method. Alternatively, we also add another overloaded method that accepts the file path as input.

[olavloite]

Sorry, I didn't notice this during the last review, but this seems a little strange. It is a method in the Database class and takes a Database instance as an argument. Why does it not just operate on this database?

[harshachinta]

The reason it cannot operate on this database is because a user cannot set ProtoDescriptor directly to Database Class as setProtoDescriptors method belongs to Database.Builder. Below is a small example to illustrate this,

Database database = client.getDatabase(INSTANCE_ID, DB_ID);

// If a user wants to update the ProtoDescriptor field then, 
// the below line cannot be written as setProtoDescriptors method does not belong to Database class.
database.setProtoDescriptors("descriptors.pb"); 

// This does not update proto descriptor in the database because the field was not updated.
database.updateDdl(Collections.singletonList("DROP TABLE Singer"), null); 

So a new Database instance has to be passed to update the ProtoDescriptor field. However, I agree that this is incorrect and confusing at same time.

Another Approach
@olavloite Please validate if below is more meaningful to do.

Expose a new method toBuilder() in Database.java that return a Builder of that Database instance. This allows users to setProtoDescriptor by calling the builder.
The change in Database.java is

public class Database extends DatabaseInfo {
  public Builder toBuilder() {
      return new Builder(this);
    }


  public OperationFuture<Void, UpdateDatabaseDdlMetadata> updateDdl(
      Iterable<String> statements, String operationId) throws SpannerException {
     // The updateDatabaseDdl is called with 'this' as argument instead of instance(), database()
    return dbClient.updateDatabaseDdl(this, statements, operationId);
  }
}

The user can use it like below

Database database = client.getDatabase(INSTANCE_ID, DB_ID);
// The below database instance has the updated ProtoDescriptor field
database = database.toBuilder().setProtoDescriptors("descriptors.pb").build();
// Update works fine
database.updateDdl(Collections.singletonList("DROP TABLE Singer"), null);

[olavloite]

Hmmm.... If the protoDescriptors field is not something that is actually part of the Database class and thereby also not stored with the database, and is only needed for the update method, then we need to add it as an argument to the method afte rall. Other approaches would be non-idiomatic Java.

So the signature of the method would then become Database#updateDatabaseDdl(Iterable<String> statements, String protoDescriptors, String operationId). Another option would be to create a DatabaseUpdateOptions class that contains all possible update options, the first one being protoDescriptors (that would make it more easily reusable for additional options in the future).

[harshachinta]

I agree with you @olavloite. Creating a new DatabaseUpdateOptions class that can update options will help us reuse in future. I will take this effort as part of the next PR. I will tag this comment in the other PR later to understand the need of that change.

[olavloite]

While I understand the argument that having the proto descriptors in the resources directory of your project makes sense, I'm not sure that this will work as expected in all cases. This code does not live in the project of the end user, but in the client library. In an end user project, the client library will be one of many dependencies packaged as a jar. Will this always include the resources of directories of the end user project? What if that project uses multiple class loaders? I'm afraid that this could cause unexpected behavior.