Add pkg/querier package #4849

rdimitrov · 2024-10-31T09:46:37Z

Summary

The following PR adds a querier package to pkg. Its purpose is to expose a set of tools to interact with the Minder database.
The wrappers for managing the ruletypes and profiles are wrapped around their service alternatives as making changes to these entries without validating them can cause conflicts in the profile references and evaluations.

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-10-31T09:58:10Z

Pull Request Test Coverage Report for Build 11631537336

Details

0 of 204 (0.0%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.4%) to 54.835%

Changes Missing Coverage	Changed/Added Lines	%
pkg/querier/bundle.go	21	0.0%
pkg/querier/profile.go	31	0.0%
pkg/querier/ruletype.go	31	0.0%
pkg/querier/project.go	56	0.0%
pkg/querier/querier.go	65	0.0%

Totals
Change from base Build 11631234497:	-0.4%
Covered Lines:	15537
Relevant Lines:	28334

💛 - Coveralls

evankanderson

How are you feeling about this approach having started it?

I'm a bit nervous about the interaction with transactions; I think I'd rather expose methods to manage a sql.Tx outside the interface than try to bundle it into the interface if you need transaction support. (Or you could include the transaction in the context, which might be a nice API)

evankanderson · 2024-10-31T13:25:18Z

proto/minder/v1/minder.proto

+
+// BundleSubscription represents a subscription to a bundle.
+message BundleSubscription {
+    string id = 1;
+    string project_id = 2;
+    string bundle_id = 3;
+    string current_version = 4;
+}


It feels like this should be someplace different from the external API, since it's not referenced by any clients.

We have an internal/proto, but maybe it makes sense to have a pkg/proto (and exclude that from coverage)?

evankanderson · 2024-10-31T13:54:20Z

pkg/querier/querier.go

+	ProfileHandlers
+	BundleHandlers
+	Close()
+	Commit() error


Do you need a way to start a transaction, given that you have a Commit method?

Oh, it looks like you need to throw away the object (and the connection pool) after each transaction?

database/query/projects.sql

evankanderson · 2024-11-01T15:37:55Z

pkg/querier/querier.go

+	RuleTypeHandlers
+	ProfileHandlers
+	BundleHandlers
+	CommitTx() error


I'd probably call this Commit and Cancel, and document that the object can't be used after calling these.

evankanderson · 2024-11-01T15:43:30Z

pkg/querier/querier.go

+	return fmt.Errorf("no transaction to cancel")
+}
+
+// initDb function initializes the database connection and transaction details, if needed


This doesn't do anything with transactions, does it?

evankanderson · 2024-11-01T15:47:54Z

pkg/querier/querier.go

+var _ Querier = (*Type)(nil)
+
+// New creates a new instance of the querier
+func New(ctx context.Context, config *server.Config) (Store, func(), error) {


You may want to define a type for the second return argument (it's not clear what you do with a func() on its own).

evankanderson · 2024-11-01T15:48:23Z

pkg/querier/querier.go

+}
+
+// Type represents the querier type
+type Type struct {


Why is this type exported?

evankanderson · 2024-11-01T15:49:21Z

proto/minder/v1/minder.proto

@@ -1023,6 +1023,8 @@ message Project {
    // display_names are short *non-unique* strings to provide
    // a user-friendly name for presentation in lists, etc.
    string display_name = 5;
+    bool is_organization = 8;


Discussed offline, but I don't think we want to change our public API in this PR.

Signed-off-by: Radoslav Dimitrov <[email protected]>

evankanderson · 2024-11-01T17:04:48Z

pkg/querier/querier.go

+		return nil, nil, fmt.Errorf("unable to setup eventer: %w", err)
+	}
+	// Return the new Type
+	return &querierType{


Oh, this is cute -- you're using querierType as both Store and Querier. I think that won't hurt you; anyone who goes to cast one to the other gets what's coming to them.

evankanderson · 2024-11-01T17:05:22Z

pkg/querier/querier.go

+	}
+	// Return the new Type
+	return &querierType{
+		store:      store,


You probably don't need both querier and store with the new implementation, but I'm not going to begrudge you 8 bytes.

You know this unlocks my embedded programming side if you do that 😁

evankanderson · 2024-11-01T17:09:23Z

pkg/querier/querier.go

+	if q.tx != nil {
+		err := q.store.Commit(q.tx)
+		// Clear the transaction and the querier
+		q.tx = nil
+		q.querier = nil
+		return err
+	}
+	return fmt.Errorf("no transaction to commit")


No need to fix here, but I'd tend to use the "early exit" model for these sorts of checks:

Suggested change

if q.tx != nil {

err := q.store.Commit(q.tx)

// Clear the transaction and the querier

q.tx = nil

q.querier = nil

return err

}

return fmt.Errorf("no transaction to commit")

if q.tx == nil {

return fmt.Errorf("no transaction to commit")

}

err := q.store.Commit(q.tx)

// Clear the transaction and the querier

q.tx = nil

q.querier = nil

return err

When reading code, I find that I need to keep a mental stack open whenever I'm in a conditional to recall what the code path is with and without the conditional. Having conditionals that close quickly with early returns lets me retire that call stack in my head, and leaves the "most common" code path running to the end of the function.

evankanderson · 2024-11-01T17:10:45Z

pkg/querier/querier.go

+	zerolog.Ctx(ctx).Debug().
+		Str("name", cfg.Database.Name).
+		Str("host", cfg.Database.Host).
+		Str("user", cfg.Database.User).
+		Str("ssl_mode", cfg.Database.SSLMode).
+		Int("port", cfg.Database.Port).
+		Msg("connecting to minder database")


We log these inside GetDBConnection too, don't we?

evankanderson · 2024-11-01T17:14:11Z

To be clear, the above are nits, and I'm fine with the code going in as-is.

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov requested a review from a team as a code owner October 31, 2024 09:46

rdimitrov force-pushed the move-db branch from 98eca80 to a1734f0 Compare October 31, 2024 09:49

rdimitrov self-assigned this Oct 31, 2024

rdimitrov force-pushed the move-db branch from 637ff32 to 430daae Compare October 31, 2024 13:37

evankanderson reviewed Oct 31, 2024

View reviewed changes

JAORMX reviewed Oct 31, 2024

View reviewed changes

database/query/projects.sql Show resolved Hide resolved

rdimitrov mentioned this pull request Oct 31, 2024

Deprecate the is_organization column in the projects table #4853

Open

rdimitrov force-pushed the move-db branch 3 times, most recently from 44b522d to eb9837a Compare November 1, 2024 12:23

eleftherias previously approved these changes Nov 1, 2024

View reviewed changes

rdimitrov dismissed eleftherias’s stale review via 90c13b6 November 1, 2024 15:08

rdimitrov force-pushed the move-db branch from eb9837a to 90c13b6 Compare November 1, 2024 15:08

evankanderson reviewed Nov 1, 2024

View reviewed changes

rdimitrov added 4 commits November 1, 2024 19:00

Add pkg/querier package

bad3963

Signed-off-by: Radoslav Dimitrov <[email protected]>

Add a Querier interface per transaction

871a3ac

Signed-off-by: Radoslav Dimitrov <[email protected]>

Include the rule and profile services in the querier

e02a91f

Signed-off-by: Radoslav Dimitrov <[email protected]>

Use the correct set bundle version query for the subscriptions table

8aa1a0b

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov force-pushed the move-db branch from aa562d1 to 0451394 Compare November 1, 2024 17:02

Cleanup querier after transaction commit or cancel

a7147bd

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov force-pushed the move-db branch from 0451394 to a7147bd Compare November 1, 2024 17:04

evankanderson previously approved these changes Nov 1, 2024

View reviewed changes

Refactor if statements and remove duplicated log message

17900b2

Signed-off-by: Radoslav Dimitrov <[email protected]>

rdimitrov dismissed evankanderson’s stale review via 17900b2 November 1, 2024 17:18

evankanderson approved these changes Nov 1, 2024

View reviewed changes

rdimitrov merged commit dc6fce9 into main Nov 1, 2024
24 checks passed

rdimitrov deleted the move-db branch November 1, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pkg/querier package #4849

Add pkg/querier package #4849

rdimitrov commented Oct 31, 2024

coveralls commented Oct 31, 2024 •

edited

Loading

evankanderson left a comment

evankanderson Oct 31, 2024

evankanderson Oct 31, 2024

evankanderson Oct 31, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

rdimitrov Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson Nov 1, 2024

evankanderson commented Nov 1, 2024

Add pkg/querier package #4849

Add pkg/querier package #4849

Conversation

rdimitrov commented Oct 31, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Oct 31, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11631537336

Details

💛 - Coveralls

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Nov 1, 2024

coveralls commented Oct 31, 2024 •

edited

Loading