feat: Implement CouchDB Store #7

DRK3 · 2020-02-13T16:12:19Z

closes #4
Signed-off-by: Derek Trider [email protected]

codecov · 2020-02-13T16:14:19Z

Codecov Report

Merging #7 into master will decrease coverage by 9.24%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##            master       #7      +/-   ##
===========================================
- Coverage   100.00%   90.75%   -9.25%     
===========================================
  Files            1        2       +1     
  Lines           33      119      +86     
===========================================
+ Hits            33      108      +75     
- Misses           0        6       +6     
- Partials         0        5       +5

Impacted Files	Coverage Δ
pkg/storage/couchdb/couchdbstore.go	`87.20% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f206aa...42630ee. Read the comment docs.

DRK3 · 2020-02-13T23:40:09Z

pkg/storage/store.go

 	OpenStore(name string) (Store, error)

 	// CloseStore closes the store with the given name.
 	CloseStore(name string) error

 	// Close closes all stores created under this store provider.
 	Close() error
+
+	// StoreExists checks whether a store exists under the given name already.
+	StoreExists(name string) (bool, error)


@fqutishat If you recall, I had an issue in the EDV repo where I needed the ability to know if a store is open without actually opening it. The solution we had before was to track the open stores in a map in the operations.go file. However, I realized while working on this story that that strategy wouldn't work for a persistent store, since now there can be stores already created from running this executable previously. Thus, I need to ask the provider if a store is open first. So I added this "StoreExists" method to the Provider interface. Let me know if there's another way to do this.

where you use Provider.StoreExists

Per our discussion, I added the CreateStore method and made the OpenStore method only open existing stores.

DRK3 · 2020-02-13T23:41:45Z

pkg/storage/couchdb/couchdbstore.go

+}
+
+// CloseStore closes a previously opened store.
+// CouchDB and Kivik don't really have "close" mechanisms for individual databases, so there's nothing to do here.


This seems strange... but I couldn't find a way to close a CouchDB database. Is there a way?

Is this what you're looking for?

@llorllale Yep, that's it. The older version of Kivik I used initially didn't have that call, so that's why I thought it didn't exist.

I updated the method.

fqutishat · 2020-02-14T00:09:18Z

pkg/storage/couchdb/couchdbstore_test.go

+
+	"github.com/sirupsen/logrus"
+
+	"github.com/trustbloc/edge-core/pkg/storage"


wrong location

fqutishat · 2020-02-14T00:13:19Z

pkg/storage/couchdb/couchdbstore_test.go

+	couchDBURL    = "localhost:5984"
+	testStoreName = "teststore"
+	testKey       = "sampleDBKey"
+	testValue     = `{"JSONKey":"JSONValue"}`


you assume the value always JSON

@troyronda @bstasyszyn @llorllale @rolsonquadras

Per our discussion on Friday with @bstasyszyn , I updated the CouchDB store Put method so that a non-JSON payload will be saved as an attachment. I verified that JSON payloads do get merged with the CouchDB document (this should answer @bstasyszyn's question from Friday)

fqutishat · 2020-02-14T00:15:56Z

go.mod

+require (
+	github.com/flimzy/diff v0.1.6 // indirect
+	github.com/flimzy/testy v0.1.16 // indirect
+	github.com/go-kivik/couchdb v2.0.0+incompatible


why it's incompatible

I don't think we have a choice. The go-kivik module has the same and (i believe) go mod tidy will put it back if you try to take it out.

@fqutishat The "incompatible" thing has something to do with the way the go modules are set up in the Go Kivik library. I don't think I can do anything about it until they update it. According to what I've read it shouldn't have any impact on us though. See https://stackoverflow.com/questions/57355929/what-incompatible-in-go-mod-mean-will-it-make-harm for more info

The author of the Go Kivik repo has an open issue for go.mod support, so I think when that's resolved then this "incompatible" thing will go away (after we update the dependency of course): go-kivik/kivik#417

fqutishat · 2020-02-14T00:50:12Z

pkg/storage/couchdb/couchdbstore.go

+
+// Put stores the given key-value pair in the store.
+func (c *CouchDBStore) Put(k string, v []byte) error {
+	_, err := c.db.Put(context.Background(), k, v)


you need to create doc and create key inside doc called data

doc := map[string]interface{}{
"_id": k,
"data":v
}

@bstasyszyn @troyronda @llorllale @rolsonquadras

Do we ever want to save the value as an attachment?

https://github.com/trustbloc/fabric-peer-ext/blob/e486262f7eafc9247351e6f683885c627b829e80/pkg/collections/offledger/storeprovider/store/couchdbstore/dbstore.go#L249

Per our discussion on Friday with @bstasyszyn , I updated the CouchDB store Put method so that a non-JSON payload will be saved as an attachment. I verified that JSON payloads do get merged with the CouchDB document (this should answer @bstasyszyn's question from Friday)

fqutishat · 2020-02-14T00:51:11Z

pkg/storage/couchdb/couchdbstore.go

+
+// Put stores the given key-value pair in the store.
+func (c *CouchDBStore) Put(k string, v []byte) error {
+	_, err := c.db.Put(context.Background(), k, v)


@bstasyszyn @troyronda @llorllale @rolsonquadras

fqutishat · 2020-02-14T00:53:55Z

pkg/storage/couchdb/couchdbstore.go

+}
+
+// Get retrieves the value in the store associated with the given key.
+func (c *CouchDBStore) Get(k string) ([]byte, error) {


when you retrieve value from couchdb you don't need to return the whole doc just return doc["data"]

llorllale · 2020-02-14T01:28:03Z

pkg/storage/couchdb/couchdbstore.go

+
+// OpenStore opens and returns a store for the given name.
+func (p *Provider) OpenStore(name string) (storage.Store, error) {
+	exists, err := p.couchDBClient.DBExists(context.Background(), name)


why are we still doing this? This stuff gave us lots of headaches in the past. The solution back then was to have a "leader" create the DB if it doesn't exist.

Why isn't this a devops issue?

llorllale · 2020-02-14T01:32:19Z

pkg/storage/couchdb/couchdbstore.go

+
+// Close closes the provider.
+func (p *Provider) Close() error {
+	p.couchDBClient = nil


Client.Close()?

@llorllale The older version of Kivik I used initially didn't have that call, so that's why I didn't use it before. I see the newer one does - I've updated the method.

bstasyszyn · 2020-02-14T13:12:00Z

go.mod

+require (
+	github.com/flimzy/diff v0.1.6 // indirect
+	github.com/flimzy/testy v0.1.16 // indirect
+	github.com/go-kivik/couchdb v2.0.0+incompatible


I don't think we have a choice. The go-kivik module has the same and (i believe) go mod tidy will put it back if you try to take it out.

bstasyszyn · 2020-02-14T13:22:56Z

pkg/storage/couchdb/couchdbstore.go

+
+	err = p.couchDBClient.CreateDB(context.Background(), name)
+	if err != nil {
+		return nil, err


There could be a race condition in which two clients are attempting to create the same DB. At the time you check if the DB exists it may not be there but then when you try to create it you'll get an error because it exists. You'll need to check again if the error means that the DB is there.

The code has been changed so now there are separate create store and open store methods (per our discussion with @fqutishat on Friday), so the DB exists check is no longer there. Let me know if you still think there's a race condition.

bstasyszyn · 2020-02-14T13:45:53Z

pkg/storage/couchdb/couchdbstore.go

+
+// Put stores the given key-value pair in the store.
+func (c *CouchDBStore) Put(k string, v []byte) error {
+	_, err := c.db.Put(context.Background(), k, v)


Not sure if it's a good idea to always use context.Background(). Usually it's a good idea to expose context in the API to allow the client to pass whatever type of context they want (i.e. with timeout, cancel, etc.)

@bstasyszyn @fqutishat So would this mean adding a third argument to the Put() function for context? Or should I perhaps add it in to the Store/Provider struct and then grab the context whenever I need it? Just wondering what's the best way.

The context is for one time use, i.e. per request. So it would need to be in Put. The convention is to add it as the first arg.

Per our discussion, I created a follow-up issue for this: #8

bstasyszyn · 2020-02-14T13:47:04Z

pkg/storage/couchdb/couchdbstore.go

+
+// Put stores the given key-value pair in the store.
+func (c *CouchDBStore) Put(k string, v []byte) error {
+	_, err := c.db.Put(context.Background(), k, v)


Do we ever want to save the value as an attachment?

fqutishat · 2020-02-18T05:16:54Z

scripts/check_unit.sh

+  exit 1
+fi
+
+DOCKER_RM_EXIT_CODE=0


duplicate code

I refactored the error code check into a separate function. Take a look and let me know what you think.

llorllale · 2020-02-18T11:58:04Z

pkg/storage/couchdb/couchdbstore.go

+// We want to do it all in one step, hence this manual stuff below.
+func wrapTextAsCouchDBAttachment(textToWrap []byte) []byte {
+	encodedTextToWrap := base64.StdEncoding.EncodeToString(textToWrap)
+	return []byte(`{"_attachments": {"data": {"data": "` + encodedTextToWrap + `", "content_type": "text/plain"}}}`)


will these need indexing?

No, they shouldn't. This whole attachment thing is just to allow the CouchDB store to work with non-JSON payloads. The primary use case for the CouchDB store is to store EDV EncryptedDocuments, which are JSON. Those EncryptedDocuments will need indexing, but they won't be stored as attachments, so we should be okay.

bstasyszyn · 2020-02-18T12:48:26Z

pkg/storage/couchdb/couchdbstore.go

+
+	store := &CouchDBStore{db: db}
+
+	p.dbs[name] = store


You need a write lock here.

After looking through the OpenStore method, I think that I actually need a write lock for the whole method since it first checks to see if it exists, and then opens the store in the map. Let me know if you think I'm locking more than I need to here.

bstasyszyn · 2020-02-18T12:48:55Z

pkg/storage/couchdb/couchdbstore.go

+}
+
+func (p *Provider) storeExists(name string) (bool, error) {
+	_, existsInCache := p.dbs[name]


Needs a read lock. (Also in other places below.)

Since I added a write lock to the OpenStore method (which is the only place that calls the private method storeExists), I don't need to (and can't) add a read lock here. Let me know if this seems ok.

bstasyszyn · 2020-02-18T12:54:53Z

pkg/storage/couchdb/couchdbstore.go

+		return nil, dbErr
+	}
+
+	store := &CouchDBStore{db: db}


If the store exists in cache then why not return it instead of creating a new one every time?

bstasyszyn · 2020-02-19T13:48:36Z

pkg/storage/couchdb/couchdbstore.go

+
+// Put stores the given key-value pair in the store.
+func (c *CouchDBStore) Put(k string, v []byte) error {
+	c.mux.Lock()


You don't need a lock here since you're accessing an immutable field.

Done. (I also got rid of that line in the close method that clears the cache by remaking the map, as discussed)

bstasyszyn · 2020-02-19T13:49:09Z

pkg/storage/couchdb/couchdbstore.go

+func (c *CouchDBStore) Get(k string) ([]byte, error) {
+	destinationData := make(map[string]interface{})
+
+	c.mux.RLock()


Don't need a lock.

bstasyszyn · 2020-02-19T13:55:14Z

pkg/storage/couchdb/couchdbstore.go

+	}
+
+	// Check cache first
+	store, exists := p.dbs[name]


Seems a bit awkward that you check if the store exists using storeExists and then you check again if it exists here. Maybe it would be better to get rid of storeExists, unless it's being used elsewhere?

Signed-off-by: Derek Trider <[email protected]>

DRK3 self-assigned this Feb 13, 2020

cla-bot bot added the cla-signed label Feb 13, 2020

DRK3 force-pushed the CouchDB branch 4 times, most recently from bf47165 to 7bda9e0 Compare February 13, 2020 22:13

DRK3 marked this pull request as ready for review February 13, 2020 22:13

DRK3 force-pushed the CouchDB branch 2 times, most recently from 4efa717 to b4b2524 Compare February 13, 2020 23:22

DRK3 commented Feb 13, 2020

View reviewed changes

fqutishat reviewed Feb 14, 2020

View reviewed changes

fqutishat requested review from bstasyszyn, llorllale, rolsonquadras and troyronda February 14, 2020 00:16

fqutishat reviewed Feb 14, 2020

View reviewed changes

fqutishat suggested changes Feb 14, 2020

View reviewed changes

fqutishat reviewed Feb 14, 2020

View reviewed changes

llorllale reviewed Feb 14, 2020

View reviewed changes

bstasyszyn reviewed Feb 14, 2020

View reviewed changes

DRK3 force-pushed the CouchDB branch 5 times, most recently from 1cbd5e6 to 0cd563e Compare February 14, 2020 22:44

DRK3 mentioned this pull request Feb 17, 2020

Allow different contexts to be used in edge-core CouchDB store #8

Open

DRK3 force-pushed the CouchDB branch 10 times, most recently from 3be481a to ac557e0 Compare February 18, 2020 05:07

fqutishat reviewed Feb 18, 2020

View reviewed changes

fqutishat self-requested a review February 18, 2020 05:20

llorllale reviewed Feb 18, 2020

View reviewed changes

bstasyszyn reviewed Feb 18, 2020

View reviewed changes

DRK3 force-pushed the CouchDB branch 4 times, most recently from a60f216 to 599441b Compare February 18, 2020 21:53

bstasyszyn reviewed Feb 19, 2020

View reviewed changes

feat: Implement CouchDB Store

42630ee

Signed-off-by: Derek Trider <[email protected]>

DRK3 force-pushed the CouchDB branch from 599441b to 42630ee Compare February 19, 2020 16:14

bstasyszyn approved these changes Feb 19, 2020

View reviewed changes

fqutishat approved these changes Feb 19, 2020

View reviewed changes

fqutishat merged commit a8e4e00 into trustbloc:master Feb 19, 2020


		"github.com/sirupsen/logrus"

		"github.com/trustbloc/edge-core/pkg/storage"

feat: Implement CouchDB Store #7

feat: Implement CouchDB Store #7

Conversation

DRK3 commented Feb 13, 2020

codecov bot commented Feb 13, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

fqutishat Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DRK3 Feb 13, 2020 • 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

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

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

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

DRK3 Feb 18, 2020 • 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

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

codecov bot commented Feb 13, 2020 •

edited

Loading

fqutishat Feb 14, 2020 •

edited

Loading

DRK3 Feb 13, 2020 •

edited

Loading

DRK3 Feb 18, 2020 •

edited

Loading