Fix request scoped transactions #3103
Conversation
dnephin
requested review from
ssoroka,
BruceMacD,
jmorganca and
pdevine
as code owners
dnephin
force-pushed
the
dnephin/fix-request-txn
branch
2 times, most recently
from
a40b397
to
ab282d5
Compare
| "github.com/infrahq/infra/internal/access" | ||
| "github.com/infrahq/infra/internal/server/models" | ||
| ) | ||
|
|
||
| func pprofHandler(c *gin.Context) { | ||
| func pprofHandler(c *gin.Context, _ *api.EmptyRequest) (*api.EmptyResponse, error) { |
There was a problem hiding this comment.
Now that the database transaction comes from wrapRoute this handler had to be updated to fit the interface required by wrapRoute.
| @@ -108,30 +108,6 @@ func TestBindsEmptyRequest(t *testing.T) { | |||
| assert.NilError(t, err) | |||
| } | |||
|
|
|||
| func TestGetRoute(t *testing.T) { | |||
There was a problem hiding this comment.
I think this test was probably useful before, but now it does not really do much. We have good test coverage for all of our real production routes, so we don't need to test an artificial route.
dnephin
force-pushed
the
dnephin/fix-request-txn
branch
from
ab282d5
to
789a386
Compare
| @@ -70,40 +70,31 @@ func handleInfraDestinationHeader(c *gin.Context) error { | |||
| // authenticatedMiddleware is applied to all routes that require authentication. | |||
| // It validates the access key, and updates the lastSeenAt of the user, and | |||
| // possibly also of the destination. | |||
| func authenticatedMiddleware(srv *Server) gin.HandlerFunc { | |||
There was a problem hiding this comment.
I think the name "authenticatedMiddleware" is confusing here where this is just an auth check and scope now, it doesnt actually pass the request through any middleware
There was a problem hiding this comment.
Very true, I'll rename this to remove "middlware", and I should also update the godoc at the same time.
There was a problem hiding this comment.
I pushed a commit that renames these to authenticateRequest, and validateRequestOrganization. What do you think of those names?
| bindRoute(a, group.RouterGroup, route.method, route.path, handler) | ||
| } | ||
|
|
||
| func wrapRoute[Req, Res any](a *API, route route[Req, Res]) func(*gin.Context) error { |
There was a problem hiding this comment.
I would like to see a comment explaining that wrapRoute effectively functions as a middleware for requests, there is a lot going on in here
There was a problem hiding this comment.
Good call, there is a lot going on here. #3104 is a follow up PR that reduces it a bit.
More godoc here would be better. I'll add that now.
dnephin
force-pushed
the
dnephin/data-txn-metadata
branch
from
92a7486
to
a77eec0
Compare
dnephin
force-pushed
the
dnephin/fix-request-txn
branch
2 times, most recently
from
d3984ae
to
b1171c9
Compare
dnephin
force-pushed
the
dnephin/data-txn-metadata
branch
from
a77eec0
to
233ad27
Compare
And handle sendAPIError once, instead of on every error.
To store the behaviour of different routes. This is temporary until we can set these on the route definition directly.
This commit fixes two bugs related to request scoped transactions.
Now that they are no longer middleware. Also document wrapRoute.
dnephin
force-pushed
the
dnephin/fix-request-txn
branch
from
b1171c9
to
a2e50cf
Compare
Summary
Branched from #3028 this PR cleans up the commits from #3032 to fix both #2697 and #3076
We had two problems with our request scoped transactions:
This PR addresses the problems by:
wrapRoute, which has access to the error from both the middleware and the handlerswrapRouteadd a defer which will rollback if commit has not happenedRelated Issues
Resolves #2697
Resolves #3076