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

V1 prototype #53

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

V1 prototype #53

wants to merge 39 commits into from

Conversation

azazeal
Copy link

@azazeal azazeal commented Feb 18, 2024

This PR contains an ambitious prototype that aims to serve as the foundation of the next major version of this package.

This iteration tries to ensure that the behavior of the package is consistent across the different data stores we aim to support. In order to achieve this, this implementation enforces validations against the 3 different types of tokens (buckets, keys & values).

Any literal may be considered a valid bucket if it:

  • a) is non-empty (and therefore non-nil).
  • b) is up to 50 bytes long.
  • c) consists of valid UTF8 encoded runes.
  • d) does not end in white space.

Similarly, any literal may be considered a valid key if it:

  • a) is non-empty (and therefore non-nil).
  • b) is up to 200 bytes long.

And, finally, any literal may be considered a valid value if it:

  • a) is not nil.
  • b) is up to 16 KiB (or 16,384 bytes) long.

Because of the above, and disregarding the expansion of most functions to also accept a Context, this implementation would only be backwards compatible in scenarios where the above constraints were already respected (even accidentally).

@github-actions github-actions bot added the needs triage Waiting for discussion / prioritization by team label Feb 18, 2024
@azazeal azazeal force-pushed the prototype branch 7 times, most recently from acc58c1 to 240f3de Compare February 18, 2024 23:09
@azazeal azazeal removed the needs triage Waiting for discussion / prioritization by team label Feb 19, 2024
@azazeal azazeal force-pushed the prototype branch 7 times, most recently from d99b82b to 01d5ecc Compare February 22, 2024 21:50
@azazeal azazeal marked this pull request as ready for review February 22, 2024 22:06
@azazeal azazeal requested a review from a team as a code owner February 22, 2024 22:06
@mohammed90
Copy link

There was a discussion at caddyserver/caddy#6097 about these upcoming changes. Given our hard experience with Badger (see linked thread) and the transitive dependencies each driver/database pulls, do you think it's possible to break the drivers from the core? Something akin to what the database/sql package offers. It allows downstream users to determine their desired DB to support and trim their transitive deps. It also avoids build constraints imposed by the drivers (Badger in our case).

@azazeal
Copy link
Author

azazeal commented Feb 24, 2024

There was a discussion at caddyserver/caddy#6097 about these upcoming changes. Given our hard experience with Badger (see linked thread) and the transitive dependencies each driver/database pulls, do you think it's possible to break the drivers from the core? Something akin to what the database/sql package offers. It allows downstream users to determine their desired DB to support and trim their transitive deps. It also avoids build constraints imposed by the drivers (Badger in our case).

With this implementation, the drivers are broken from the core. For example, unless you import _ "github.com/smallstep/nosql/driver/badger" then you won't end up building the driver for it. You may pick and choose which drivers you want to include, much like database/sql does.

This however, doesn't mean that you won't end up getting the transitive dependencies. To avoid that, we'd have to convert each driver directory in their own Go module (an option I don't believe we're willing to consider) or extract each driver into their own repo/package (e.x. github.com/smallstep/nosqlpgdrv, github.com/smallstep/nosqlmysqldrv, github.com/smallstep/nosqlbadgerdrv & github.com/smallstep/nosqlboltdrv), with the additional overhead this entails.

Your suggestion/request isn't unreasonable, and something that personally I'd like to see addressed, so I'll try to see where the team stands on and take it from there.

Hope this helps.

@mohammed90
Copy link

Thank you for the consideration! I understand the maintenance burden. I'll keep an eye on here 🙂

nosql.go Outdated Show resolved Hide resolved
nosql.go Outdated Show resolved Hide resolved
nosql.go Outdated Show resolved Hide resolved
nosql.go Outdated Show resolved Hide resolved
Comment on lines 16 to 18
// If bucket is set, then the returned value will be utf-8 valid and not contain the zero byte.
func New(t *testing.T, minSize, maxSize int, bucket bool) (tok []byte) {
if minSize == maxSize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit surprising to see the testing package being used in a package that doesn't look like a test or a test utility package (from a quick skim). I supposed this is used as a test utility?

Also:

Suggested change
// If bucket is set, then the returned value will be utf-8 valid and not contain the zero byte.
func New(t *testing.T, minSize, maxSize int, bucket bool) (tok []byte) {
if minSize == maxSize {
// If bucket is set, then the returned value will be utf-8 valid and not contain the zero byte.
func New(t *testing.T, minSize, maxSize int, bucket bool) (tok []byte) {
t.Helper()
if minSize == maxSize {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing surprising here, it's an internal package, used by tests exclusively.

Copy link
Member

@hslatman hslatman Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proving my point, it is surprising, because it's not clear for someone seeing this code for the first time that it's only used in tests. It would be clear(er) if this were in something like testutils

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I avoided a generic testutils package. I left the token package just for drivers that may want to generate a token in order to test something.

driver/postgresql/driver_test.go Show resolved Hide resolved
driver/mysql/driver_test.go Show resolved Hide resolved
@mohammed90
Copy link

I'd hate to be demanding, but can you at least put badger behind a build tag 🙂

@azazeal
Copy link
Author

azazeal commented Oct 7, 2024

Since this PR has been open for quite a while, I'd like to just comment that the plan for it is to be merged.

There's internal issues, so we've paused the merge for the time being.

@azazeal
Copy link
Author

azazeal commented Oct 7, 2024

I'd hate to be demanding, but can you at least put badger behind a build tag 🙂

If you don't

import _ "github.com/smallstep/nosql/badger/v{1,2}

then you don't end up building badger libs.

You may add your own build tag in the calling program that decides whether to add said import or not to your builds. Won't that work for you, @mohammed90?

@mohammed90
Copy link

If you don't

import _ "github.com/smallstep/nosql/badger/v{1,2}

then you don't end up building badger libs.

Oh, I missed that. That is not bad, and works.

I was hoping it'd be feasible to de-bundle the storage drivers themselves so they don't end up being transitive deps downstream. The discussion in caddyserver/caddy#6613 (comment) shows another need for slimming the dep tree. I tried my hand at making them more like database/sql drivers, but the dependency on internal/each makes it unfeasible unless it's exposed. It's a complex matter. Thank you for the effort!

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.

3 participants