✨ Query endpoint #1643

anik120 · 2025-01-21T20:31:20Z

Description

Implementation of Query Endpoint API RFC

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2025-01-21T20:31:38Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`046c66c`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/679d5455523465000898e963
😎 Deploy Preview	https://deploy-preview-1643--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2025-01-21T20:51:52Z

Codecov Report

Attention: Patch coverage is 52.63158% with 171 lines in your changes missing coverage. Please review.

Project coverage is 66.22%. Comparing base (a19f91a) to head (f3588fb).

Files with missing lines	Patch %	Lines
catalogd/internal/storage/multireadseeker.go	14.10%	64 Missing and 3 partials ⚠️
catalogd/internal/storage/localdir.go	67.58%	43 Missing and 16 partials ⚠️
catalogd/internal/storage/index.go	60.86%	33 Missing and 3 partials ⚠️
catalogd/cmd/catalogd/main.go	0.00%	6 Missing ⚠️
catalogd/internal/serverutil/serverutil.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1643      +/-   ##
==========================================
- Coverage   67.42%   66.22%   -1.21%     
==========================================
  Files          55       57       +2     
  Lines        4632     4950     +318     
==========================================
+ Hits         3123     3278     +155     
- Misses       1284     1427     +143     
- Partials      225      245      +20

Flag	Coverage Δ
e2e	`53.29% <ø> (ø)`
unit	`54.08% <52.63%> (-0.37%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

catalogd/internal/storage/multireadseeker.go

catalogd/internal/serverutil/serverutil.go

catalogd/internal/serverutil/serverutil_tests.go

azych · 2025-01-22T08:47:28Z

What's the relation of this PR to #1642?
This one seems to include two additional commits, with the first 4 being the same.

Which PR is a duplicate and which should be reviewed?
If this is the one to review (and also not a Draft), can you add a description of the main changes this introduces?

anik120 · 2025-01-22T21:31:51Z

What's the relation of this PR to #1642? This one seems to include two additional commits, with the first 4 being the same.

Which PR is a duplicate and which should be reviewed? If this is the one to review (and also not a Draft), can you add a description of the main changes this introduces?

@azych apologies for the confusion. You're just seeing the result of rapid experimentation. This is the one to review, but it's still a WIP. I've converted this PR to a draft.

catalogd/internal/storage/localdir_test.go

joelanford · 2025-01-28T20:57:55Z

catalogd/internal/storage/localdir.go

+		httpError(w, fs.ErrNotExist)
+		return


This should change to write a 200 status and write no content to the response body.

I noticed that index.Get() was only ever returning true (so this code path was never actually being hit). Something else solved the test case failure though, but I went ahead and cleaned up Get()'s signature 53692a9

catalogd/internal/storage/localdir.go

joelanford · 2025-01-28T21:00:59Z

catalogd/internal/storage/localdir.go

+		storeMetaFuncs = append(storeMetaFuncs, storeIndexData)
+	}
+
+	var (


Throughout these lines where we setup and run concurrent channel writers and readers, we should add some comments to explain what is happening.

joelanford · 2025-01-28T21:13:51Z

catalogd/internal/storage/localdir.go

+func serveJsonLines(w http.ResponseWriter, r *http.Request, modTime time.Time, rs io.ReadSeeker) {
+	w.Header().Add("Content-Type", "application/jsonl")
+	http.ServeContent(w, r, "", modTime, rs)
+}


With my suggestion of backing away from the multi-read-seeker code, we'll have to have a different version of this function that the query handler can use (the reader returned by the index will no longer be a io.ReadSeeker, it'll just be a reader)

Unfortunately, if we don't use http.ServeContent for the query handler, we also do not get the Last-Modified/If-Modified-Since header handling. Does the standard library have a way for us to implement this caching logic so that we avoid implementing bugs in our own implementation? If we copy code from the Go stdlib, let's just make sure it's license allows that and we do whatever attribution is necessary.

Was able to find a way to http.ServeContent with io.Reader 79da4de

Copied the checkPreconditions function and all the code associated with it from the net/http package in the http_preconditions_check.go file. I did have to make modifications to the copied code too. Imho it's smells funky to me, it's a lot of copied code that we'll have to hope won't carry any bugs, especially with the modifications I did. But, for now it's in there.

joelanford · 2025-01-28T21:37:26Z

catalogd/internal/serverutil/serverutil_test.go

+
+func (m *mockStorageInstance) StorageServerHandler() http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(m.content))


Do we actually care what this content is, or do we just care about its length? If the latter, I'd suggest removing the testCompressableJSON variable and replacing with with something like strings.Rand(sizeThreshold+1)

joelanford · 2025-01-28T21:51:27Z

catalogd/internal/storage/index.go

+	"github.com/operator-framework/operator-registry/alpha/declcfg"
+)
+
+type index struct {


We should add some GoDoc to index to help our future selves understand how the indexing works.

Suggested change

type index struct {

// index is a simple index of sections of an FBC file used to lookup FBC blobs that

// match any combination of their schema, package, and name fields. A section is the

// byte offset and length of an FBC blob within the file.

//

// This index strikes a balance between space and performance. It indexes each field

// separately, and performs logical set intersections at lookup time in order to implement

// a multi-parameter query.

//

// Note: it is permissible to change the indexing algorithm later if it is necessary to

// tune the space / performance tradeoff. However care should be taken to ensure

// that the actual content returned by the index remains identical, as users of the index

// may be sensitive to differences introduced by index algorithm changes (e.g. if the

// order of the returned sections changes).

type index struct {

I realized that we'd want robust test cases for the indexing algorithm too, but that too is probably a follow up in the interest of time.

joelanford · 2025-01-28T21:56:36Z

catalogd/internal/storage/index.go

+	return newMultiReadSeeker(srs...), true
+}
+
+func (i *index) getSectionSet(schema, packageName, name string) sets.Set[section] {


One thing that occurs to me is that the indexed sections are always guaranteed to be sorted by the offset. We could likely take advantage of that fact and implement a more performant algorithm that can step directly through each slice rather than building a set for each slice, performing a set intersection, the converting back into a slice and resorting.

But this is something we can capture as a follow-up.

catalogd/internal/storage/localdir.go

joelanford · 2025-01-29T21:38:34Z

catalogd/internal/storage/localdir.go

-// in os.ErrNotExists
-type filesOnlyFilesystem struct {
-	FS fs.FS
+func serveJsonLines(w http.ResponseWriter, r *http.Request, modTime time.Time, rs io.ReadSeeker) {


Now that we have a separate function to serve data for the query handler, I think we should have the all handler directly call http.ServeFile (after explicitly setting the "Content-Type")

How about something like 046c66c. We get to retain the benefits of serverContent that way.

Signed-off-by: Joe Lanford <[email protected]>

anik120 requested a review from a team as a code owner January 21, 2025 20:31

anik120 force-pushed the query-enpoint branch from 1a96032 to a27fada Compare January 21, 2025 20:34

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/storage/multireadseeker.go Outdated Show resolved Hide resolved

anik120 force-pushed the query-enpoint branch 2 times, most recently from 17e0f9a to 729645c Compare January 21, 2025 21:57

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil_tests.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 21, 2025

View reviewed changes

catalogd/internal/serverutil/serverutil_tests.go Outdated Show resolved Hide resolved

anik120 force-pushed the query-enpoint branch from 729645c to 478c152 Compare January 22, 2025 21:24

anik120 marked this pull request as draft January 22, 2025 21:30

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2025

anik120 mentioned this pull request Jan 22, 2025

Query api #1642

Closed

4 tasks

joelanford reviewed Jan 22, 2025

View reviewed changes

catalogd/internal/storage/localdir_test.go Outdated Show resolved Hide resolved

grokspawn changed the title ~~Query endpoint~~ ✨ Query endpoint Jan 23, 2025

anik120 force-pushed the query-enpoint branch 4 times, most recently from d8dcf61 to f94eb2e Compare January 24, 2025 20:16

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir_test.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 28, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Outdated Show resolved Hide resolved

anik120 mentioned this pull request Jan 29, 2025

Query endpoint with sqlite #1654

Closed

4 tasks

anik120 force-pushed the query-enpoint branch 2 times, most recently from ee14624 to 9009472 Compare January 29, 2025 21:13

joelanford reviewed Jan 29, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Outdated Show resolved Hide resolved

joelanford reviewed Jan 29, 2025

View reviewed changes

catalogd/internal/storage/localdir.go Show resolved Hide resolved

joelanford reviewed Jan 29, 2025

View reviewed changes

anik120 force-pushed the query-enpoint branch from 9009472 to aa78a6c Compare January 31, 2025 20:45

joelanford and others added 8 commits January 31, 2025 15:49

add a new feature-gated api/v1/query endpoint

1ef699b

Signed-off-by: Joe Lanford <[email protected]>

add singleflight for shared index access for concurrent requests

4f6dad9

Signed-off-by: Joe Lanford <[email protected]>

a few improvements and optimizations

1301a85

Signed-off-by: Joe Lanford <[email protected]>

another round of refactoring improvement

f9fd668

Signed-off-by: Joe Lanford <[email protected]>

Tests

09a3795

use io.MultiReader instead of manual implementation

a5524f7

include checkPrecoditions check

2ffc6fc

add comments

3b6b263

anik120 force-pushed the query-enpoint branch from aa78a6c to 3b6b263 Compare January 31, 2025 20:49

anik120 added 4 commits January 31, 2025 16:18

code cleanup

192cbce

cleanup index.Get's signature

53692a9

only allow GET/HEAD methods

e15a5dc

refractor serverJSONLines

046c66c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Query endpoint #1643

✨ Query endpoint #1643

anik120 commented Jan 21, 2025 •

edited

Loading

netlify bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

azych commented Jan 22, 2025

anik120 commented Jan 22, 2025

joelanford Jan 28, 2025

anik120 Jan 31, 2025

joelanford Jan 28, 2025

joelanford Jan 28, 2025

anik120 Jan 29, 2025

anik120 Jan 31, 2025

joelanford Jan 28, 2025

joelanford Jan 28, 2025 •

edited

Loading

anik120 Jan 31, 2025

joelanford Jan 28, 2025

joelanford Jan 29, 2025

anik120 Jan 31, 2025

-type index struct {
+// index is a simple index of sections of an FBC file used to lookup FBC blobs that
+// match any combination of their schema, package, and name fields. A section is the
+// byte offset and length of an FBC blob within the file.
+//
+// This index strikes a balance between space and performance. It indexes each field
+// separately, and performs logical set intersections at lookup time in order to implement
+// a multi-parameter query.
+//
+// Note: it is permissible to change the indexing algorithm later if it is necessary to
+// tune the space / performance tradeoff. However care should be taken to ensure
+// that the actual content returned by the index remains identical, as users of the index
+// may be sensitive to differences introduced by index algorithm changes (e.g. if the
+// order of the returned sections changes).
+type index struct {

✨ Query endpoint #1643

Are you sure you want to change the base?

✨ Query endpoint #1643

Conversation

anik120 commented Jan 21, 2025 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jan 21, 2025 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

azych commented Jan 22, 2025

anik120 commented Jan 22, 2025

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

joelanford Jan 28, 2025 • edited Loading

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

anik120 commented Jan 21, 2025 •

edited

Loading

netlify bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

joelanford Jan 28, 2025 •

edited

Loading