DLP => v2 #1056

jabubake · 2018-03-09T19:12:56Z

(WIP)
Pending tasks:

-> Code cleanup / add tests (Thank you Kurtis !)
-> Submit for code review
-> Region tag / comment review
-> Update w/ release version after [do not merge] add dlp v2 googleapis/google-cloud-java#2958 is merged, and library is available on Maven Central

(WIP) Pending tasks: -> Update / Add Tests -> Region tag / comment review -> Submit for code review + fixes -> Merge once google-cloud-java PR : googleapis/google-cloud-java#2958 is released

ace-n · 2018-03-18T01:17:54Z

dlp/src/main/java/com/example/dlp/DeIdentification.java

+  /**
+   * [START dlp_deidentify_mask]
+   *
+   * <p>Deidentify a string by masking sensitive information with a character using the DLP API.


Nit: is the <p> intentional?

If not, please apply any required changes throughout this PR.

ace-n · 2018-03-18T01:19:52Z

dlp/src/main/java/com/example/dlp/DeIdentification.java

-              .setValue(string)
+      ByteContentItem byteContentItem =
+          ByteContentItem.newBuilder()
+              .setType(ByteContentItem.BytesType.TEXT_UTF8)


Nit: is this necessary?

ace-n · 2018-03-18T01:21:09Z

dlp/src/main/java/com/example/dlp/DeIdentification.java

+      // (Optional) The encrypted ('wrapped') AES-256 key to use when shifting dates
+      // This key should be encrypted using the Cloud KMS key specified above
+      // If this is specified, then 'keyName' and 'contextFieldId' must also be set
+      // const wrappedKey = 'YOUR_ENCRYPTED_AES_256_KEY'


s/const/String/ ?

ace-n · 2018-03-18T01:21:40Z

dlp/src/main/java/com/example/dlp/DeIdentification.java

+      // If this is specified, then 'keyName' and 'contextFieldId' must also be set
+      // const wrappedKey = 'YOUR_ENCRYPTED_AES_256_KEY'
+
+      // If contextFieldId , keyName or wrappedKey is set : all three arguments must be valid


Nit - remove spaces before , and : characters.

ace-n · 2018-03-18T01:23:35Z

dlp/src/main/java/com/example/dlp/DeIdentification.java

+
+      while ((line = br.readLine()) != null) {
+        // convert csv rows to Table.Row
+        rows.add(convertCsvRowToTableRow(line));


We should include this in our snippet - IMO it's not intuitive enough to leave the exercise to the reader.

Never mind - it is included in the snippet. :)

ace-n · 2018-03-19T06:43:57Z

dlp/src/test/java/com/example/dlp/RiskAnalysisIT.java

-    assertTrue(Pattern.compile(
-        "Most common value occurs \\d time\\(s\\)").matcher(output).find());
+
+    assertThat(output, containsString("Most common value occurs"));


Nit: should we check for the full string here? (e.g. to ensure that a digit is printed)

ace-n · 2018-03-19T06:46:49Z

dlp/src/test/java/com/example/dlp/RiskAnalysisIT.java

  private ByteArrayOutputStream bout;
  private PrintStream out;

+  private String topicId = "dlp-tests";
+  private String subscriptionId = "dlp-test";


Question: should these be based on environment variables?

(I don't know how the Java samples are tested - so perhaps I'm wrong about this. If you do change this, apply those changes throughout this PR.)

ace-n · 2018-03-19T06:47:46Z

dlp/src/test/java/com/example/dlp/TemplatesIT.java

+  public void testCreateInspectTemplate() throws Exception {
+    Templates.main(new String[] {
+        "-c",
+        "-displayName", String.format("test-name-%s", new Date()),


Can we use some sort of UUID, instead of the current time (which may not be globally unique)?

ace-n · 2018-03-19T06:50:30Z

dlp/src/test/java/com/example/dlp/TemplatesIT.java

+  }
+
+  @Test
+  public void testCreateInspectTemplate() throws Exception {


Question/thought: would it be better to combine the {Create, List, Delete}Template tests into one? That way the test for DeleteTemplate (which depends on templates having been created) will only run if CreateTemplate passes

ace-n · 2018-03-19T06:52:15Z

dlp/src/test/java/com/example/dlp/DeIdentificationIT.java

-    assertFalse(output.contains(text));
-    assertTrue(Pattern.compile("My SSN is \\w+").matcher(output).find());
+    assertThat(
+        output, containsString("Successfully saved date-shift output to: results.temp.csv"));


Should we also check the contents of results.temp.csv against a file containing the desired results?

ace-n · 2018-03-19T06:54:35Z

@jabubake you were right - that was a lot of Java code 😄 Nice work 👍

(Mostly nits, but there are some bigger things we may want to consider changing)

* Return results of all tests. * Use for loop instead of while.

ace-n · 2018-03-20T01:12:47Z

LGTM - Les, would you mind taking a look (in case Jisha is busy)?

lesv

PTAL - So, in general, I'd prefer smaller snippets, but it's probably ok here.

For testing, at least in Java, it's often better to snippet inside the method, and let the method return a result you check, rather that always looking at the printf results.

Ok to merge after you complete my comments, but please mention me so that I look at the last few commits.

lesv · 2018-03-20T02:38:14Z

dlp/src/main/java/com/example/dlp/Inspect.java


+  /**
+   * [START dlp_inspect_gcs]


Shouldn't this be before /**

lesv · 2018-03-20T02:38:50Z

dlp/src/main/java/com/example/dlp/Inspect.java

        }
      } else {
        System.out.println("No findings.");
      }
-    } catch (Exception e) {
-      e.printStackTrace();
-      System.out.println("Error in inspectGCSFileAsync: " + e.getMessage());
    }
    // [END dlp_inspect_gcs]


Shouldn't this be one line down ?? after final } since we aren't hiding a return to the test app.

lesv · 2018-03-20T02:39:16Z

dlp/src/main/java/com/example/dlp/Inspect.java


+  /**
+   * [START dlp_inspect_bigquery]


lesv · 2018-03-20T02:40:50Z

dlp/src/main/java/com/example/dlp/DeIdentification.java

+    }
+    return tableRowBuilder.build();
+  }
+  // [END dlp_deidentify_date_shift]


Wow - one long snippet. Should it be shorter?

lesv · 2018-03-20T02:45:44Z

dlp/src/main/java/com/example/dlp/RiskAnalysis.java

+              .build();
+      subscriber.startAsync();
+
+      // wait for job completion


Why do you show them this without commenting on what you might do in a production system?

Replaced with the following:

// Wait for job completion semi-synchronously // For long jobs, consider using a truly asynchronous execution model such as Cloud Functions

lesv · 2018-03-20T02:48:57Z

dlp/src/main/java/com/example/dlp/RiskAnalysis.java


+  /**
+   * [START dlp_l_diversity]


should be outside the /**

lesv · 2018-03-20T02:49:38Z

dlp/src/main/java/com/example/dlp/RiskAnalysis.java

+      // wait for job completion
+      try{
+        done.get(1, TimeUnit.MINUTES);
+        Thread.sleep(500);


lesv · 2018-03-20T02:50:05Z

dlp/src/main/java/com/example/dlp/RiskAnalysis.java

@@ -384,10 +557,9 @@ private static void calculateLDiversity(
    // [END dlp_l_diversity]


move outside final }

lesv · 2018-03-20T02:50:51Z

dlp/src/main/java/com/example/dlp/Templates.java

+public class Templates {
+
+  /**
+   * [START dlp_create_inspect_template]


move outside /**

lesv · 2018-03-20T02:53:12Z

dlp/src/test/resources/dates.csv

+Ann,01/01/1970,4532908762519852,07/21/1996
+James,03/06/1988,4301261899725540,04/09/2001
+Dan,08/14/1945,4620761856015295,11/15/2011
+Laura,11/03/1992,4564981067258901,01/04/2017


should there be a newline here?

ace-n · 2018-03-20T04:38:32Z

@simonz130 expressed similar feedback about .NET - perhaps (when we're not concerned with launches) we can chat about whether/how we want to redesign our tests.

jabubake · 2018-03-20T05:29:28Z

dlp/src/main/java/com/example/dlp/RiskAnalysis.java

+      // For long jobs, consider using a truly asynchronous execution model such as Cloud Functions
+      try{
+        done.get(1, TimeUnit.MINUTES);
+        Thread.sleep(500); // Wait for the job to become available


avoid Thread.sleep in the sample. the done blocks anyway.

jabubake · 2018-03-20T05:42:40Z

dlp/src/main/java/com/example/dlp/Inspect.java

+      // For long jobs, consider using a truly asynchronous execution model such as Cloud Functions
+      try{
+        done.get(1, TimeUnit.MINUTES);
+        Thread.sleep(500); // Wait for the job to become available


Not comfortable with this, since this sleep amount seems aribtrary

ace-n · 2018-03-20T06:14:26Z

@lesv FYI Jisha is wrapping this up - feel free to take a look in the future.

jabubake · 2018-03-20T06:42:22Z

Tests pass now. @lesv do take a final pass and merge if you approve.
FYI, @ace-n

lesv · 2018-03-20T19:08:38Z

Well, other than there being a merge conflict that I resolved... (let's see that tests work.)

lesv · 2018-03-20T19:14:15Z

@dpebot merge when green

dpebot · 2018-03-20T19:14:17Z

Okay! I'll merge when all statuses are green and all reviewers approve.

ace-n

LGTMing to unblock @dpebot

* DLP => v2 (WIP) Pending tasks: -> Update / Add Tests -> Region tag / comment review -> Submit for code review + fixes -> Merge once google-cloud-java PR : googleapis/google-cloud-java#2958 is released * Update to most recent versioning. * Updated DeIdentification samples and tests. * Revert pubsub to public version. * Fix Inspect samples/tests (minus pubsub). * Updated Jobs and add tests. * Updated Metadata classes. * Updated QuickStart tests and samples. * Updated Redact samples and tests. * Updated RiskAnalysis. * Update Template samples. * Update trigger tests. * Make Checkstyle Happy Again. * Fix (and ignore) tests using pubsub. * Update PR tests to complete all tests before returning results. (#1065) * Return results of all tests. * Use for loop instead of while. * WIP: Address PR feedback, part 1 * Update deps * Address PR feedback * Remove mvn clean verify failure * Add ReID FPE sample * Address PR feedback * Add k-map sample * checkstyle fixes

jabubake added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 9, 2018

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 9, 2018

kurtisvg force-pushed the dlp-v2 branch from 278b739 to d72410b Compare March 13, 2018 16:29

jabubake and others added 2 commits March 13, 2018 10:03

DLP => v2

6bdeaf6

(WIP) Pending tasks: -> Update / Add Tests -> Region tag / comment review -> Submit for code review + fixes -> Merge once google-cloud-java PR : googleapis/google-cloud-java#2958 is released

Update to most recent versioning.

284902e

kurtisvg force-pushed the dlp-v2 branch from 82edb0a to 284902e Compare March 13, 2018 17:03

Updated DeIdentification samples and tests.

184c006

kurtisvg force-pushed the dlp-v2 branch from 701dbd5 to 184c006 Compare March 13, 2018 20:46

kurtisvg added 11 commits March 13, 2018 14:31

Revert pubsub to public version.

1592ef4

Fix Inspect samples/tests (minus pubsub).

e38238b

Updated Jobs and add tests.

d99f680

Updated Metadata classes.

fb7261f

Updated QuickStart tests and samples.

f4b54b2

Updated Redact samples and tests.

d88015d

Updated RiskAnalysis.

d44100c

Update Template samples.

268796c

Update trigger tests.

3dc8bad

Make Checkstyle Happy Again.

678fd0b

Fix (and ignore) tests using pubsub.

f767a97

jabubake requested a review from ace-n March 15, 2018 22:52

ace-n suggested changes Mar 19, 2018

View reviewed changes

kurtisvg and others added 4 commits March 19, 2018 10:59

Update PR tests to complete all tests before returning results. (#1065)

70ca0f7

* Return results of all tests. * Use for loop instead of while.

WIP: Address PR feedback, part 1

abd1037

Update deps

d11d705

Address PR feedback

eb0e6d2

ace-n requested a review from lesv March 20, 2018 01:12

ace-n previously approved these changes Mar 20, 2018

View reviewed changes

Remove mvn clean verify failure

9af924d

ace-n dismissed their stale review via 9af924d March 20, 2018 01:16

lesv and others added 2 commits March 19, 2018 19:29

Merge branch 'master' into dlp-v2

9352db0

Add ReID FPE sample

cef37d5

lesv suggested changes Mar 20, 2018

View reviewed changes

Address PR feedback

cd75921

Add k-map sample

8ba4acb

jabubake commented Mar 20, 2018

View reviewed changes

jabubake force-pushed the dlp-v2 branch from 8ba4acb to 278b739 Compare March 20, 2018 06:08

jabubake force-pushed the dlp-v2 branch from 278b739 to 8ba4acb Compare March 20, 2018 06:17

checkstyle fixes

58625f9

jabubake removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 20, 2018

Merge branch 'master' into dlp-v2

9cac39b

lesv approved these changes Mar 20, 2018

View reviewed changes

dpebot added the automerge label Mar 20, 2018

dpebot self-assigned this Mar 20, 2018

ace-n approved these changes Mar 20, 2018

View reviewed changes

lesv merged commit d332e2e into master Mar 20, 2018

lesv deleted the dlp-v2 branch March 20, 2018 20:25

		@@ -384,10 +557,9 @@ private static void calculateLDiversity(
		// [END dlp_l_diversity]

DLP => v2 #1056

DLP => v2 #1056

Uh oh!

Conversation

jabubake commented Mar 9, 2018 • edited by dpebot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ace-n commented Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jabubake commented Mar 9, 2018 •

edited by dpebot

Loading

ace-n commented Mar 19, 2018 •

edited

Loading

ace-n commented Mar 20, 2018 •

edited

Loading

ace-n Mar 20, 2018 •

edited

Loading

ace-n Mar 20, 2018 •

edited

Loading