smallstep / nosql

NoSQL is an abstraction layer for data persistency
Apache License 2.0
20 stars 23 forks source link

V1 prototype #53

Open azazeal opened 9 months ago

azazeal commented 9 months ago

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:

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

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

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).

mohammed90 commented 9 months ago

There was a discussion at https://github.com/caddyserver/caddy/pull/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 commented 9 months ago

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 commented 9 months ago

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

mohammed90 commented 7 months ago

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

azazeal commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/caddyserver/caddy/issues/6613#issuecomment-2408169051 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!