refactor(query): add QueryDatastoreReader to act as an implementation shim for datastores

[barakmich]

Description

Currently, there are very few calls in pkg/query direct to the datastore interface. We'd like to keep this interface minimal, in an effort to start to clean up datastores over time as we continue with query planner.

This has the added benefit of being a way to inject various testing scenarios directly with the query planner tests and benchmarks; for example, adding a slight delay on any call to simulate a network latency.

[codecov]

Codecov Report

❌ Patch coverage is 56.75676% with 144 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.85%. Comparing base (70f05dd) to head (a5fa991).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/query/datastore.go	54.08%	61 Missing and 1 partial ⚠️
pkg/query/reader.go	68.67%	35 Missing and 12 partials ⚠️
pkg/query/reader_timing.go	0.00%	33 Missing ⚠️
internal/services/v1/permissions_queryplan.go	0.00%	1 Missing ⚠️
pkg/query/context.go	66.67%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2956      +/-   ##
==========================================
+ Coverage   74.84%   74.85%   +0.01%     
==========================================
  Files         496      498       +2     
  Lines       60695    60746      +51     
==========================================
+ Hits        45423    45466      +43     
- Misses      12118    12120       +2     
- Partials     3154     3160       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[miparnisari]

could you edit the description of this PR to explain the motivation behind this change? you prefixed it with feat but this strikes me as a refactor.

[miparnisari]

is DatastoreIterator the only expected caller of this method?
more generally, maybe rephrase this to write an expectation of all callers: "it is assumed that callers of this method pass in resources that share a type". i'm also thinking that we could be defensive on this and reuse the for loop above to validate this assumption, which won't cost any extra cpu cycles

    resourceIDs := make([]string, len(resources))
	resourceType := resources[0].ObjectType
	for i, res := range resources {
		resourceIDs[i] = res.ObjectID
		if res.ObjectType != resourceType {
			return nil, fmt.Errorf("expected resource type %q but got %q", resourceType, res.ObjectType)
		}
	}

[barakmich]

Yep, this is worth a signature change. Fixed.

[miparnisari]

wouldn't it it be a bug if subject.Relation != ""?

[barakmich]

added invariants

[miparnisari]

for a follow up PR, can you please add more unit tests to this file? in https://app.codecov.io/gh/authzed/spicedb/blob/main/pkg%2Fquery%2Fdatastore.go i see that there are large blocks of code not covered by anything

[miparnisari]

overall LGTM, just some questions and one suggested follow-up

[tstirrat15]

See comment, otherwise LGTM

[tstirrat15]

Is the idea with this shim that we'd eventually have a bulk method that isn't making individual requests?

[barakmich]

Or uses the underlying cache, but yes, that's an eventual benefit.