Skip to content
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

feat: add support for Proto Columns DDL #2277

Merged
merged 25 commits into from
Mar 15, 2023
Merged

feat: add support for Proto Columns DDL #2277

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b95f577
feat: add code changes and tests for Proto columns DDL support
harshachinta Jan 27, 2023
1653916
feat: add auto generated code
harshachinta Jan 30, 2023
88cb28d
feat: code changes and tests for Proto columns DDL support
harshachinta Feb 1, 2023
75a23de
feat: add descriptors file
harshachinta Feb 1, 2023
f738af6
feat: code refactoring
harshachinta Feb 8, 2023
a1b5ab0
feat: Integration tests and code refactoring
harshachinta Feb 11, 2023
678e8ee
feat: code refactoring
harshachinta Feb 12, 2023
e85e350
feat: unit tests and clirr differences
harshachinta Feb 12, 2023
bb04177
feat: lint changes
harshachinta Feb 13, 2023
f4c7c96
feat: code refactor
harshachinta Feb 13, 2023
80526b5
feat: code refactoring
harshachinta Feb 13, 2023
88fa533
feat: code refactoring
harshachinta Feb 13, 2023
d33f816
feat: code refactoring
harshachinta Feb 14, 2023
fb0293a
feat: add java docs to new methods
harshachinta Feb 14, 2023
c45143c
feat: lint formatting
harshachinta Feb 14, 2023
7c6472d
feat: lint formatting changes
harshachinta Feb 14, 2023
50b8666
feat: lint formatting
harshachinta Feb 14, 2023
72af9f3
feat: lint formatting
harshachinta Feb 14, 2023
e355e4a
feat: test exception cases
harshachinta Feb 15, 2023
eb938bc
feat: code refactoring
harshachinta Feb 27, 2023
f762e39
feat: add java docs and refactoring
harshachinta Feb 27, 2023
30be84a
feat: add java docs
harshachinta Feb 28, 2023
70acee6
feat: java docs refactor
harshachinta Mar 15, 2023
51a4031
feat: remove overload method setProtoDescriptors that accepts file pa…
harshachinta Mar 15, 2023
bfae4ad
feat: remove updateDdl method overload to update proto descriptor
harshachinta Mar 15, 2023
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
32 changes: 30 additions & 2 deletions google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.cloud.Timestamp;
import com.google.cloud.spanner.encryption.CustomerManagedEncryption;
import com.google.common.base.Preconditions;
import com.google.common.io.ByteStreams;
import com.google.protobuf.ByteString;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -63,11 +62,39 @@ public Builder setDialect(Dialect dialect) {
throw new UnsupportedOperationException("Unimplemented");
}

/**
* Optional for creating a new database.
*
* <p>The proto descriptors input as byte[] to be used for the database.
gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
*
* <p>It is used by CREATE/ALTER PROTO BUNDLE statements which are part of DDL statements.
* Contains a protobuf-serialized [google.protobuf.FileDescriptorSet]. To generate a proto
* descriptors file run `protoc` with --include_imports and --descriptor_set_out.
gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
*/
public abstract Builder setProtoDescriptors(@Nonnull byte[] protoDescriptors);
harshachinta marked this conversation as resolved.
Show resolved Hide resolved

/**
* Optional for creating a new database.
*
* <p>The proto descriptors input as InputStream to be used for the database.
gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
*
* <p>It is used by CREATE/ALTER PROTO BUNDLE statements which are part of DDL statements.
* Contains a protobuf-serialized [google.protobuf.FileDescriptorSet]. To generate a proto
* descriptors file run `protoc` with --include_imports and --descriptor_set_out.
gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
*/
public abstract Builder setProtoDescriptors(@Nonnull InputStream inputStream)
harshachinta marked this conversation as resolved.
Show resolved Hide resolved
throws IOException;

/**
* Optional for creating a new database.
*
* <p>The proto descriptors file path input as String to be used for the database. The proto
gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
* descriptors file must be present with in the project resources directory.
*
* <p>It is used by CREATE/ALTER PROTO BUNDLE statements which are part of DDL statements.
* Contains a protobuf-serialized [google.protobuf.FileDescriptorSet]. To generate a proto
* descriptors file run `protoc` with --include_imports and --descriptor_set_out.
gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
*/
public abstract Builder setProtoDescriptors(@Nonnull String filePath) throws IOException;

abstract Builder setProto(com.google.spanner.admin.database.v1.Database proto);
Expand Down Expand Up @@ -172,7 +199,8 @@ public Builder setProtoDescriptors(@Nonnull InputStream inputStream) throws IOEx
@Override
public Builder setProtoDescriptors(@Nonnull String filePath) throws IOException {
Preconditions.checkNotNull(filePath);
Preconditions.checkState(filePath.length() != 0, "Input Proto Descriptors File Path cannot be empty.");
Preconditions.checkState(
filePath.length() != 0, "Input Proto Descriptors File Path cannot be empty.");
InputStream inputStream = getClass().getClassLoader().getResourceAsStream(filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in java doc

Copy link

Choose a reason for hiding this comment

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

Is Project Resources a good default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the overload changes.

gauravpurohit06 marked this conversation as resolved.
Show resolved Hide resolved
Preconditions.checkNotNull(inputStream, "Input Proto Descriptors File path is invalid.");
return setProtoDescriptors(inputStream);
Expand Down
3 changes: 1 addition & 2 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/DatabaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
import static com.google.cloud.spanner.DatabaseInfo.State.CREATING;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.MockitoAnnotations.initMocks;
Expand All @@ -37,7 +37,6 @@
import com.google.rpc.Status;
import com.google.spanner.admin.database.v1.DatabaseDialect;
import com.google.spanner.admin.database.v1.EncryptionInfo;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.util.Collections;
Expand Down