Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(couchdb): use url.PathEscape in DBExists for consistency with other DB* methods #1041

Merged

Conversation

dominicbarnes
Copy link
Contributor

Currently, url.PathEncode is applied for CreateDB and DestroyDB, but not for DBExists, which behave inconsistently.

I discovered this when I created a database that included special characters, but DBExists would not correctly detect a database even after being created.

For a reproducible example, I could not find a good test case to update, as the request path is never part of the test assertion. You can reproduce the issue with the following setup:

docker-compose.yml

services:
  couchdb:
    image: couchdb:3
    ports:
      - 5984:5984
    environment:
      COUCHDB_USER: admin
      COUCHDB_PASSWORD: password

go.mod

module kivik-testing

go 1.23.0

require github.com/go-kivik/kivik/v4 v4.3.0

require (
	github.com/google/uuid v1.6.0 // indirect
	golang.org/x/net v0.17.0 // indirect
	golang.org/x/sync v0.4.0 // indirect
)

main.go

package main

import (
	"context"
	"log"
	"net/http"
	"net/url"

	"github.com/go-kivik/kivik/v4"
	_ "github.com/go-kivik/kivik/v4/couchdb"
)

func main() {
	ctx := context.Background()

	couchdb, err := kivik.New("couch", "http://admin:password@localhost:5984")
	if err != nil {
		log.Fatal(err)
	}

	dbName := "foo/bar"

	// ensure database is created
	if err := couchdb.CreateDB(ctx, dbName); err != nil {
		switch kivik.HTTPStatus(err) {
		case http.StatusPreconditionFailed:
			// do nothing, database already exists
		default:
			log.Fatalf("failed to create database %s: %s", dbName, err)
		}
	}

	// check existence with existing kivik
	exists, err := couchdb.DBExists(ctx, dbName)
	if err != nil {
		log.Fatalf("failed to check database %s existence", dbName)
	}
	log.Printf("exists (got %t, expected true)", exists)

	// check existince with hack
	exists, err = couchdb.DBExists(ctx, url.PathEscape(dbName))
	if err != nil {
		log.Fatalf("failed to check database %s existence", dbName)
	}
	log.Printf("exists with hack (got %t, expected true)", exists)
}

Upon running my "test" script, I get the following output:

$ go run .
exists (got false, expected true)
exists with hack (got true, expected true)

This PR includes a fix, which is to use url.PathEscape within DBExists which mirrors CreateDB and DestroyDB and will produce consistent/correct behavior.

It would be nice to validate this fix with a test case, but I don't think the current test suite really has this kind of assertion baked in (it kinda assumes the right URL path is used, since it just returns the configured *http.Response no matter what the request).

@flimzy
Copy link
Member

flimzy commented Aug 21, 2024

Thank you for catching this and submitting a patch! It would be good to add a test, as well, which would fail prior to the patch, but pass after. Is that something you'd like to add?

@dominicbarnes
Copy link
Contributor Author

@flimzy yeah, let me throw something together

@dominicbarnes
Copy link
Contributor Author

@flimzy I just pushed bb18c2a which adds a test that fails before the change and passes after.

To avoid altering any other tests, this creates a new test helper which creates a client which inspects req.URL.RawPath as part of the HTTP mock, returning an error if there is a mismatch.

This probably isn't the most elegant approach, but it certainly gets the job done short of pulling in a full HTTP mocking library. Let me know what you think, I'm happy to iterate on it further if you have any suggestions.

@dominicbarnes
Copy link
Contributor Author

dominicbarnes commented Aug 21, 2024

@flimzy after looking over some other tests, I found some other examples of inspecting req.URL via newCustomClient, so I've ditched the one-time helper in 8c9b10a

I think this way is more consistent with the rest of the existing tests, so let me know what you think.

@flimzy flimzy merged commit 9e790c4 into go-kivik:master Aug 21, 2024
@flimzy
Copy link
Member

flimzy commented Aug 21, 2024

Looks great. Thanks for the contribution! I'll roll a patch release shortly.

@dominicbarnes dominicbarnes deleted the couchdb-db-exists-url-path-encode branch August 21, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants