Round out --use scenario (add "use" sub-command, add local file, add .bacpac / .mdf & .ldf / compressed file support) #319

stuartpa · 2023-04-06T10:28:57Z

DRAFT: Not ready for code review yet.

This PR rounds out the "use" scenario for containers, to allow the casual user who doesn't know containers/linux well, to be able to get going quickly with already existing databases:

--use now supports local and remote files
sqlcmd create mssql --use [<local file>|<remote file>]

The T/SQL database name by default becomes the --use file name (without extension). This can be overridden with a ",", e.g.

sqlcmd create mssql --use https://aka.ms/AdventureWorksLT.bak,adventure_works

This will result in the restored database being named [adventure_works] instead of [AdventureWorksLT]

--use now supports .mdf/.bacpac and .bak files
sqlcmd create mssql --use [.mdf | .bacpac | *.bak]
--use now supports compressed file formats (this also allows a .mdf/.ldf file pair)
sqlcmd create mssql --use [.7z | .zip | .tar] # containing any of the above (inc. a .mdf/.ldf pair)
the new sqlcmd use subcommand allows all of the above, to add a database to an already existing container
sqlcmd use [<local file>|<remote file>]

This PR also enables the open sub-command e.g. sqlcmd open ads to be leveraged directly in sqlcmd create, e.g.

sqlcmd create mssql --use https://aka.ms/AdventureWorksLT.bak --open ads --open-file https://aka.ms/AdventureWorksLT.sql

This enables a T/SQL repro (including database and T/SQL script file) to be shared in a single command line

Examples:

sqlcmd install mssql --accept-eula

# Restore a backup (override database name to be [adventure_works], instead of [AdventureWorksLT])
sqlcmd use https://aka.ms/AdventureWorksLT.bak,adventure_works
sqlcmd use c:\Users\alias\Downloads\AdventureWorksLT.bak,adventure_works

# Restore a Dacfx .bacpac
sqlcmd use https://github.com/Microsoft/sql-server-samples/releases/download/wide-world-importers-v1.0/WideWorldImportersDW-Full.bacpac

# Attach a compressed .mdf/.ldf
sqlcmd use https://downloads.brentozar.com/StackOverflow2010.7z

shueybubbles · 2023-04-06T18:29:41Z

What about supporting non-public URLs (SAS etc)? Does it download the file then discover its type similar to how you'd have to examine a zip to know its content?

shueybubbles · 2023-04-06T20:00:42Z

cmd/modern/root/use.go

+
+		endpoint, user := config.CurrentContext()
+
+		c.sql = sql.New(sql.SqlOptions{UnitTesting: false})


UnitTesting: false}

isn't false the default value?
We shouldn't need to have product code give hints about unit testing. Test hooks should be available for tests to inject things.

shueybubbles · 2023-04-06T20:06:07Z

cmd/modern/root/use.go

+		output.FatalfWithHintExamples([][]string{
+			{"Create new context with a sql container ", "sqlcmd create mssql"},
+		}, "Current context does not have a container")
+	}


what will this look like for non-container connections?
We can't claim this is production/enterprise ready until we support that.

I'd have expected most of the code from lines 76 to 96 to be outside the "if it's a container block"

Supporting a UNC or local file path to a bak file is a relatively straightforward restore operation

Restore from URL is also straightforward without copying the file locally if the server has a credential stored for the URL already.

This PR is only for container scenarios (I'll update the PR description). I am not sure if non-container scenarios are needed. (folks already have solutions for all of this). Containers are particularly tricky in this regard for our normal users (who are windows centric). This functionality abstracts most of the users during the early part of their journey from all these internals.

shueybubbles · 2023-04-06T20:11:43Z

cmd/modern/root/use.go

+				"Unable to download file to container")
+		}
+
+		useDatabase.CopyToContainer(id)


CopyToContainer(id)

for .bak files using a URL we should provide a way to create a CREDENTIAL to store on the server so it can restore directly from the URL

SQL 2022 supports both https and s3 URLs #Resolved

Added an issue for this:

#332

shueybubbles · 2023-04-06T20:14:46Z

cmd/modern/root/use.go

+		useDatabase.BringOnline(
+			c.sql.Query,
+			user.BasicAuth.Username,
+			secret.Decode(user.BasicAuth.Password, user.BasicAuth.PasswordEncryption),


can we have a "credential" or "authentication" abstraction associated with the user to pass around instead of explicit user name and password? We're going to need to support all the auth types.

shueybubbles · 2023-04-06T20:44:28Z

internal/container/controller.go

+
+	// Create and add some files to the archive.
+	var buf bytes.Buffer
+	tw := tar.NewWriter(&buf)


tw := tar.NewWriter(&buf)

instead of having tw.Close below, shouldn't there be a "defer tw.Close()" call right after this? Nobody ever error checks a Close afaik

shueybubbles · 2023-04-06T20:53:20Z

internal/uri/type.go

+import "net/url"
+
+type Uri struct {
+	uri string


uri string

what's the benefit of keeping the string around? can't we get it from the URL?

shueybubbles · 2023-04-06T20:54:12Z

internal/uri/uri.go

+func (u Uri) IsLocal() bool {
+	if len(u.Scheme()) > 2 {
+		return false
+	} else {


else {

no else needed here or on line 21

couldn't this just be
return len(u.Scheme()) <= 2

shueybubbles · 2023-04-06T20:57:27Z

internal/uri/uri.go

+
+// actualUrl returns the url without the query string
+func (u Uri) ActualUrl() string {
+	terminator := strings.LastIndex(u.uri, ",")


,

I think the comment is inaccurate. The query string is delimited by ?

shueybubbles · 2023-04-06T21:06:26Z

internal/uri/uri.go

+func (u Uri) Filename() string {
+	filename := filepath.Base(u.ActualUrl())
+	if filename == "" {
+		panic("filename is empty")


panic("filename is empty")

this reliance on file name in the user's URL seems very limiting, even at this phase of our development.
It's fine for the Uri to say "I don't know the file name" then let the consumer decide what to do. The consumer might say "ok let's download it and see what we get" or "fail if the user didn't tell us what mechanism to use".
Presumably if the mechanism is "restore" we can assume it's a "bak", correct?

shueybubbles · 2023-04-06T21:07:28Z

internal/uri/uri.go

+	return ""
+}
+
+func (u Uri) GetDbNameAsIdentifier() string {


etDbNameAsIdenti

these SQL query escaping functions don't belong here.
They should be in their own package for general purpose use to safely inject identifiers into format strings.

shueybubbles · 2023-04-06T21:16:19Z

internal/uri/uri_test.go

+	}
+
+	tests := []test{
+		{"https://example.com/testdb.bak,myDbName", "myDbName"},


{"https://example.com/testdb.bak,myDbName" [](http://example.com/codeflow?start=0&length=44)

It's a bit odd to conflate "URL + a database name" as "Uri"

The fact that --use flag and "use" command take this format as a parameter is specific to the semantic of those commands, correct?
I would expect these strings to be deserialized into something more meaningful like

type DatabaseFile struct { Source *url.URL DatabaseName string }

shueybubbles · 2023-04-06T21:17:57Z

internal/uri/uri_test.go

+		{"https://example.com/test.foo", "test"},
+		{"https://example.com/test.foo,test", "test"},
+		{"https://example.com/test.7z,tql_name", "tsql_name"},
+		{"https://example.com/test.mdf,tsql_name?foo=bar", "tsql_name"},


"https://example.com/test.mdf,tsql_name?foo=bar

how does putting the query portion of the URL after the comma work? It doesn't look like valid input.

A URL with a query would be

https://example.com/test.mdf?version=10,mydatabase

shueybubbles · 2023-04-06T21:27:02Z

pkg/mssqlcontainer/ingest/ingest.go

+		return false
+	} else {
+		return true
+	}


I propose we pick one of three styles to avoid superfluous "else" blocks

1

return i.extractor != nil

2

func (i *ingest) IsExtractionNeeded() needed bool { i.extractor = extract.NewExtractor(i.uri.FileExtension(), i.controller) needed = iExtractor != nil }

3

func (i *ingest) IsExtractionNeeded() bool { i.extractor = extract.NewExtractor(i.uri.FileExtension(), i.controller) if i.extractor == nil { return false } return true }

My vote is 1 .

shueybubbles · 2023-04-06T21:30:30Z

pkg/mssqlcontainer/ingest/ingest.go

+	i.options.Filename = i.uri.Filename()
+
+	if i.options.Filename == "" {
+		panic("filename is empty")


ename is empty")

if the user provides a mechanism but the URL doesn't have a detectable file name, can we just generate a unique name with an extension that matches the mechanism?

shueybubbles · 2023-04-06T21:36:56Z

pkg/mssqlcontainer/ingest/ingest.go

+
+	alterDefaultDb := fmt.Sprintf(
+		"ALTER LOGIN [%s] WITH DEFAULT_DATABASE = [%s]",
+		username,


username,

user names need to be bracket escaped as well.
Fundamentally, we need a package with the same helper functions SMO has for safe scripting. See https://github.com/microsoft/sqlmanagementobjects/blob/54e95cd095e232e43e711e65a83d55514ddc5e0e/src/Microsoft/SqlServer/Management/Smo/SqlSmoObject.cs#L329

shueybubbles · 2023-04-06T21:38:42Z

internal/uri/factory.go

@@ -0,0 +1,17 @@
+package uri


add the copyright header to your new files

shueybubbles · 2023-04-06T21:40:57Z

pkg/mssqlcontainer/ingest/extract/7zip.go

+func (e *sevenZip) IsInstalled(containerId string) bool {
+	e.containerId = containerId
+
+	return false


return false

tbd?

shueybubbles · 2023-04-06T21:42:59Z

pkg/mssqlcontainer/ingest/extract/7zip.go

+		"wget",
+		"-O",
+		"/opt/7-zip/7-zip.tar",
+		"https://7-zip.org/a/7z2201-linux-x64.tar.xz"})


x64

don't we need to get the architecture at runtime depending on where sqlcmd is running?

shueybubbles · 2023-04-06T21:52:08Z

pkg/mssqlcontainer/ingest/extract/7zip.go

+	containerId string
+}
+
+func (e *sevenZip) Initialize(controller *container.Controller) {


Initialize(controller *container.Controller)

How can we abstract away the differences between
"SQL running in a container", "SQL running on the local host", "SQL running in the cloud", and "SQL running on a remote host"?
Or would we have multiple extractor implementations and this one should be renamed "sevenZipInLinuxX64Container" and we'd have others like "windowsZipLocalHost" ?

I don't see how this model evolves to support the non-container scenarios.

shueybubbles · 2023-04-06T21:58:25Z

Could you create a couple mermaid diagrams in the wiki? One would show the relationship between components like ingest and extract and container, and one would show how the contents of the bak/ldf/dacpac file would move from their source to the database for each environment we support (sql in a container, sql on localhost, sql on a remote host, sql in the cloud).

graph TD;
a -- label --> b

shueybubbles · 2023-04-06T22:03:28Z

pkg/mssqlcontainer/ingest/extract/7zip.go

+}
+
+func extractPaths(input string) []string {
+	re := regexp.MustCompile(`Path\s*=\s*(\S+)`)


`Path

i assume this is never localized

shueybubbles · 2023-04-06T22:04:13Z

pkg/mssqlcontainer/ingest/extract/7zip_test.go

+
+	paths := extractPaths(stdout)
+
+	fmt.Println(paths)


add some asserts

shueybubbles · 2023-04-06T22:06:13Z

pkg/mssqlcontainer/ingest/location/factory.go

+			uri:        uri,
+			controller: controller,
+		}
+	} else {


remove else

shueybubbles · 2023-04-06T22:15:45Z

pkg/mssqlcontainer/ingest/mechanism/attach.go

+	m.setFilePermissions(m.CopyToLocation() + "/" + options.Filename)
+	if options.LdfFilename == "" {
+		text += `CREATE DATABASE [%s] ON (FILENAME = '%s/%s') FOR ATTACH;`
+		query(fmt.Sprintf(


query(fmt.Sprintf

you need to escape the parameters properly, and use nvarchar for the file name

eg from SMO

ddl.AppendFormat(SmoApplication.DefaultCulture, ", FILENAME = N'{0}' ", SqlString(fileName)); public static String SqlString(String s) { return EscapeString(s, '\''); }

it'd be a good test case to have some file names that use extended characters. Go stores strings as UTF8 but nvarchar is UTF16, while varchar is SQL-collation-dependent.
How do we guarantee the Go strings get encoded properly in the query?

maybe go-mssqldb does the right thing already for strings and we should use a parameterized query to let the driver do the conversion instead of manually inserting tokens in the query.

shueybubbles · 2023-04-06T22:23:12Z

pkg/mssqlcontainer/ingest/mechanism/options.go

+	Password string
+
+	Filename    string
+	LdfFilename string


LdfFilename string

should this be an array?

shueybubbles · 2023-04-06T22:33:02Z

pkg/mssqlcontainer/ingest/mechanism/bacpac.go

+
+	m.controller.DownloadFile(
+		m.containerId,
+		"https://aka.ms/sqlpackage-linux",


s/sqlpackage-linux",

is sqlpackage CPU architecture agnostic? Does this also bring down Net Core runtime?

Simplifying the getting started README for when this sqlcmd PR is pushed microsoft/go-sqlcmd#319

…o-sqlcmd into stuartpa/sqlcmd-urls

Round out --use scenario

0e6223f

stuartpa force-pushed the stuartpa/sqlcmd-urls branch from b239523 to 0e6223f Compare April 6, 2023 10:38

Adding unit test coverage

f0b9f3b

shueybubbles reviewed Apr 6, 2023

View reviewed changes

internal/uri/factory.go Outdated

@@ -0,0 +1,17 @@

package uri

Copy link

Collaborator

shueybubbles Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the copyright header to your new files

shueybubbles reviewed Apr 6, 2023

View reviewed changes

pkg/mssqlcontainer/ingest/extract/7zip_test.go

paths := extractPaths(stdout)

fmt.Println(paths)

Copy link

Collaborator

shueybubbles Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some asserts

shueybubbles reviewed Apr 6, 2023

View reviewed changes

pkg/mssqlcontainer/ingest/location/factory.go

uri: uri,

controller: controller,

}

} else {

Copy link

Collaborator

shueybubbles Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove else

shueybubbles reviewed Apr 6, 2023

View reviewed changes

dlevy-msft added this to the January 2024 Release milestone Dec 7, 2023

stuartpa added 7 commits December 23, 2023 04:11

Add-ons initial

8b8ef64

Saving changes

9e745c0

Merge conflicts

a1ca145

Merged add-ons

7cf2691

Merged add-ons

ec08c06

Add-on fleet-manager

1b5a4c3

Add-on fleet-manager

7c78a44

stuartpa added a commit to stuartpa/dab-swa-todo that referenced this pull request Dec 29, 2023

Update README.md

9a00d52

Simplifying the getting started README for when this sqlcmd PR is pushed microsoft/go-sqlcmd#319

sqlcmd create --use https://*.git working

d42b514

This was referenced Jan 11, 2024

SQLCMD Create User and Password #490

Open

ADS Login Fails if a container is deleted and a new one is created #432

Open

stuartpa and others added 18 commits February 1, 2024 03:30

sqlcmd deploy

9209cad

Added .sqlcmd/sqlcmd.yaml support

5233a42

Added .sqlcmd/sqlcmd.yaml support

d9b0993

Add azd as a tool

7525053

adding mac support

9282eaa

Tested Mac support

12c2f73

Merge branch 'stuartpa/sqlcmd-urls' of https://github.com/microsoft/g…

cb34e92

…o-sqlcmd into stuartpa/sqlcmd-urls

Mac support

e95442a

Fix Mac issue

7c4472f

Remove customer go-sqlcmddb

e1a6bdb

Remove customer go-sqlcmddb

d4a205e

Add linux stubs

7ffd5e9

Add linux stubs

f26a522

Fix panic when ads not installed

91584e4

Upgrade to go 1.20 required

d6531a1

Upgrade to go 1.20 required

b708505

Fix issue with keyvault not being excluded on non windows os

17093e9

Remove TEMP output if azd init not run

2cc4c97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Round out --use scenario (add "use" sub-command, add local file, add .bacpac / .mdf & .ldf / compressed file support) #319

Round out --use scenario (add "use" sub-command, add local file, add .bacpac / .mdf & .ldf / compressed file support) #319

stuartpa commented Apr 6, 2023 •

edited

Loading

shueybubbles commented Apr 6, 2023 •

edited

Loading

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

stuartpa Apr 21, 2023

shueybubbles Apr 6, 2023 •

edited by stuartpa

Loading

stuartpa Apr 27, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023 •

edited

Loading

shueybubbles Apr 6, 2023 •

edited

Loading

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles commented Apr 6, 2023 •

edited

Loading

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023

shueybubbles Apr 6, 2023


		endpoint, user := config.CurrentContext()

		c.sql = sql.New(sql.SqlOptions{UnitTesting: false})

Round out --use scenario (add "use" sub-command, add local file, add .bacpac / .mdf & .ldf / compressed file support) #319

Are you sure you want to change the base?

Round out --use scenario (add "use" sub-command, add local file, add .bacpac / .mdf & .ldf / compressed file support) #319

Conversation

stuartpa commented Apr 6, 2023 • edited Loading

shueybubbles commented Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shueybubbles Apr 6, 2023 • edited by stuartpa 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

shueybubbles Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

shueybubbles Apr 6, 2023 • 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

shueybubbles commented Apr 6, 2023 • 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

stuartpa commented Apr 6, 2023 •

edited

Loading

shueybubbles commented Apr 6, 2023 •

edited

Loading

shueybubbles Apr 6, 2023 •

edited by stuartpa

Loading

shueybubbles Apr 6, 2023 •

edited

Loading

shueybubbles Apr 6, 2023 •

edited

Loading

shueybubbles commented Apr 6, 2023 •

edited

Loading