fixing namespace seeding issue #241
Conversation
nitro-neal
requested review from
decentralgabe and
andresuribe87
as code owners
pkg/service/credential/service.go
Outdated
| @@ -220,7 +220,7 @@ func (s Service) CreateCredential(request CreateCredentialRequest) (*CreateCrede | |||
| func getStatusListCredential(s Service, issuerID string, schemaID string) (*credential.VerifiableCredential, error) { | |||
| storedStatusListCreds, err := s.storage.GetStatusListCredentialsByIssuerAndSchema(issuerID, schemaID) | |||
| if err != nil { | |||
| return nil, util.LoggingNewErrorf("problem with getting status list credential for issuer: %s schema: %s", issuerID, schemaID) | |||
| logrus.Warnf("problem with getting status list credential for issuer: %s schema: %s. Continuing because this could be first time accessing this pair...", issuerID, schemaID) | |||
There was a problem hiding this comment.
logrus.WithError(err).Warnf(...
There was a problem hiding this comment.
If you see ReadPrefix and ReadAllKeys we return an error; however, in Read and in ReadAll we do not.
I think all of these should not return an error and instead return a warning.
Similarly, for deletes we may want to change to a warning as well.
Edit: Neal I see you mentioned this too. Sorry, missed that.
Heavy +1 to this. |
| // TODO: (Neal) there is a current bug with our Bolt implementation where if we do a GET without anything in the db it will throw an error | ||
| // Doing initial writes and then deleting will "warm up" our database and when we do a GET after that it will not crash and return empty list | ||
| // https://github.com/TBD54566975/ssi-service/issues/176 | ||
| if err := db.Write(credentialNamespace, fakeKey, nil); err != nil { | ||
| return nil, util.LoggingErrorMsg(err, "problem writing status initial write to db") | ||
|
|
||
| } | ||
| if err := db.Delete(credentialNamespace, fakeKey); err != nil { | ||
| return nil, util.LoggingErrorMsg(err, "problem with initial delete to db") | ||
| } | ||
|
|
||
| if err := db.Write(statusListCredentialNamespace, fakeKey, nil); err != nil { | ||
| return nil, util.LoggingErrorMsg(err, "problem writing status initial write to db") | ||
| } | ||
|
|
||
| if err := db.Delete(statusListCredentialNamespace, fakeKey); err != nil { | ||
| return nil, util.LoggingErrorMsg(err, "problem with initial delete to db") | ||
| } | ||
|
|
pkg/storage/bolt.go
Outdated
| errMsg := fmt.Sprintf("namespace<%s> does not exist", namespace) | ||
| logrus.Error(errMsg) | ||
| return errors.New(errMsg) | ||
| logrus.Infof("namespace<%s> does not exist", namespace) |
There was a problem hiding this comment.
I don't think we should log on this since it's expected behavior.
Instead, a test can detect whether the implementation is correct. Is there a unit test covering this behavior?
There was a problem hiding this comment.
Yup agree,
I have created unit tests and removed this log
| @@ -141,7 +136,7 @@ func (b *BoltDB) ReadAllKeys(namespace string) ([]string, error) { | |||
| err := b.db.View(func(tx *bolt.Tx) error { | |||
| bucket := tx.Bucket([]byte(namespace)) | |||
| if bucket == nil { | |||
| return util.LoggingNewErrorf("namespace<%s> does not exist", namespace) | |||
| return nil | |||
There was a problem hiding this comment.
I would keep the error as a warn or info
Currently we do an empty write to the credential bolt db to get around a bug where if you do a read before anything is written it will throw an error
it's because the namespace bolt is using is created on first write. we should open up another issue to address it.
solutions:
#176
I have handled this error by logic in the code.
If we think a better solution would be to NOT return an error if the bucket / namespace does not exist I am happy to implement that fix instead of this.