Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

insights: add http handler for exporting data for one code insight#46662

Merged
leonore merged 34 commits into
mainfrom
insights/export-sql
Jan 23, 2023
Merged

insights: add http handler for exporting data for one code insight#46662
leonore merged 34 commits into
mainfrom
insights/export-sql

Conversation

@leonore

@leonore leonore commented Jan 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

closes #45746

This adds a new external api endpoint (httpapi) ./api/insights/export/{id} for exporting code insight data for an insight view. {id} corresponds to the id we use at the moment on the frontend, e.g. https://sourcegraph.sourcegraph.com/insights/aW5zaWdodF92aWV3OiIyQmRnV2VFYktwWGF2UjlGcXpuVDA1cld0c2si

The export store function adds empty rows for times we have no data for, and does some joining on tables to get all the metadata for a user

Some nitty things to consider in your review

  • Naming of things in the archive (headers, filename)
  • Format of the route ./api/insights/export/{id}

Test plan

Unit tests for basic store behaviour and repo permissions

Tested with

  • Search insight

  • Capture group insight

  • Compute type insight

  • Ensured insights with archived values had the archived values.

  • Ensured default filters were taken into account

  • Tested authorization

  • Ran the curl against oss: returns "code insights data export handler is only available in enterprise"

  • Ran the curl against enterprise: returns "code insights data export handler is only available in enterprise"

    • not the best error message there but comes from makeNotFoundHandler

Curl this was tested with

curl \
  -H 'Authorization: token {token}' https://sourcegraph.test:3443/.api/insights/export/aW5zaWdodF92aWV3OiIySEx0R0plUVJxS05kS3IyRG1SdmU3ZjNjbXoi -O -J -v

-O makes it output it into the file it defines and -J uses the header filename rather than the {id} name.

leo added 17 commits January 16, 2023 13:55
@cla-bot cla-bot Bot added the cla-signed label Jan 19, 2023
@leonore leonore requested review from a team and vovakulikov January 19, 2023 12:30
@sourcegraph-bot

sourcegraph-bot commented Jan 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 4338f05...5a6c9c7.

Notify File(s)
@sourcegraph/code-insights-backend enterprise/internal/insights/httpapi/export.go
enterprise/internal/insights/insights.go
enterprise/internal/insights/resolvers/insight_series_resolver.go
enterprise/internal/insights/resolvers/insight_view_resolvers_test.go
enterprise/internal/insights/store/insight_store.go
enterprise/internal/insights/store/mocks_temp.go
enterprise/internal/insights/store/mocks_test.go
enterprise/internal/insights/store/permissions.go
enterprise/internal/insights/store/search_contexts.go
enterprise/internal/insights/store/store.go
enterprise/internal/insights/store/store_test.go

@coury-clark

coury-clark commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

@leonore was this tested against an unauthed user, and a user that doesn't have access to the insight?

@leonore

leonore commented Jan 19, 2023

Copy link
Copy Markdown
Contributor Author

@leonore was this tested against an unauthed user,

yes, it returns "Private mode requires authentication"

and a user that doesn't have access to the insight?

good catch, we don't assert on dashboard permissions. will fix that

@vovakulikov vovakulikov mentioned this pull request Jan 20, 2023
Merged
coury-clark
coury-clark reviewed Jan 20, 2023
View reviewed changes
Comment thread enterprise/internal/insights/httpapi/export.go Outdated
coury-clark
coury-clark reviewed Jan 20, 2023
View reviewed changes
Comment thread enterprise/internal/insights/httpapi/export.go Outdated
coury-clark
coury-clark reviewed Jan 20, 2023
View reviewed changes
Comment thread enterprise/internal/insights/httpapi/export.go Outdated
coury-clark
coury-clark reviewed Jan 20, 2023
View reviewed changes
Comment thread enterprise/internal/insights/store/permissions.go Outdated
coury-clark
coury-clark reviewed Jan 20, 2023
View reviewed changes
Comment thread enterprise/internal/insights/store/store.go Outdated
coury-clark
coury-clark reviewed Jan 20, 2023
View reviewed changes
Comment thread enterprise/internal/insights/store/store.go Outdated
coury-clark
coury-clark suggested changes Jan 20, 2023
View reviewed changes

@coury-clark coury-clark left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few things that need to be addressed, but overall good work!

@leonore leonore requested a review from coury-clark January 20, 2023 18:32
@leonore

leonore commented Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

@coury-clark I've pushed some fixes for your requests. FYI I am planning on cleaning up all the repo regex loading stuff and commonising some things there as I just pulled it out the resolvers (plan is to move it out to store/search_contexts.go) but for the sake of time/timezone I wanted to get your review so that I could make more progress monday. hope that's ok!

coury-clark
coury-clark reviewed Jan 21, 2023
View reviewed changes
Comment thread enterprise/internal/insights/store/store.go Outdated
coury-clark
coury-clark reviewed Jan 21, 2023
View reviewed changes
Comment thread enterprise/internal/insights/store/permissions.go Outdated
var orgIds []int
currentActor := actor.FromContext(ctx)
if !currentActor.IsAuthenticated() {
return nil, nil, errors.New("user is unauthenticated")

@coury-clark coury-clark Jan 21, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally this returns 401, but there may be a middleware that does that already. I'd suggest giving it a quick test. I'm also wondering if we should move this check up to the API handler explicitly, just for clarity and to ensure we don't ever accidentally refactor this into the wrong place.

@leonore leonore Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah so if you try and hit the API without an authorization header it will already return 401 (with Private mode requires authentication). if you try it with a wrong token it does the same. I didn't pinpoint where exactly this was happening in the middleware. I've moved this a way up to be more explicit though.

@leonore leonore merged commit 4cb577c into main Jan 23, 2023
@leonore leonore deleted the insights/export-sql branch January 23, 2023 11:50
coury-clark added a commit that referenced this pull request Jan 23, 2023
@coury-clark
Revert "insights: add http handler for exporting data for a code insi…
2518c83 
…ght (#46662)"

This reverts commit 4cb577c.
@coury-clark coury-clark mentioned this pull request Jan 23, 2023
Merged
@leonore leonore mentioned this pull request Jan 23, 2023
Merged
coury-clark added a commit that referenced this pull request Jan 23, 2023
@coury-clark
Revert "insights: add http handler for exporting data for one code in…
936ad3e 
…sight" (#46790)

Revert "insights: add http handler for exporting data for a code insight (#46662)"

This reverts commit 4cb577c.
@leonore leonore mentioned this pull request Jan 24, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vovakulikov vovakulikov Awaiting requested review from vovakulikov

1 more reviewer

@coury-clark coury-clark coury-clark approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

insights: export datapoints including truncated points

4 participants

@leonore @sourcegraph-bot @coury-clark @vovakulikov