[PLUGIN-632] - Added: ServiceAccountType as JSON, unit tests, validations #13

flakrimjusufi · 2021-04-29T14:39:19Z

Make Google drive/sheet plugins production ready
JIRA Ticket: https://cdap.atlassian.net/browse/PLUGIN-632

jasir99 · 2021-04-30T12:39:34Z

lgtm

rmstar

Please test source and sink plugins in the following scenarios:

In CDF with file path set to auto-detect.
In CDF, specify file path
In CDF, specify Service Account JSON
In CDAP, specify file path
In CDAP, specify Service Account JSON
Test with service account type, file path/JSON set as macro param.

rmstar · 2021-05-03T21:06:36Z

src/main/java/io/cdap/plugin/google/drive/source/GoogleDriveInputFormat.java

@@ -18,6 +18,9 @@

 import com.github.rholder.retry.RetryException;
 import com.google.api.services.drive.model.File;
+import com.google.gson.JsonObject;


Why are these imports added?

These imports were left behind after we did some additional tests. Thanks for letting me know. I removed them.

rmstar · 2021-05-03T21:06:52Z

src/main/java/io/cdap/plugin/google/drive/source/SchemaBuilder.java

@@ -27,7 +27,7 @@
 * Util class for building pipeline schema.
 */
 public class SchemaBuilder {
-  public static final String SCHEMA_ROOT_RECORD_NAME = "FileFromFolder";
+  public static final String SCHEMA_ROOT_RECORD_NAME = "etlSchemaBody";


why was this change needed?

The schema was not being updated in Google Drive Source and initially we thought that this was the case, but then we found out that the issue was related with widget, not with SCHEMA_ROOT_RECORD_NAME. Reverted to its initial name in the updated PR.

rmstar · 2021-05-03T21:07:23Z

src/main/java/io/cdap/plugin/google/common/GoogleDriveClient.java

@@ -98,7 +105,7 @@ protected Credential getCredentials() throws IOException {
  }

  protected List<String> getRequiredScopes() {
-    return Collections.singletonList(DriveScopes.DRIVE_READONLY);
+    return Collections.singletonList(DriveScopes.DRIVE);


why did we need to make this change?

This was also as part of the testings that we have done. We were dealing with this error message when testing the pipeline:

404 Not Found { "code" : 404, "errors" : [ { "domain" : "global", "location" : "fileId", "locationType" : "parameter", "message" : "File not found: 1ZqAr0q6gITvjqvfOJqkSIxd-usTNhvJy.", "reason" : "notFound" } ], "message" : "File not found: 1ZqAr0q6gITvjqvfOJqkSIxd-usTNhvJy." }. Please check the system logs for more details.

...and checking the documentation of Google Drive API for 404 error type, stating that:

To fix this error:

1. Inform the user they don't have read access to the file or that the file doesn't exist. 2. Instruct the user to ask the owner for permission to the file.

...we changed the SCOPE from DRIVE_READONLY to DRIVE.

But afterwards, when we verified that in fact we were not in the right path when testing the pipelines and DRIVE_READONLY was enough for read/write, we changed back to its initial SCOPE.

In the last commit, we reverted back the scope to DRIVE_READONLY.

With SCOPE set to DRIVE does everything work correctly?

Actually, everything also works correctly with SCOPE set to DRIVE_READONLY, that's why we reverted back the code.

rmstar · 2021-05-03T21:09:02Z

src/main/java/io/cdap/plugin/google/common/GoogleAuthBaseConfig.java

-  private boolean validateAccountFilePath(FailureCollector collector) {
-    if (!containsMacro(ACCOUNT_FILE_PATH)) {
+  private boolean validateServiceAccount(FailureCollector collector) {
+    if (isServiceAccountFilePath() != null && isServiceAccountFilePath()) {


store the return value in a variable so you don't have to call isServiceAccountFilePath() twice. Same thing for isServiceAccountJson() below.

PR updated.

flakrimjusufi · 2021-05-04T12:50:28Z

Please test source and sink plugins in the following scenarios:

In CDF with file path set to auto-detect.

In CDF, specify file path

In CDF, specify Service Account JSON

In CDAP, specify file path

In CDAP, specify Service Account JSON

Test with service account type, file path/JSON set as macro param.

I'm unable to test with file path set to auto-detect in CDF because my keys have insufficient permissions. To be more specific, I've used flakrim@cirus.co to test the plugin since I don't have access to Google Drive with my partner account flakrim@88547414.corp-partner.google.com. In CDF, we only have access with partnership accounts.
Is there a way to test the pipelines in CDF while specifying the file path? As far as I know, in CDF we can only test with Service Account as JSON. Please correct me if my assumption is wrong.
Test passed successfully.
Test passed successfully.
Test passed successfully.
There's an issue when you test with MACRO. We were getting this error message:

Java.util.concurrent.ExecutionException: com.google.gson.JsonSyntaxException: com.google.gson.stream.MalformedJsonException: Unterminated object at line 1 column 5965 at com.google.common.util.concurrent.AbstractFuture$Sync.getValue(AbstractFuture.java:294) ~[com.google.guava.guava-13.0.1.jar:na] at com.google.common.util.concurrent.AbstractFuture$Sync.get(AbstractFuture.java:281) ~[com.google.guava.guava-13.0.1.jar:na] at com.google.common.util.concurrent.AbstractFuture.get(AbstractFuture.java:116) ~[com.google.guava.guava-13.0.1.jar:na]
That's because of this block of code:

google-drive/src/main/java/io/cdap/plugin/google/drive/source/GoogleDriveInputFormatProvider.java

Lines 40 to 43 in a9be711

    
             this.conf = new ImmutableMap.Builder<String, String>() 
        
               .put(PROPERTY_CONFIG_JSON, GSON.toJson(config)) 
        
               .build(); 
        
           }

After debugging this issue, we found out that the entire config was serialized.

Taking a consideration that we have to do a deserialization of the entire config and this is time consuming (after this fix, the entire functionality of source and sink plugin needs to be tested), we removed Macros at this moment of time. We were told that the requirements were prioritized and the fix needs to be delivered as soon as possible.

However, we can proceed with the deserialization of config and we can include Macros in this PR if needed.

CuriousVini · 2021-05-05T02:17:42Z

@flakrimjusufi
Do you know what permissions are missing from your account?

CuriousVini · 2021-05-05T02:33:22Z

Also are you noticing permissions issues at pipeline runtime? If so, could you please make sure dataproc service account (GCE default service account) has permissions to access drive directories/files.

rmstar · 2021-05-05T03:32:04Z

Taking a consideration that we have to do a deserialization of the entire config and this is time consuming (after this fix, the entire functionality of source and sink plugin needs to be tested), we removed Macros at this moment of time. We were told that the requirements were prioritized and the fix needs to be delivered as soon as possible.

Macros are supported, so we should make sure they work correctly. Please fix it as part of this PR.

flakrimjusufi · 2021-05-05T10:52:52Z

@flakrimjusufi
Do you know what permissions are missing from your account?

With our partnership accounts, we don't have access in any of gmail, google drive, etc. So, to test Google Drive Source/Sink in data proc, I need one of the followings:

My account in cirus (which I've used to create a project and work with Google Drive API) to have access in data proc.
My partnership account to have access in Google Drive API.

Permission issues are showing up in validation when file path is set to auto-detect in datafusion, I can't run the pipeline.

flakrimjusufi · 2021-05-06T15:00:04Z

Taking a consideration that we have to do a deserialization of the entire config and this is time consuming (after this fix, the entire functionality of source and sink plugin needs to be tested), we removed Macros at this moment of time. We were told that the requirements were prioritized and the fix needs to be delivered as soon as possible.

Macros are supported, so we should make sure they work correctly. Please fix it as part of this PR.

@rmstar

In the last update, we deserialized the config and added Macros.

rmstar · 2021-05-10T23:22:22Z

src/main/java/io/cdap/plugin/google/common/GoogleAuthBaseConfig.java

+
+  @Nullable
+  public String getServiceAccountFilePath() {
+    if (containsMacro(ACCOUNT_FILE_PATH)  || accountFilePath == null ||


nit: replace accountFilePath == null || accountFilePath.isEmpty() -> Strings.insNullOrEmpty(accountFilePath)

rmstar · 2021-05-10T23:29:49Z

src/main/java/io/cdap/plugin/google/common/GoogleAuthBaseConfig.java

+  }
+
+  @Nullable
+  public String getServiceAccount() {


This is only used in GoogleBasicAuthConfig.java where you're checking if the returned string is null or empty. The string value is unused.

It seems this is unnecessary. You just need something like this:

if (config.isServiceAccountFilePath()) { ... } else if (config. isServiceAccountJson()) { ... } else { // use default credentials }

Removed in the last update.

rmstar · 2021-05-10T23:32:09Z

src/test/java/io/cdap/plugin/google/common/GoogleAuthBaseConfigTest.java

+import org.junit.Test;
+
+public class GoogleAuthBaseConfigTest {
+


can you also add tests for validation error handling?

Added tests for validation error handling in the last update.

flakrimjusufi · 2021-05-11T14:11:24Z

@rmstar

In the last commit, GoogleSheets plugins are also updated with serviceAccountType as JSON, macros and documentation.

Both Source and Sink were tested thoroughly and everything is working well.

rmstar

Can you add an ETL test with macros for google drive and sheets?

rmstar · 2021-05-13T06:00:55Z

src/main/java/io/cdap/plugin/google/common/GoogleAuthBaseConfig.java

+  public static final String NAME_SERVICE_ACCOUNT_JSON = "serviceAccountJSON";
+  public static final String SERVICE_ACCOUNT_FILE_PATH = "filePath";
+  public static final String SERVICE_ACCOUNT_JSON = "JSON";
+  public static final String SCHEMA = "schema";


where is this used?

SCHEMA is used in deserialization of config:

google-drive/src/main/java/io/cdap/plugin/google/drive/source/GoogleDriveSourceConfig.java

Lines 358 to 360 in 3074341

if (properties.has(GoogleDriveSourceConfig.SCHEMA)) {

googleDriveSourceConfig.setSchema(properties.get(GoogleDriveSourceConfig.SCHEMA).getAsString());

}

rmstar · 2021-05-13T06:06:12Z

src/main/java/io/cdap/plugin/google/sheets/source/GoogleSheetsSourceConfig.java

  /**
   * Returns the instance of Schema.
   * @return The instance of Schema
   */
  public Schema getSchema() {
-    if (schema == null) {
-      if (dataSchemaInfo.isEmpty()) {
-        throw new RuntimeException("There are no headers to process. " +


Is this not relevant anymore?

It doesn't look relevant to me because we are already checking if Source folder (directory) contains any spreadsheets files in here:

google-drive/src/main/java/io/cdap/plugin/google/sheets/source/GoogleSheetsSourceConfig.java

Lines 348 to 354 in 3074341

private void validateSourceFolder(FailureCollector collector, List<File> spreadsheetsFiles) {

if (spreadsheetsFiles.isEmpty()) {

collector.addFailure(String.format("No spreadsheets found in '%s' folder with '%s' filter.",

getDirectoryIdentifier(), getFilter()), null)

.withConfigProperty(DIRECTORY_IDENTIFIER).withConfigProperty(FILTER);

}

}

...but I will revert this RuntimeException in getSchema().

hm why are we throwing a RuntimeException here? If config validation fails, we should add the failure to the failure collector (pass in FailureCollector, call collector.addFailure()).

PR updated.

rmstar · 2021-05-13T06:07:19Z

src/main/java/io/cdap/plugin/google/sheets/source/GoogleSheetsSourceConfig.java

-        throw new RuntimeException("There are no headers to process. " +
-          "Perhaps no validation step was executed before schema generation.");
-      }
+    if (schema == null && !containsMacro("serviceAccountType")) {


What happens if service account type is not macro (set to default, i.e. file path), but the file path is a macro?

Thanks for catching this. In this scenario the validation would fail. In the next update, I will also check if file path or service acccount json contains Macro.

flakrimjusufi · 2021-05-17T14:45:48Z

Can you add an ETL test with macros for google drive and sheets?

I'm seeing lots of dependencies that are missing in pom.xml. Should I create a separate PR only with ETL Tests? It will take some time because first I will need to include all the missing dependencies, create the ETL Tests and then I will have to do a thorough test of both Google Drive and Google Sheets plugin.

rmstar · 2021-05-18T19:09:13Z

Should I create a separate PR only with ETL Tests?

You can add a test in a separate PR if that's easier for you.

rmstar

please squash commits

…ions, documentation and re-factoring of GoogleDrive and GoogleSheets plugin

flakrimjusufi · 2021-05-20T08:28:26Z

please squash commits

Commits squashed.

rmstar reviewed May 3, 2021

View reviewed changes

rmstar reviewed May 10, 2021

View reviewed changes

rmstar reviewed May 13, 2021

View reviewed changes

rmstar approved these changes May 20, 2021

View reviewed changes

[PLUGIN-632] - Added: ServiceAccountType as JSON, more tests, validat…

2e92d32

…ions, documentation and re-factoring of GoogleDrive and GoogleSheets plugin

flakrimjusufi force-pushed the feature/PLUGIN-632 branch from 9892bea to 2e92d32 Compare May 20, 2021 08:27

rmstar merged commit ffdaf36 into data-integrations:develop May 20, 2021

This was referenced May 21, 2021

[PLUGIN-632] Added ETL Test for Google Drive and Google Sheets plugin #14

Open

[PLUGIN-632] - Cherrypick for release/1.3 #15

Merged

		import org.junit.Test;

		public class GoogleAuthBaseConfigTest {

	if (properties.has(GoogleDriveSourceConfig.SCHEMA)) {
	googleDriveSourceConfig.setSchema(properties.get(GoogleDriveSourceConfig.SCHEMA).getAsString());
	}

	private void validateSourceFolder(FailureCollector collector, List<File> spreadsheetsFiles) {
	if (spreadsheetsFiles.isEmpty()) {
	collector.addFailure(String.format("No spreadsheets found in '%s' folder with '%s' filter.",
	getDirectoryIdentifier(), getFilter()), null)
	.withConfigProperty(DIRECTORY_IDENTIFIER).withConfigProperty(FILTER);
	}
	}

[PLUGIN-632] - Added: ServiceAccountType as JSON, unit tests, validations #13

[PLUGIN-632] - Added: ServiceAccountType as JSON, unit tests, validations #13

Conversation

flakrimjusufi commented Apr 29, 2021

jasir99 commented Apr 30, 2021

rmstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flakrimjusufi commented May 4, 2021

CuriousVini commented May 5, 2021

CuriousVini commented May 5, 2021

rmstar commented May 5, 2021

flakrimjusufi commented May 5, 2021 • edited Loading

flakrimjusufi commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flakrimjusufi commented May 11, 2021 • edited Loading

rmstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flakrimjusufi commented May 17, 2021

rmstar commented May 18, 2021

rmstar left a comment

Choose a reason for hiding this comment

flakrimjusufi commented May 20, 2021

flakrimjusufi commented May 5, 2021 •

edited

Loading

flakrimjusufi commented May 11, 2021 •

edited

Loading