Add data masking client, test, recording #3391

nathannfan · 2017-06-20T20:57:18Z

Description

Add generated client, test and recording for Data Masking APIs.

TODO: need to update AssemblyInfo.cs because we already have 2 SQL SDK pull requests open that haven't been reviewed yet, so need to determine the version based on which ones merge first.

Azure/azure-rest-api-specs#1321

This checklist is used to make sure that common guidelines for a pull request are followed.

I have read the contribution guidelines.
The pull request does not introduce breaking changes.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

SDK Generation Guidelines

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

nathannfan · 2017-06-20T20:57:43Z

Corresponding swagger pull request: Azure/azure-rest-api-specs#1321

Should be merged soon, is only blocked because the reviewer assigned is OOF.

yaakoviyun · 2017-06-22T17:52:56Z

src/SDKs/SqlManagement/Sql.Tests/Utilities/SqlManagementTestUtilities.cs

@@ -40,6 +40,10 @@ public class SqlManagementTestUtilities

        public const string DefaultEuapPrimaryLocation = "East US 2 EUAP";

+        public const string DefaultLogin = "dummylogin";
+
+        public const string DefaultPassword = "Un53cuRE!";


can we generate it in runtime with some random seed?

Unfortunately, because this affects how test recordings and playback works, changing this would mean changing every single scenario test and re-recording them again, so it's not trivial.

EDIT: Created issue #3405 to track this.

yaakoviyun · 2017-06-22T17:54:19Z

src/SDKs/SqlManagement/Sql.Tests/DataMaskingScenarioTests.cs

+        [Fact]
+        public void TestCreateUpdateGetDataMaskingRules()
+        {
+            string testPrefix = "sqlcrudtest-";


Please add data masking string to the test prefix

yaakoviyun · 2017-06-22T17:55:33Z

src/SDKs/SqlManagement/Management.Sql/generate.cmd

@@ -5,7 +5,7 @@

 @echo off
 if  "%1" == "" (
-    set specFile="https://raw.githubusercontent.com/Azure/azure-rest-api-specs/9346f0adc0caddbfa33dbe69a469c199705cf38a/arm-sql/compositeSql.json"
+    set specFile="https://raw.githubusercontent.com/Azure/azure-rest-api-specs/03ff383aac77901093d96c049e248d2c7a3cf4ff/arm-sql/compositeSql.json"


why do we need this change?

Ask from the .net SDK team to know what version of compositeSql.json is used to generate client

It's SDK Generation Guidelines number 2 :)

yaakoviyun · 2017-06-22T17:55:48Z

src/SDKs/SqlManagement/Management.Sql/Generated/Models/TransparentDataEncryptionStatus.cs

@@ -27,4 +27,32 @@ public enum TransparentDataEncryptionStatus
        [EnumMember(Value = "Disabled")]
        Disabled
    }
+    internal static class TransparentDataEncryptionStatusEnumExtension


how is this change related?

Generated client from latest swagger

yaakoviyun · 2017-06-22T17:56:28Z

src/SDKs/SqlManagement/Management.Sql/Generated/Models/SecurityAlertPolicyUseServerDefault.cs

+                case "Enabled":
+                    return SecurityAlertPolicyUseServerDefault.Enabled;
+                case "Disabled":
+                    return SecurityAlertPolicyUseServerDefault.Disabled;            }


the "}" should be in a new line

Filed Azure/autorest#2386 against autorest code generator to fix this

yaakoviyun · 2017-06-22T17:56:41Z

src/SDKs/SqlManagement/Management.Sql/Generated/Models/SecurityAlertPolicyState.cs

+                case "Enabled":
+                    return SecurityAlertPolicyState.Enabled;
+                case "Disabled":
+                    return SecurityAlertPolicyState.Disabled;            }


the "}" should be in a new line

This is generated code and changing the formatting would just be overwritten the next time someone generates client

yaakoviyun · 2017-06-22T17:57:26Z

src/SDKs/SqlManagement/Management.Sql/Generated/Models/DataMaskingRule.cs

+        public DataMaskingRuleState? RuleState { get; set; }
+
+        /// <summary>
+        /// Gets or sets the schema name on which the data masking rule is


can the comment be in one line? (here and below)

This is generated code and changing the formatting would just be overwritten the next time someone generates client

yaakoviyun · 2017-06-22T17:57:54Z

src/SDKs/SqlManagement/Management.Sql/Generated/Models/DataMaskingRule.cs

+        /// </exception>
+        public virtual void Validate()
+        {
+            if (SchemaName == null)


data masking function should not be null too...

Will need a re-generation which will need swagger update

Data masking function is stored here as a non-nullable enum

jaredmoo

Looks great, just minor comments

jaredmoo · 2017-06-22T18:34:13Z

src/SDKs/SqlManagement/Sql.Tests/DataMaskingScenarioTests.cs

+                {
+                    SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder()
+                    {
+                        DataSource = string.Format("{0}.database.windows.net", server.Name),


I think there is server.FullyQualifiedDomainName that you can use

jaredmoo · 2017-06-22T18:34:42Z

src/SDKs/SqlManagement/Sql.Tests/DataMaskingScenarioTests.cs

+                    {
+                        DataSource = string.Format("{0}.database.windows.net", server.Name),
+                        UserID = SqlManagementTestUtilities.DefaultLogin,
+                        Password = SqlManagementTestUtilities.DefaultPassword,


Nice change, can you also update ImportExport tests to use these?

jaredmoo · 2017-06-22T18:35:59Z

src/SDKs/SqlManagement/Sql.Tests/DataMaskingScenarioTests.cs

+                // Create test table with columns
+                HttpRecorderMode testMode = HttpMockServer.GetCurrentMode();
+
+                if (testMode != HttpRecorderMode.Playback)


Please add comment explaining why it's not needed in Playback, i.e. because there is no HTTP requests in this block and it requires the server to really exist

jaredmoo · 2017-06-22T18:36:48Z

src/SDKs/SqlManagement/Sql.Tests/DataMaskingScenarioTests.cs

+        {
+            string testPrefix = "sqlcrudtest-";
+            string suiteName = this.GetType().FullName;
+            SqlManagementTestUtilities.RunTestInNewV12Server(suiteName, "TestCreateUpdateGetDataMaskingRules", testPrefix, (resClient, sqlClient, resourceGroup, server) =>


Can you use new SqlManagementTestContext instead? See Restore LTR test for example

jaredmoo · 2017-06-22T21:19:16Z

src/SDKs/SqlManagement/Sql.Tests/DataMaskingScenarioTests.cs

+        public void TestCreateUpdateGetDataMaskingRules()
+        {
+            string testPrefix = "sqldatamaskingcrudtest-";
+            string suiteName = this.GetType().FullName;


suiteName unused?

shahabhijeet · 2017-06-22T19:10:57Z

src/SDKs/SqlManagement/Management.Sql/Generated/Models/DataMaskingRule.cs

+        /// </exception>
+        public virtual void Validate()
+        {
+            if (SchemaName == null)


Will need a re-generation which will need swagger update

nathannfan · 2017-06-23T23:49:37Z

@shahabhijeet Thanks, abhijeet!

msftclas added the cla-not-required label Jun 20, 2017

nathannfan added 2 commits June 21, 2017 18:51

Add Data Masking client, tests, recordings

9f87ae6

Update threat detection recording

d16ab44

nathannfan force-pushed the dm branch from bb7d0ab to d16ab44 Compare June 22, 2017 02:09

Update compositeSql.json to reflect commit used

4c5e539

jaredmoo added SQL needs-review labels Jun 22, 2017

yaakoviyun approved these changes Jun 22, 2017

View reviewed changes

jaredmoo suggested changes Jun 22, 2017

View reviewed changes

Respond to comments, re-record

4660023

jaredmoo approved these changes Jun 22, 2017

View reviewed changes

Remove unused variable

47da8ef

shahabhijeet approved these changes Jun 23, 2017

View reviewed changes

shahabhijeet merged commit d6c9636 into Azure:psSdkJson6 Jun 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add data masking client, test, recording #3391

Add data masking client, test, recording #3391

nathannfan commented Jun 20, 2017

nathannfan commented Jun 20, 2017

yaakoviyun Jun 22, 2017

nathannfan Jun 22, 2017 •

edited

Loading

yaakoviyun Jun 22, 2017

nathannfan Jun 22, 2017

yaakoviyun Jun 22, 2017

nathannfan Jun 22, 2017

jaredmoo Jun 22, 2017

yaakoviyun Jun 22, 2017

nathannfan Jun 22, 2017

yaakoviyun Jun 22, 2017

jaredmoo Jun 22, 2017

yaakoviyun Jun 22, 2017

nathannfan Jun 22, 2017

yaakoviyun Jun 22, 2017

nathannfan Jun 22, 2017

yaakoviyun Jun 22, 2017

shahabhijeet Jun 22, 2017

nathannfan Jun 22, 2017

jaredmoo left a comment

jaredmoo Jun 22, 2017

jaredmoo Jun 22, 2017

jaredmoo Jun 22, 2017

jaredmoo Jun 22, 2017

jaredmoo Jun 22, 2017

shahabhijeet Jun 22, 2017

nathannfan commented Jun 23, 2017

Add data masking client, test, recording #3391

Add data masking client, test, recording #3391

Conversation

nathannfan commented Jun 20, 2017

Description

nathannfan commented Jun 20, 2017

Choose a reason for hiding this comment

nathannfan Jun 22, 2017 • edited Loading

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

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

jaredmoo 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

nathannfan commented Jun 23, 2017

nathannfan Jun 22, 2017 •

edited

Loading