Proposal: clean up connection_details.go

[sio4]

Hello, While I am looking in the file connection_details.go several times, I think it was originally born to be store common connection information structure and related functions like parsing URL. But since there are some kind of differences on each dialect, now it mixed with some switchs and if buffalo and pop grow more, it will become more complex.

So in my opinion, all things depend on specific database engines should be moved to its dialect file and connection_details.go should stay with common things. (of course, I also guess dialect_*.go was born to support differences of something like SQL syntaxes basically, I think we can just treat it as drivers.)

@markbates, and other maintainers,
If you agree with my idea, I will start to clean up the codes. Today's idea is, just simply,

Ideas

• Move all database-specific things to its dialect.
  • There are only default values of Port and MySQL specific URL parser.
• Fix ConnectionDetails structure and configurations to use Options more.
  • Encoding is used my MySQL only. It can be moved under Options and,
  • Write common Options handling function in connection_details.go (something like I PRed for cockroach)
  • For special options for stability, like multiStatement for MySQL, shoud handled by function that handle default values for each dialect, with standard form.
• Keep compatibility but some can be deprecated in future. (something like Encoding)

I think above changes are good starting point of the change.
How do you think about this?

[stanislas-m]

Looks good to me. I'll check your PR by the end of the week. :)

[sio4]

How about adding test like this?

func Test_MySQL_DB_Operations(t *testing.T) {
	r := require.New(t)

	err := LoadConfigFile()
	r.NoError(err)
	testFor := os.Getenv("SODA_DIALECT")
	if "mysql" != testFor {
		return // not my turn
	}
	if "on" != os.Getenv("POP_EXTRA_TEST") {
		t.Log("Skip extra DDL tests for mysql.")
		t.Log("POP_EXTRA_TEST=on if you want to run extra tests.")
		return
	}
	t.Logf("Current SODA_DIALECT is %v. Test more...\n", testFor)
	connection := Connections[testFor]
	r.NotNil(connection, "oops! doing extra tests but could not found connection!")
	t.Logf("Use connection: %v\n", Connections[testFor])

	testDB := "pop_test_mysql_extra"
	cd := connection.Dialect.Details()
	cd.Database = testDB
	d := connection.Dialect

	d.DropDB()
	err = d.CreateDB()
	r.NoError(err)
	err = d.CreateDB()
	r.Error(err)
	err = d.DropDB()
	r.NoError(err)
}

Although before testing, we do ./tsoda ... which is use those functions, I think we can add extra tests for DDL functions such as CreateDB, TruncateAll, LoadSchema,... and it will help to catch some problems.

By the way, is there any reason for did not connect to any test coverage management tools?

[tombiscan]

Great work with this lib everyone!

Love the idea of cleanup. "Since there are some kind of differences on each dialect" bit got me, when I tried to deploy to Google Cloud SQL (PostgreSQL).

PostgreSQL accepts connection string that does not contain the URL format expected by Pop. App Engine Flex and the new App Engine Standard are both expecting that format when trying to connect to Cloud SQL instance (due to the Unix domain socket).

Example: user=USER password=PASSWORD host=/cloudsql/PROJECT_ID:REGION_ID:INSTANCE_ID/ dbname=DB_NAME

Related issue: #53

Currently, Pop is trying to parse the URL:

pop/connection_details.go

Line 57 in 2826150

u, err := url.Parse(ul)

which makes the use of connection string format impossible.

As a quick workaround, I managed to get around that by adding the condition:
deviseops@470f311

Hopefully, the cleanup would be able to address that issue more nicely.

[sio4]

Hi @B-Scan,
I agree with your comments. And I also feel so good with this well-structured project!

Basically, I consider about adding additional database engines probably added in the future and some variations of existing database engines as you mentioned GCP edition of Postgres, and more.

I think the first goal for this is that if we need to add an additional database engine, it should be adding a single file, not modifying others like connection_details.go, to maintain it independently. (When I have tried to add another version of MySQL dialect, I've found it is somewhat complicated since there are some more files that need to be modified.)

And I also think we need to consider variations. It can be different versions of the same engine, or differently implemented services using the same database engine. To solve this issue, I added ServerInfo structure in the dialect of Cockroach experimentally. With this information structure, we can handle the same request(function call) with slightly different ways for each variation in a single dialect.

Although this part is not the main body of the library, it can help to make the base more stable.

[stanislas-m]

This should be done now, thanks! :)