acc: Simplify writing handlers; support headers in responses #2338

denik · 2025-02-12T09:42:10Z

Changes

Handlers now receive testserver.Request and return any which could be

string or []byte (returns it as is but sets content-type to json or plain text depending on content)
struct (encodes it as json and sets content-type to json)
testserver.Response (full control over status and headers)

Note if testserver.Response is returned from the handler, it's Body attribute can still be an object. In that case, it'll be serialized and appropriate content-type header will be added.

The config is now using the same testserver.Response struct, the same logic applies both configured responses and responses returned from handlers.

As a result, one can set headers both in Golang handlers and in test.toml.

This also fixes a bug with RecordRequest not seeing the body if it was already consumed by the handler.

Tests

Existing rests.
acceptance/selftest/server is extended to set response header.

shreyas-goenka · 2025-02-12T10:54:59Z

libs/testserver/fake_workspace.go

 	jobId := s.nextJobId
 	s.nextJobId++

 	jobSettings := jobs.JobSettings{}
 	err := jsonConvert(request, &jobSettings)
 	if err != nil {
-		return internalError(err)
+		return Response{
+			StatusCode: 400,


nit: Use http.StatusBadRequest instead of an integer directly. Same for other status codes.

I actually prefer 200/400/404/500 over anything else. Do you find those codes hard to read?

Not if I know what the code means. But I had to look up that 400 was BadRequest since I did not remember that off the top of my head.

We can go with the approach you prefer though.

shreyas-goenka

Thanks! Looks good to me, just some minor comments.

shreyas-goenka · 2025-02-12T11:00:24Z

libs/testserver/fake_workspace.go

 	} else {
-		return workspace.ObjectInfo{}, http.StatusNotFound
+		return nil


It's a bit too implicit that nil corresponds to 404. Normally, in Go, returning nil does not correspond to an error.

Should we explicitly return a 404 here?

Fair enough. I thought it would be generally applicable, but this is the only case that relies on it. Updated to return explicit response here.

shreyas-goenka · 2025-02-12T11:05:29Z

acceptance/bundle/debug/out.stderr.txt

@@ -78,12 +78,10 @@
 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit
 10:07:59 Debug: Apply pid=12345 mutator=validate
 10:07:59 Debug: GET /api/2.0/workspace/get-status?path=/Workspace/Users/[USERNAME]/.bundle/debug/default/files
-< HTTP/1.1 404 Not Found


Why do these logs go away?

They don't quite go away -- they are formatted differently, seems like a line was split previously not not anymore.

Seems like it started being logged again once we explicitly returned 404?

This PR does not change how debug logs are formatted, so it's not really the focus there.

Yeah, fair enough. It's interesting though, because a faithful server would allow us to catch a regression whether we stop logging the status code in the future. It's not clear why this is happening when both approaches seem to be the same.

shreyas-goenka · 2025-02-12T11:06:09Z

acceptance/bundle/debug/out.stderr.parallel.txt

-< {} pid=12345 mutator=validate mutator (read-only)=parallel mutator (read-only)=validate:files_to_sync sdk=true
-< {} pid=12345 mutator=validate mutator (read-only)=parallel mutator (read-only)=validate:files_to_sync sdk=true
+10:07:59 Debug: non-retriable error: Not Found pid=12345 mutator=validate mutator (read-only)=parallel mutator (read-only)=validate:files_to_sync sdk=true
+< HTTP/0.0 000 Not Found (Error: Not Found) pid=12345 mutator=validate mutator (read-only)=parallel mutator (read-only)=validate:files_to_sync sdk=true


What's happening here?

Debug log capture response and it changed slightly.

The HTTP/0.0 000 bit looks weird. Where is it coming from?

From script - we have sed command there.

…ody bug Handlers now receive testserver.Request and return any which could be - nil (returns 404) - string / []byte (returns it as is but sets content-type to json or test depending on content) - object (encodes it as json and sets content-type to json) - testserver.Response (full control over status, headers) The config is now using the same testserver.Response struct as handlers, so the same logic applies there. It is now possible to specify headers in test.toml. This also fixes a bug with RecordRequest reading the body, not leaving it for the actual handler.

shreyas-goenka · 2025-02-12T12:57:55Z

acceptance/bundle/debug/out.stderr.txt

@@ -78,12 +78,10 @@
 10:07:59 Debug: No script defined for postinit, skipping pid=12345 mutator=initialize mutator=seq mutator=scripts.postinit
 10:07:59 Debug: Apply pid=12345 mutator=validate
 10:07:59 Debug: GET /api/2.0/workspace/get-status?path=/Workspace/Users/[USERNAME]/.bundle/debug/default/files
-< HTTP/1.1 404 Not Found


Yeah, fair enough. It's interesting though, because a faithful server would allow us to catch a regression whether we stop logging the status code in the future. It's not clear why this is happening when both approaches seem to be the same.

This is needed for b.WorkspaceClient().CurrentWorkspaceID(ctx) which is used by initialize_urls.go mutator ("bundle summary") (#2316). It also also needed for to call serverless endpoint (#2348). Builds on top of #2338

This is needed for b.WorkspaceClient().CurrentWorkspaceID(ctx) which is used by initialize_urls.go mutator ("bundle summary") #2316 It also also needed for to call serverless detection endpoint #2348 Builds on top of #2338

denik temporarily deployed to test-trigger-is February 12, 2025 09:42 — with GitHub Actions Inactive

denik marked this pull request as ready for review February 12, 2025 09:45

denik requested review from pietern, andrewnester and shreyas-goenka as code owners February 12, 2025 09:45

denik temporarily deployed to test-trigger-is February 12, 2025 10:14 — with GitHub Actions Inactive

shreyas-goenka reviewed Feb 12, 2025

View reviewed changes

denik temporarily deployed to test-trigger-is February 12, 2025 11:29 — with GitHub Actions Inactive

denik temporarily deployed to test-trigger-is February 12, 2025 11:50 — with GitHub Actions Inactive

denik added 4 commits February 12, 2025 12:54

clean up

2dbc70d

Fix JobsCreate() request

7709407

Do not convert nil to 404

c9beddc

denik force-pushed the denik/acc-request-response branch from c2a13c4 to c9beddc Compare February 12, 2025 11:55

denik temporarily deployed to test-trigger-is February 12, 2025 11:55 — with GitHub Actions Inactive

denik requested a review from shreyas-goenka February 12, 2025 11:56

shreyas-goenka approved these changes Feb 12, 2025

View reviewed changes

denik added this pull request to the merge queue Feb 12, 2025

Merged via the queue into main with commit 4034766 Feb 12, 2025
9 checks passed

denik deleted the denik/acc-request-response branch February 12, 2025 13:07

denik mentioned this pull request Feb 12, 2025

acc: Set X-Databricks-Org-Id on scim/v2/Me endpoint #2349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

acc: Simplify writing handlers; support headers in responses #2338

acc: Simplify writing handlers; support headers in responses #2338

denik commented Feb 12, 2025 •

edited

Loading

shreyas-goenka Feb 12, 2025

denik Feb 12, 2025

shreyas-goenka Feb 12, 2025

shreyas-goenka left a comment

shreyas-goenka Feb 12, 2025

denik Feb 12, 2025

shreyas-goenka Feb 12, 2025

denik Feb 12, 2025

shreyas-goenka Feb 12, 2025

denik Feb 12, 2025

shreyas-goenka Feb 12, 2025

shreyas-goenka Feb 12, 2025

denik Feb 12, 2025

shreyas-goenka Feb 12, 2025

denik Feb 12, 2025

shreyas-goenka Feb 12, 2025

acc: Simplify writing handlers; support headers in responses #2338

acc: Simplify writing handlers; support headers in responses #2338

Conversation

denik commented Feb 12, 2025 • edited Loading

Changes

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka 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

denik commented Feb 12, 2025 •

edited

Loading