[GitHub] [calcite-avatica-go] F21 commented on a change in pull request #54: [CALCITE-4174] avatica-go should handle complex/long URLs (josiahg)

[GitBox <gi...@apache.org>]

F21 commented on a change in pull request #54:
URL: https://github.com/apache/calcite-avatica-go/pull/54#discussion_r470292931



##########
File path: dsn.go
##########
@@ -207,7 +207,8 @@ func ParseDSN(dsn string) (*Config, error) {
 	}
 
 	if parsed.Path != "" {
-		conf.schema = strings.TrimPrefix(parsed.Path, "/")
+		s := strings.Split(parsed.Path, "/")
+		conf.schema = s[len(s)-1]

Review comment:
       @joshelser Good catch! Yes, that is pretty subtle and could be difficult to debug. I think there should not be a trailing slash after the schema (if provided) as this is the convention used by a few drivers I checked. This is the case for the MySQL driver you've linked as well as the postgres driver: https://pkg.go.dev/github.com/lib/pq?tab=doc
   
   In other words, `https://some.host/proxied/avatica` means the default schema is `avatica` and `https://some.host/proxied/avatica/` means no default schema. As you mentioned, this is definitely a subtle difference, but at least we are following convention, so there's consistency across drivers.
   
   I am not opposed to moving the default schema to a query string, but it seems to be a break from the standard convention and I don't think it's really necessary. In addition, that would be a breaking change as well, so if we do go down this path, we'd have to consider if we're breaking anything else.
   
   I think the `FormatDSN()` method is a good idea. We can change the properties in the [`Config`](https://godoc.org/github.com/apache/calcite-avatica-go#Config) struct to public (breaking change, but additive) and add a `FormatDSN` method to turn the config back to the DSN.
   
   The above probably sounds a bit rambly, but I personally prefer the following approach:
   - `https://some.host/proxied/avatica` means the schema is `avatica` and `https://some.host/proxied/avatica/` means no schema.
   - Let's make the properties in `Config` public and add a `FormatDSN()` method to it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org