Add scripting API implemented by the SqlScriptPublishModel #316

pensivebrian · 2017-04-14T21:59:58Z

Update the ScriptingService to expose new scripting JSON-RPC APIs that use the SqlScriptPublishModel for script generation.

The SqlScriptPublishModel is the model behind the SSMS scripting wizard. To enable scripting for CLI tools, we've ported SqlScriptPublishModel to .NET Core. The SqlScriptPublishModel wraps the SMO scripting APIs for .sql script generation.

Added three new methods to the ScriptingService: ScriptingRequest, ScriptingListObjectsRequest, ScriptingCancelRequest.
Generating scripts are long running operations, so the ScriptingRequest and ScriptingListObjectsRequest kick off a long running scripting task and return immediately.
Long running scripting task reports progress and completion, and can be cancelled by a ScriptingCancelRequest request.
Bumped the SMO nuget package to 140.17049.0. This new version contains a signed SSMS_Rel build of SMO with the SqlScriptPublishModel.
For testing, adding the Northwind database schema

TODO (in later pull requests)

Integrate the new ScriptingService APIs with the ConnectionService
Integrate with the metadata support recently added

The SqlScriptPublishModel is the model behind the SSMS scripting wizard. It has been ported to .NET Core to enable scripting scenarios to SqlToolsService. This change adds the SqlScriptPublishModel nuget package to the checked in packages in the bin\nuget folder.

The scripting service uses the SqlScriptPublishModel to drive scripting operations.

Running against docker, you cannot user the server name from the connection string when composing a SMO Urn instance. The result is the SMO Urn won't resolve since the docker server name must be used, however you cannot use the docker server name in the connection string. The fix is to connect to the server with the passed connection string and resolve the server name used for SMO Urn.

…eScriptExecuteRequest

msftclas · 2017-04-14T22:00:03Z

@pensivebrian,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

MrMeemus · 2017-04-14T22:05:33Z

@MrMeemus is added to the review. #Closed

kevcunnane · 2017-04-14T23:42:23Z

src/Microsoft.SqlTools.ServiceLayer/Localization/sr.Designer.cs

+        /// <summary>
+        ///    Looks up a localized string similar to TIME column values must be between 00:00:00.0000000 and 23:59:59.9999999.
+        /// </summary>
+        public static string EditDataTimeOver24Hrs {


EditDataTimeOver24Hrs [](start = 29, length = 21)

The resx is in sync with this? #ByDesign

I've sync'd the latest code from dev. It seems the checkin run srgen and include the latest sr.Designer.cs

In reply to: 111650561 [](ancestors = 111650561)

kevcunnane · 2017-04-14T23:42:50Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingExtensionMethods.cs

+
+namespace Microsoft.SqlTools.ServiceLayer.Scripting
+{
+    public static class ScriptingExtensionMethods


Please make internal or document any public classes / methods. Thanks! #Resolved

Fixed.

In reply to: 111650593 [](ancestors = 111650593)

kevcunnane

Please fix the test timeout on AppVeyor and the main service issue. All in all this is great, let's get this in once we fix these things.

After all tests run, if a static method "Cleanup" exists on a type, it's invoked so that class level test resources, such as a database, can be deleted.

coveralls · 2017-04-15T04:31:24Z

Coverage decreased (-6.07%) to 75.002% when pulling 83afb81 on feature/scripterService into 88eb0f6 on dev.

kburtram

It looks like this PR is having a big negative impact on code coverage. Please add additional test coverage to avoid this.

1) Use the existing pattern for kicking off async tasks for the scripting service operations. 2) Changed the SMO Urn format to not set the server name, so the Urn should resolve against the current server. 3) More logging 4) Use the xunit IClassFixture mechnism to manage the northwind database 5) Added docs

coveralls · 2017-04-17T22:33:21Z

Coverage decreased (-3.8%) to 77.322% when pulling e492c32 on feature/scripterService into 88eb0f6 on dev.

benrr101

Definitely some patterns I'd like to see addressed.

Also, I'd really really really like to see some unit tests for the service, and ideally more components. However, since a lot of this is SMO driven, it'll be harder to mock it up. Doesn't mean it's impossible..... at the very least please create an issue to address this going forward.

benrr101 · 2017-04-19T18:09:47Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/Contracts/ScriptingCancelEvent.cs

+    /// <summary>
+    /// Event sent to indicate that a scripting operation has been canceled.
+    /// </summary>
+    public class ScriptingCancelEvent


In previous API requests (edit/initialize in particular) we have a single event that is raised when the initialization is completed (edit/initializeSessionReady). The event has parameters to indicate success or failure. This also makes it simpler on the client side to implement b/c there is only one listener for the edit/initializeSessionReady event. In this scenario, you'll need to create 3 listeners (complete, cancel, and error) that will need to be shut down when any one of them receives an event.
Since you've got a different payload to include with each kind of event, I can see why using separate events would be beneficial, but it is slightly different than the APIs already developed. #Resolved

Makes sense to combine completed and cancel if that's the pattern used by services.

benrr101 · 2017-04-19T18:10:38Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/Contracts/ScriptingCancelRequest.cs

+        /// <summary>
+        /// Gets or sets the operation id of the scripting operation to cancel.
+        /// </summary>
+        public string OperationId { get; set; }


I'm assuming this should inherit from ScriptingEventParams? #Resolved

Yep, nice catch.

benrr101 · 2017-04-19T18:13:30Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/Contracts/ScriptingObject.cs

+            return objectName;
+        }
+
+        public override int GetHashCode()


I'm hoping this is the auto-implemented GetHashCode from ReSharper 😃 #WontFix

Nope, I don't use ReSharper :)

benrr101 · 2017-04-19T18:17:57Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/Contracts/ScriptingOptions.cs

+        /// <summary>
+        /// Generate ANSI padding statements
+        /// </summary>
+        public string ScriptAnsiPadding { get; set; }


JSON.NET very successfully handles Nullables. I would strongly advise switching to that since it provides much better type safety. If you're really not into changing these to Nullable, I would even more strongly advise adding a comment to indicate what the expected values are (eg, I thought ScriptAnsiPadding was a value for what the ANSI padding character should be, not whether or not it should generate padding statements). #Resolved

benrr101 · 2017-04-19T18:19:03Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/Contracts/ScriptingOptions.cs

+        /// <summary>
+        /// Convert user-defined data types to base types.
+        /// </summary>
+        public string ConvertUDDTToBaseType { get; set; }


As per MSDN standards (which I don't have a link to, atm) acronyms in variable names should only have the first character capitalized if >2 characters #WontFix

I'm using the same names used by SMO. I'm my mind, this makes it easier to reason over all of options. I'm going to leave as-is for now unless folks feel strongly.

In reply to: 112277263 [](ancestors = 112277263)

benrr101 · 2017-04-19T18:54:38Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingScriptOperation.cs

+            }
+            catch (Exception e)
+            {
+                throw new ArgumentException("Error parsing ConnectionString property", e);


Just noticed this, but make sure that all these exception messages are being added to strings.sr. They will need to be localized if they will be sent over JSON RPC #Resolved

benrr101 · 2017-04-19T18:58:10Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs

+            {
+                try
+                {
+                    this.ActiveOperations[operation.OperationId] = operation;


This should be using the TryAdd method, otherwise it's possible to orphan an in progress operation #WontFix

A new operation is instantiated per request, and each has a unique operation id so this should never happen. I added debug assert to make this more explicit.

benrr101 · 2017-04-19T19:00:49Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs

+                finally
+                {
+                    ScriptingOperation temp;
+                    this.ActiveOperations.TryRemove(operation.OperationId, out temp);


Doesn't this just remove the operation every time you start it up, regardless of if it fails? #ByDesign

benrr101 · 2017-04-19T19:03:40Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs

+        /// </summary>
+        private void RunScriptTask(ScriptingScriptOperation operation)
+        {
+            Task.Run(async () =>


I'm not quite sure the benefit of wrapping this in a Task.Run... If all the operation.Executes don't await anything, then there's no need to make .Execute async. Then you don't need to wrap the startup operation in a Task. #Resolved

You're right, the await is not needed. I removed the awaits and made Execute synchronous.

benrr101 · 2017-04-19T19:04:23Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingService.cs

+        /// </summary>
+        private void RunScriptTask(ScriptingScriptOperation operation)
+        {
+            Task.Run(async () =>


But I'm also curious since this pattern is seemingly reused several times, why isn't this method used more than once? Or more concisely, what makes this private method different than the other implementations of the same pattern. #Resolved

Good point. I refactored to single method reused by the request handlers.

coveralls · 2017-04-19T19:29:54Z

Coverage increased (+12.9%) to 81.418% when pulling 327c659 on feature/scripterService into 84ec20d on dev.

1) Keep the RequestContext isolated to the service 2) Combine completion/error/cancel events 3) Misc code review fixes

1) Cancellation logic moved into base class 2) Refactored sending events from the scripting service 3) Make all tests pass

…eters

…imeouts

My tests now run, and coveage was above back up to 80% before the latest drop

kevcunnane · 2017-04-21T20:41:53Z

src/Microsoft.SqlTools.ServiceLayer/Scripting/ScriptingObjectMatchProcessor.cs

+namespace Microsoft.SqlTools.ServiceLayer.Scripting
+{
+    /// <summary>
+    /// Implementes the matchin logic to filter scripting objects based on


Minor typos: Implements the matching logic #Resolved

kevcunnane

Looks good

coveralls · 2017-04-24T20:09:42Z

Coverage decreased (-12.9%) to 68.776% when pulling 60b4afd on feature/scripterService into 06eddd5 on dev.

coveralls · 2017-04-24T22:06:51Z

Coverage increased (+8.6%) to 76.134% when pulling aeffd07 on feature/scripterService into e65699e on dev.

pensivebrian added 17 commits March 11, 2017 20:10

Initial commit of the scripting service.

2e6813a

The scripting service uses the SqlScriptPublishModel to drive scripting operations.

Add list object and script object support to the scripter service.

9e2dfe3

Merge dev branch

5b2f86b

Refresh SMO from SSMS_Rel 14.0.17049.0

b6e5c59

Add scripting include/exclude object filter support

9d9438a

Fix null ref in ScriptingObject.GetHashCode()

dba5ed5

Log object being scripted

81ea9ea

Refactor scripting operation to lazily load the server name from SMO

4fe577b

merging dev

c1ce933

Format ScriptingOptions.cs

47db0c9

Reformat some of the scripting operations

1c38721

Merge dev

5d42466

Bump SMO version in doc samples

562c693

Run srgen and fix build and test failures

6024b0e

Rename the ScriptingService method from HandleExecuteRequest to Handl…

4a2a965

…eScriptExecuteRequest

pensivebrian assigned benrr101, kburtram, MrMeemus, kevcunnane and llali Apr 14, 2017

pensivebrian requested review from kburtram, kevcunnane and llali April 14, 2017 21:59

msftclas added the cla-not-required label Apr 14, 2017

kevcunnane reviewed Apr 14, 2017

View reviewed changes

kevcunnane reviewed Apr 15, 2017

View reviewed changes

pensivebrian added 3 commits April 14, 2017 19:41

Add docs to ScriptingObjectMatchProcessor

9be1640

Introduce a Cleanup method for the TestDriver.Tests

ce48e89

After all tests run, if a static method "Cleanup" exists on a type, it's invoked so that class level test resources, such as a database, can be deleted.

Actually reuse the same Northwind database for all scripting tests

83afb81

kburtram previously requested changes Apr 15, 2017

View reviewed changes

pensivebrian added 2 commits April 19, 2017 10:37

Misc code review fixes and docs

8771009

Merging dev

327c659

benrr101 previously requested changes Apr 19, 2017

View reviewed changes

pensivebrian added 5 commits April 19, 2017 18:13

Code review feedback

7f1b685

1) Keep the RequestContext isolated to the service 2) Combine completion/error/cancel events 3) Misc code review fixes

Misc Fixes

4ad88e9

1) Cancellation logic moved into base class 2) Refactored sending events from the scripting service 3) Make all tests pass

Use resources for string and rename ScriptingListObjectsCompleteParam…

8df5a61

…eters

Fix test ScriptSchemaInvalidConnectionString and shorten a few test t…

a1e56de

…imeouts

User nullable bools for the ScriptOptions properties

a596f54

pensivebrian mentioned this pull request Apr 21, 2017

Merge and use official SqlToolsService for scripting microsoft/mssql-scripter#43

Closed

Merge branch 'dev' into feature/scripterService

56f6977

kevcunnane reviewed Apr 21, 2017

View reviewed changes

kevcunnane approved these changes Apr 21, 2017

View reviewed changes

pensivebrian added 2 commits April 21, 2017 14:00

Doc update for ScriptingObjectMatchProcessor

d7bbd85

Rename ScriptingObjectMatchProcessor to ScriptingObjectMatcher

60b4afd

Merge branch 'dev' into feature/scripterService

aeffd07

pensivebrian merged commit 4aac4a4 into dev Apr 24, 2017

benrr101 deleted the feature/scripterService branch April 25, 2017 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scripting API implemented by the SqlScriptPublishModel #316

Add scripting API implemented by the SqlScriptPublishModel #316

pensivebrian commented Apr 14, 2017 •

edited

msftclas commented Apr 14, 2017

MrMeemus commented Apr 14, 2017

kevcunnane Apr 14, 2017 •

edited by pensivebrian

pensivebrian Apr 18, 2017

kevcunnane Apr 14, 2017 •

edited by pensivebrian

pensivebrian Apr 15, 2017

kevcunnane left a comment

coveralls commented Apr 15, 2017

kburtram left a comment

coveralls commented Apr 17, 2017

benrr101 left a comment

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 20, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

benrr101 Apr 19, 2017 •

edited by pensivebrian

pensivebrian Apr 19, 2017

coveralls commented Apr 19, 2017

kevcunnane Apr 21, 2017 •

edited by pensivebrian

kevcunnane left a comment

coveralls commented Apr 24, 2017

coveralls commented Apr 24, 2017

Add scripting API implemented by the SqlScriptPublishModel #316

Add scripting API implemented by the SqlScriptPublishModel #316

Conversation

pensivebrian commented Apr 14, 2017 • edited

msftclas commented Apr 14, 2017

MrMeemus commented Apr 14, 2017

kevcunnane Apr 14, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevcunnane Apr 14, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevcunnane left a comment

Choose a reason for hiding this comment

coveralls commented Apr 15, 2017

kburtram left a comment

Choose a reason for hiding this comment

coveralls commented Apr 17, 2017

benrr101 left a comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benrr101 Apr 19, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 19, 2017

kevcunnane Apr 21, 2017 • edited by pensivebrian

Choose a reason for hiding this comment

kevcunnane left a comment

Choose a reason for hiding this comment

coveralls commented Apr 24, 2017

coveralls commented Apr 24, 2017

pensivebrian commented Apr 14, 2017 •

edited

kevcunnane Apr 14, 2017 •

edited by pensivebrian

kevcunnane Apr 14, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

benrr101 Apr 19, 2017 •

edited by pensivebrian

kevcunnane Apr 21, 2017 •

edited by pensivebrian