rqlite / gorqlite

A Go client for rqlite, the distributed database built on SQLite
MIT License
149 stars 35 forks source link

feat: context support, pointer of connection, exported ErrClosed, parameterized queries #25

Closed aldy505 closed 1 year ago

aldy505 commented 2 years ago

Sorry for such a big git diff. But anyway, these are the changes that I made:

  1. gorqlite.Open() now returns a pointer of Connection, this may break some users' experience. But it's only a pointer away. Every functionality are the same.
  2. Addition of QueryContext, QueryOneContext, QueueContext, QueueOneContext, WriteContext, and WriteOneContext method.
    • Update 2022-09-18: Additional QueryOneParameterizedContext, QueryParameterizedContext, WriteOneParameterizedContext, WriteParameterizedContext, QueueOneParameterizedContext, and QueueParameterizedContext based on #26.
  3. Comment-style from /* */ to // to comply with how go doc generates its' documentation. So, every documented code will looks good on https://pkg.go.dev/github.com/rqlite/gorqlite
  4. errClosed = errors.New("gorqlite: connection is closed") is now exported as ErrClosed, so users can do:
    func main() {
      qr, err := conn.QueryContext(ctx, "some query...")
      if err != nil {
        if errors.Is(err, gorqlite.ErrClosed) {
          // do a retry because connection is closed
        }
      }
    }
  5. (Removed, handled by #26) Parameterized query is now supported, and it doesn't break the current API.

    Old code spoiler ```go func main() { rows, err := conn.QueryContext(ctx, "SELECT name FROM table WHERE id = ?", 12) // do things... // and this also works secondRows, err := conn.QueryContext(ctx, "SELECT name FROM table WHERE id = 12") // also with Write() and Queue() methods (and their similar methods) _, err = conn.WriteContext(ctx, "INSERT INTO table (id, name) VALUES (?, ?)", 12, "James") _, err = conn.QueueContext(ctx, "INSERT INTO table (id, name) VALUES (?, ?)", 12, "James") } ```
  6. Support for scanning bool and []bytes (handle []bytes only, bool is handled by #26)
  7. Exported consistency levels to constants ConsistencyLevelNone, ConsistencyLevelWeak, ConsistencyLevelStrong. Users don't have to open up documentation ever again for this one.
  8. Rewritten tests to have better and consistent behavior
aldy505 commented 2 years ago

The CI failed because NewRequestWithContext is available since go 1.13, yet the CI uses go 1.12 :/

Reference: https://pkg.go.dev/net/http#NewRequestWithContext

otoolep commented 2 years ago

Thanks for your contribution.

otoolep commented 2 years ago

Do you want to try modifying .circleci/config.yml so that it includes this line: https://github.com/rqlite/rqlite/blob/master/.circleci/config.yml#L9

That's the CircleCI image I used to test rqlite.

otoolep commented 2 years ago

That CircleCI image runs Go go1.16.7.

aldy505 commented 2 years ago

@otoolep The CI works fine now. About the versioning, should we go directly to v2 or do you have something else in mind?

otoolep commented 2 years ago

v2 sounds reasonable -- can you make that change?

aldy505 commented 2 years ago

Sure, but I might continue on the weekend, as I'm occupied on a feature on my current company right now.

aldy505 commented 2 years ago

@otoolep I've updated the version. Everything should be ok, so far.

otoolep commented 2 years ago

Thanks for this -- I'm traveling until early August, so may be slow responding. Once I return to home base, I'll examine in detail.

sgalsaleh commented 2 years ago

@otoolep Any updates on this? It would be great to have this merged. We'd like to adopt it.

sgalsaleh commented 2 years ago

Piggybacking on this PR since it doesn't seem that we can submit issues in this repo: would it be possible to support nullable types as well for the Scan function? I've tried using both *string and sql.NullString but neither of those worked. Happy to submit a PR for that after this PR is merged.

aldy505 commented 2 years ago

With #26 merged, I guess I'm going to drop my parameterized query implementation, and use @sgalsaleh's instead.

sgalsaleh commented 2 years ago

Sounds good @aldy505. There are some changes in your PR that I did not include in mine that are probably still good to have:

2- Addition of QueryContext, QueryOneContext, QueueContext, QueueOneContext, WriteContext, and WriteOneContext method.

3- Comment-style from / / to // to comply with how go doc generates its' documentation. So, every documented code will looks good on https://pkg.go.dev/github.com/rqlite/gorqlite

8- Rewritten tests to have better and consistent behavior

It would be great to get those merged in. Would you be willing to rebase the PR, or would you rather create a separate PR and this one can be closed?

Note: I also added some tests in my PR and it'd be great to have them rewritten the same way.

aldy505 commented 2 years ago

@sgalsaleh I'm actually in the middle of merge conflict between my branch and the upstream. I'll get this PR updated soon.

sgalsaleh commented 2 years ago

Ah, I see. Sounds good. Let me know when it's ready for review and I can review it and merge it.

aldy505 commented 2 years ago

I think everything should be good now. If there are things to improve, it surely would be the tests, moreover, we should test how it handles scanning of different data types (example: string or text in rqlite to int16 in Go types).

At this point, I don't think we should go to the v2 versioning mark. Any thoughts? @otoolep

otoolep commented 2 years ago

If we don't need v2, sounds fine to me.

@sgalsaleh -- review and merge away, thanks!

aldy505 commented 1 year ago

Whoa, something's wrong with Github's review requests.

Anyway, sorry for a bit of a hiatus, it's been a busy week on my office and on my other side projects. These changes should be aligned with what's reviewed before.

maxnrb commented 1 year ago

Hi, any news about this RP @sgalsaleh @otoolep ? It would be very great to have context support, thanks

sgalsaleh commented 1 year ago

@maxnrb merged

otoolep commented 1 year ago

Are we sure about this? Tests seem broken now, though that might be due to the CircleCI security issue that meant all keys were rotated.

Do we know that this PR passes all tests? @sgalsaleh

sgalsaleh commented 1 year ago

Hmm, tests passed on the PR, I'll take a look shortly.

sgalsaleh commented 1 year ago

@otoolep yeah, that looks like an SSH key issue.

sgalsaleh commented 1 year ago

The tests passed on the last commit in the PR before I merged: https://app.circleci.com/pipelines/github/rqlite/gorqlite/81/workflows/58f15b14-af6c-4939-959b-cca322d25383/jobs/84

otoolep commented 1 year ago

OK, let me look into the CircleCI config.

aldy505 commented 1 year ago

Hi, sorry I've been busy with work today. Is there any other problem regarding tests?

otoolep commented 1 year ago

No, all good once I fixed up CircleCI.