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

Replica.Restore will not read from DB.path #233

Closed
colin-sitehost opened this issue Sep 30, 2021 · 1 comment · Fixed by #239
Closed

Replica.Restore will not read from DB.path #233

colin-sitehost opened this issue Sep 30, 2021 · 1 comment · Fixed by #239

Comments

@colin-sitehost
Copy link
Contributor

colin-sitehost commented Sep 30, 2021

currently the godoc for Replica.Restore and RestoreOptions.OutputPath intimate that if OutputPath == "", then Replica.db.path will be used:

package litestream // import "github.com/benbjohnson/litestream"

func (r *Replica) Restore(ctx context.Context, opt RestoreOptions) (err error)
    Replica restores the database from a replica based on the options given.
    This method will restore into opt.OutputPath, if specified, or into the DB's
    original database path. It can optionally restore from a specific replica or
    generation or it will automatically choose the best one. Finally, a
    timestamp can be specified to restore the database to a specific
    point-in-time.

type RestoreOptions struct {
    // Target path to restore into. If blank, the original DB path is used.
    OutputPath string

    // ... other fields elided ...
}

this is not what happens today. it looks like this logic was introduced in fb80bc1, which contains both the wording and its contradictory behaviour, as it exists today. if this wording was unintentional, I would suggest removing it from both godocs, otherwise this diff (or something similar) should resolve the issue: (provided here due to your contribution policy)

diff --git a/replica.go b/replica.go
index 0cfc21d..67c1eb9 100644
--- a/replica.go
+++ b/replica.go
@@ -955,7 +955,10 @@ func (r *Replica) CalcRestoreTarget(ctx context.Context, opt RestoreOptions) (ge
 func (r *Replica) Restore(ctx context.Context, opt RestoreOptions) (err error) {
        // Validate options.
        if opt.OutputPath == "" {
-               return fmt.Errorf("output path required")
+               if r.db.path == "" {
+                       return fmt.Errorf("output path required")
+               }
+               opt.OutputPath = r.db.path
        } else if opt.Generation == "" && opt.Index != math.MaxInt32 {
                return fmt.Errorf("must specify generation when restoring to index")
        } else if opt.Index != math.MaxInt32 && !opt.Timestamp.IsZero() {
@benbjohnson
Copy link
Owner

@colin-sitehost Thanks for the bug report. I've updated the contribution policy to allow for bug fixes. Feel free to submit a PR or I can update the code myself. The main branch is currently in flux so it's best to use the latest release version instead (v0.3.5).

colin-sitehost added a commit to colin-sitehost/litestream that referenced this issue Oct 3, 2021
Per the godoc on Replica.Restore and RestoreOptions.OutputPath,
Replica.db.path should be used when RestoreOptions.OutputPath is empty.

Fixes benbjohnson#233
benbjohnson pushed a commit that referenced this issue Oct 6, 2021
Per the godoc on Replica.Restore and RestoreOptions.OutputPath,
Replica.db.path should be used when RestoreOptions.OutputPath is empty.

Fixes #233
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 a pull request may close this issue.

2 participants