Cleanup DLP Risk Analysis snippets #2069

shubha-rajan · 2020-02-13T22:13:45Z

Working towards #1479
This ended up being a larger PR than expected. I can split it up if needed.

lesv · 2020-02-13T22:36:09Z

Please fix compile problems

shubha-rajan · 2020-02-14T00:02:47Z

Please fix compile problems

build failures are a result of broken tests that were removed in #2062 and #2065. Once those are merged in, everything should pass.

kurtisvg · 2020-02-14T22:50:59Z

dlp/src/main/java/dlp/snippets/RiskAnalysisCategoricalStats.java

+                    ProjectSubscriptionName.of(projectId, subscriptionId);
+
+            MessageReceiver handleMessage =
+                    (PubsubMessage pubsubMessage, AckReplyConsumer ackReplyConsumer) -> {


This is probably too long for a lamba - can you surface it out into a full function? I think within the class is still fine

I was unsure of how to do this when the lambda used variables outside of the scope of the function, like dlpJob and done. handleMessage needs to be callable so I need to return a function. I'm not sure if there's a way to build functions using closures in Java? I ended up nesting the lambda within another function, and passing the DlpJob and Future to that function in in c634313, but that seems more complicated and less readable to me? Thoughts?

kurtisvg · 2020-02-14T22:52:29Z

dlp/src/main/java/dlp/snippets/RiskAnalysisCategoricalStats.java

+            // Retrieve completed job status
+            DlpJob completedJob =
+                    dlpServiceClient.getDlpJob(
+                            GetDlpJobRequest.newBuilder()


Can we pull the request build into a separate var/line? I think that will actually make this shorter.

resolved in 8546690

kurtisvg · 2020-02-14T22:52:48Z

dlp/src/main/java/dlp/snippets/RiskAnalysisCategoricalStats.java

+            // Get the result and parse through and process the information
+            CategoricalStatsResult result =
+                    completedJob.getRiskDetails().getCategoricalStatsResult();
+


nit: remove newline

kurtisvg · 2020-02-14T22:53:26Z

dlp/src/main/java/dlp/snippets/RiskAnalysisCategoricalStats.java

+            CategoricalStatsResult result =
+                    completedJob.getRiskDetails().getCategoricalStatsResult();
+
+            for (CategoricalStatsHistogramBucket bucket :


nit: I would pull result.getValueFrequencyHistogramBucketsList() into it's own line too (still 2 lines, but easier to read)

(same for a couple of statements below, but up to you)

fixed in e6d4f91

kurtisvg · 2020-02-14T22:55:18Z

dlp/src/main/java/dlp/snippets/RiskAnalysisKAnonymity.java

+        String tableId = "your-bigquery-table-id";
+        String topicId = "pub-sub-topic";
+        String subscriptionId = "pub-sub-subscription";
+        List<String> quasiIdColumns = Arrays.asList("name", "age", "zip_code", "...");


These should be inline - not required for testing

Resolved in 8546690

kurtisvg · 2020-02-14T23:00:36Z

dlp/src/main/java/dlp/snippets/RiskAnalysisKMap.java

+                    ProjectSubscriptionName.of(projectId, subscriptionId);
+
+            MessageReceiver handleMessage =
+                    (PubsubMessage pubsubMessage, AckReplyConsumer ackReplyConsumer) -> {


Long lamba again

kurtisvg · 2020-02-14T23:00:59Z

dlp/src/main/java/dlp/snippets/RiskAnalysisKMap.java

+            KMapEstimationResult kmapResult  = completedJob.getRiskDetails().getKMapEstimationResult();
+
+            for (KMapEstimationHistogramBucket result : kmapResult.getKMapEstimationHistogramList()) {
+


nit: no newline

resolved in 92ea643

kurtisvg · 2020-02-14T23:06:49Z

dlp/src/main/java/dlp/snippets/RiskAnalysisKMap.java

+                                    .getQuasiIdsValuesList()
+                                    .stream()
+                                    .map(
+                                            v -> {


A comment on this function would be helpful.

nit: Does there have to be a new line before v -> { (might be a java format thing, just looks weird to me)?

fixed the weird formatting thing, and also rewrote this part to be a bit clearer and more consistent with other samples in e6d4f91. I can also go in and comment on it if that's still necessary.

kurtisvg · 2020-02-14T23:08:09Z

dlp/src/main/java/dlp/snippets/RiskAnalysisLDiversity.java

+            String topicId,
+            String subscriptionId,
+            String sensitiveAttribute,
+            List<String> quasiIds) throws Exception {


sensitiveAttribute, quasiIds inlined

resolved in 82c1f07

kurtisvg · 2020-02-14T23:08:31Z

dlp/src/main/java/dlp/snippets/RiskAnalysisLDiversity.java

+                    ProjectSubscriptionName.of(projectId, subscriptionId);
+
+            MessageReceiver handleMessage =
+                    (PubsubMessage pubsubMessage, AckReplyConsumer ackReplyConsumer) -> {


Long lambda

See my previous comment about the lambda. Also, this lambda is also used in the Inspect snippets, so if we change it here, we should probably change it there as well.

shubha-rajan · 2020-02-15T06:47:26Z

Presubmit tests are going to fail because of linting errors in other files outside of this PR. Those fixes are handled in #2083, since we probably don't want all of those changes in this PR

…ency

kurtisvg · 2020-02-19T00:30:53Z

dlp/src/main/java/dlp/snippets/RiskAnalysisKMap.java

+        throw new IllegalArgumentException("The numbers of quasi-IDs and infoTypes must be equal!");
+      }
+
+      ArrayList<TaggedField> taggedFields =


nit: Does this need to be an ArrayList or can it be just a List?

kurtisvg · 2020-02-19T00:42:03Z

dlp/src/main/java/dlp/snippets/RiskAnalysisKMap.java

+      }
+
+      ArrayList<TaggedField> taggedFields =
+          IntStream.range(0, quasiIds.size())


nit: This might just be a case where it is easier to read as a for loop.

replaced with a for loop in 0e72a97

kurtisvg · 2020-02-19T21:15:52Z

forcing merge - (broken tests tracking in #1479)

* refactored Risk analysis samples * updated inspect tests and readme * moved parameters to inline, cleaned up files, reformatted for consistency * replaced iterators with intstream in RiskAnalysisKMap * moved message receiver lambda to its own method * ran code formatter * refactored long messageReciever lambda * rewrote lambda for pubsub message receiver * replaced int stream with for loop

shubha-rajan requested review from kurtisvg and a team February 13, 2020 22:13

blunderbuss-gcf bot assigned lesv Feb 13, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 13, 2020

shubha-rajan unassigned lesv Feb 13, 2020

lesv approved these changes Feb 13, 2020

View reviewed changes

kurtisvg suggested changes Feb 14, 2020

View reviewed changes

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Feb 15, 2020

shubha-rajan force-pushed the dlp-snippets-refactor-4 branch from 9d2733f to b8e00a5 Compare February 15, 2020 05:23

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 15, 2020

shubha-rajan force-pushed the dlp-snippets-refactor-4 branch from b8e00a5 to 0df96fc Compare February 15, 2020 05:26

GoogleCloudPlatform deleted a comment from googlebot Feb 15, 2020

shubha-rajan mentioned this pull request Feb 15, 2020

Fix Checkstyle errors for DLP Snippets and Tests #2083

Merged

shubha-rajan force-pushed the dlp-snippets-refactor-4 branch from 0df96fc to c634313 Compare February 15, 2020 06:19

shubha-rajan requested a review from kurtisvg February 18, 2020 17:40

shubha-rajan added 7 commits February 18, 2020 14:19

refactored Risk analysis samples

17e26c7

updated inspect tests and readme

13e8e98

moved parameters to inline, cleaned up files, reformatted for consist…

f1e0638

…ency

replaced iterators with intstream in RiskAnalysisKMap

f6d9ad3

moved message receiver lambda to its own method

065187d

ran code formatter

a063df3

refactored long messageReciever lambda

673fec4

shubha-rajan force-pushed the dlp-snippets-refactor-4 branch from 92ea643 to 673fec4 Compare February 18, 2020 22:40

rewrote lambda for pubsub message receiver

0079aa3

kurtisvg approved these changes Feb 19, 2020

View reviewed changes

replaced int stream with for loop

0e72a97

kurtisvg approved these changes Feb 19, 2020

View reviewed changes

kurtisvg merged commit c8ef8a5 into master Feb 19, 2020

kurtisvg deleted the dlp-snippets-refactor-4 branch February 19, 2020 21:16

		KMapEstimationResult kmapResult = completedJob.getRiskDetails().getKMapEstimationResult();

		for (KMapEstimationHistogramBucket result : kmapResult.getKMapEstimationHistogramList()) {

Cleanup DLP Risk Analysis snippets #2069

Cleanup DLP Risk Analysis snippets #2069

Uh oh!

Conversation

shubha-rajan commented Feb 13, 2020

Uh oh!

lesv commented Feb 13, 2020

Uh oh!

shubha-rajan commented Feb 14, 2020 • edited 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!

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!

shubha-rajan commented Feb 15, 2020

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!

kurtisvg commented Feb 19, 2020

Uh oh!

Uh oh!

shubha-rajan commented Feb 14, 2020 •

edited

Loading