Skip to content

Commit

Permalink
feedback from @Julio-Guerra
Browse files Browse the repository at this point in the history
  • Loading branch information
RomainMuller committed Feb 5, 2024
1 parent 3ee80c7 commit b0160e2
Show file tree
Hide file tree
Showing 12 changed files with 57 additions and 57 deletions.
8 changes: 2 additions & 6 deletions internal/appsec/dyngo/operation.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,10 @@ func StartOperation[O Operation, E ArgOf[O]](op O, args E) {
}
}

func newOperation(parent Operation) *operation {
return &operation{parent: parent.unwrap()}
}

// Finish finishes the operation along with its results and emits a
// FinishOperation finishes the operation along with its results and emits a
// finish event with the operation results.
// The operation is then disabled and its event listeners removed.
func Finish[O Operation, E ResultOf[O]](op O, results E) {
func FinishOperation[O Operation, E ResultOf[O]](op O, results E) {
o := op.unwrap()
defer o.disable() // This will need the RLock below to be released...

Expand Down
64 changes: 32 additions & 32 deletions internal/appsec/dyngo/operation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ func TestUsage(t *testing.T) {

t.Run("recursive-operation", func(t *testing.T) {
root := startOperation(RootArgs{}, nil)
defer dyngo.Finish(root, RootRes{})
defer dyngo.FinishOperation(root, RootRes{})

called := 0
dyngo.On(root, func(operation, HTTPHandlerArgs) { called++ })
Expand All @@ -362,7 +362,7 @@ func TestUsage(t *testing.T) {
t.Run("concurrency", func(t *testing.T) {
// root is the shared operation having concurrent accesses in this test
root := startOperation(RootArgs{}, nil)
defer dyngo.Finish(root, RootRes{})
defer dyngo.FinishOperation(root, RootRes{})

// Create nbGoroutines registering event listeners concurrently
nbGoroutines := 1000
Expand Down Expand Up @@ -405,7 +405,7 @@ func TestUsage(t *testing.T) {
startBarrier.Wait()
defer done.Done()
op := startOperation(MyOperationArgs{}, root)
defer dyngo.Finish(op, MyOperationRes{})
defer dyngo.FinishOperation(op, MyOperationRes{})
}()
}

Expand Down Expand Up @@ -463,7 +463,7 @@ func startOperation[T dyngo.ArgOf[operation]](args T, parent dyngo.Operation) op
// Helper function to run operations recursively.
func runOperation[A dyngo.ArgOf[operation], R dyngo.ResultOf[operation]](parent dyngo.Operation, args A, res R, child func(dyngo.Operation)) {
op := startOperation(args, parent)
defer dyngo.Finish(op, res)
defer dyngo.FinishOperation(op, res)
if child != nil {
child(op)
}
Expand All @@ -479,7 +479,7 @@ func TestOperationData(t *testing.T) {
for i := 0; i < 10; i++ {
dyngo.EmitData(op, &data)
}
dyngo.Finish(op, MyOperationRes{})
dyngo.FinishOperation(op, MyOperationRes{})
require.Equal(t, 10, data)
})

Expand All @@ -493,8 +493,8 @@ func TestOperationData(t *testing.T) {
for i := 0; i < 10; i++ {
dyngo.EmitData(op2, &data)
}
dyngo.Finish(op2, MyOperation2Res{})
dyngo.Finish(op1, MyOperationRes{})
dyngo.FinishOperation(op2, MyOperation2Res{})
dyngo.FinishOperation(op1, MyOperationRes{})
require.Equal(t, 10, data)
})

Expand All @@ -507,8 +507,8 @@ func TestOperationData(t *testing.T) {
for i := 0; i < 10; i++ {
dyngo.EmitData(op2, &data)
}
dyngo.Finish(op2, MyOperation2Res{})
dyngo.Finish(op1, MyOperationRes{})
dyngo.FinishOperation(op2, MyOperation2Res{})
dyngo.FinishOperation(op1, MyOperationRes{})
require.Equal(t, 20, data)
})
})
Expand All @@ -524,22 +524,22 @@ func TestOperationEvents(t *testing.T) {
})

op2 := startOperation(MyOperation2Args{}, op1)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})

// Called once
require.Equal(t, 1, called)

op2 = startOperation(MyOperation2Args{}, op1)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})

// Called again
require.Equal(t, 2, called)

// Finish the operation so that it gets disabled and its listeners removed
dyngo.Finish(op1, MyOperationRes{})
dyngo.FinishOperation(op1, MyOperationRes{})

op2 = startOperation(MyOperation2Args{}, op1)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})

// No longer called
require.Equal(t, 2, called)
Expand All @@ -554,35 +554,35 @@ func TestOperationEvents(t *testing.T) {
})

op2 := startOperation(MyOperation2Args{}, op1)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// Called once
require.Equal(t, 1, called)

op2 = startOperation(MyOperation2Args{}, op1)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// Called again
require.Equal(t, 2, called)

op3 := startOperation(MyOperation3Args{}, op2)
dyngo.Finish(op3, MyOperation3Res{})
dyngo.FinishOperation(op3, MyOperation3Res{})
// Not called
require.Equal(t, 2, called)

op2 = startOperation(MyOperation2Args{}, op3)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// Called again
require.Equal(t, 3, called)

// Finish the operation so that it gets disabled and its listeners removed
dyngo.Finish(op1, MyOperationRes{})
dyngo.FinishOperation(op1, MyOperationRes{})

op2 = startOperation(MyOperation2Args{}, op3)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// No longer called
require.Equal(t, 3, called)

op2 = startOperation(MyOperation2Args{}, op2)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// No longer called
require.Equal(t, 3, called)
})
Expand All @@ -605,32 +605,32 @@ func TestOperationEvents(t *testing.T) {

// Trigger the registered events
op2 := startOperation(MyOperation2Args{}, op)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// We should have 4 calls
require.Equal(t, 2, calls)

// Finish the operation to disable it. Its event listeners should then be removed.
dyngo.Finish(op, MyOperationRes{})
dyngo.FinishOperation(op, MyOperationRes{})

// Trigger the same events
op2 = startOperation(MyOperation2Args{}, op)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// The number of calls should be unchanged
require.Equal(t, 2, calls)

// Register again, but it shouldn't work because the operation is finished.
registerTo(op)
// Trigger the same events
op2 = startOperation(MyOperation2Args{}, op)
dyngo.Finish(op2, MyOperation2Res{})
dyngo.FinishOperation(op2, MyOperation2Res{})
// The number of calls should be unchanged
require.Equal(t, 2, calls)
})

t.Run("event-listener-panic", func(t *testing.T) {
t.Run("start", func(t *testing.T) {
op := startOperation(MyOperationArgs{}, nil)
defer dyngo.Finish(op, MyOperationRes{})
defer dyngo.FinishOperation(op, MyOperationRes{})

// Panic on start
calls := 0
Expand All @@ -643,14 +643,14 @@ func TestOperationEvents(t *testing.T) {
require.NotPanics(t, func() {
op := startOperation(MyOperationArgs{}, op)
require.NotNil(t, op)
defer dyngo.Finish(op, MyOperationRes{})
defer dyngo.FinishOperation(op, MyOperationRes{})
require.Equal(t, calls, 1)
})
})

t.Run("finish", func(t *testing.T) {
op := startOperation(MyOperationArgs{}, nil)
defer dyngo.Finish(op, MyOperationRes{})
defer dyngo.FinishOperation(op, MyOperationRes{})
// Panic on finish
calls := 0
dyngo.OnFinish(op, func(operation, MyOperationRes) {
Expand All @@ -662,7 +662,7 @@ func TestOperationEvents(t *testing.T) {
require.NotPanics(t, func() {
op := startOperation(MyOperationArgs{}, op)
require.NotNil(t, op)
dyngo.Finish(op, MyOperationRes{})
dyngo.FinishOperation(op, MyOperationRes{})
require.Equal(t, calls, 1)
})
})
Expand All @@ -675,12 +675,12 @@ func BenchmarkEvents(b *testing.B) {
for length := 1; length <= 64; length *= 2 {
b.Run(fmt.Sprintf("stack=%d", length), func(b *testing.B) {
root := startOperation(MyOperationArgs{}, nil)
defer dyngo.Finish(root, MyOperationRes{})
defer dyngo.FinishOperation(root, MyOperationRes{})

op := root
for i := 0; i < length-1; i++ {
op = startOperation(MyOperationArgs{}, op)
defer dyngo.Finish(op, MyOperationRes{})
defer dyngo.FinishOperation(op, MyOperationRes{})
}

b.Run("start event", func(b *testing.B) {
Expand All @@ -700,7 +700,7 @@ func BenchmarkEvents(b *testing.B) {
b.ResetTimer()
for n := 0; n < b.N; n++ {
leafOp := startOperation(MyOperationArgs{}, op)
dyngo.Finish(leafOp, MyOperationRes{})
dyngo.FinishOperation(leafOp, MyOperationRes{})
}
})
})
Expand All @@ -709,7 +709,7 @@ func BenchmarkEvents(b *testing.B) {

b.Run("registering", func(b *testing.B) {
op := startOperation(MyOperationArgs{}, nil)
defer dyngo.Finish(op, MyOperationRes{})
defer dyngo.FinishOperation(op, MyOperationRes{})

b.Run("start event", func(b *testing.B) {
b.ReportAllocs()
Expand Down
6 changes: 3 additions & 3 deletions internal/appsec/emitter/graphqlsec/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type (
// Finish the GraphQL query operation, along with the given results, and emit a finish event up in
// the operation stack.
func (q *RequestOperation) Finish(res RequestOperationRes) {
dyngo.Finish(q, res)
dyngo.FinishOperation(q, res)
}

func (RequestOperationArgs) IsArgOf(*RequestOperation) {}
Expand Down Expand Up @@ -69,7 +69,7 @@ type (
// Finish the GraphQL query operation, along with the given results, and emit a finish event up in
// the operation stack.
func (q *ExecutionOperation) Finish(res ExecutionOperationRes) {
dyngo.Finish(q, res)
dyngo.FinishOperation(q, res)
}

func (ExecutionOperationArgs) IsArgOf(*ExecutionOperation) {}
Expand Down Expand Up @@ -105,7 +105,7 @@ type (
// Finish the GraphQL Field operation, along with the given results, and emit a finish event up in
// the operation stack.
func (q *ResolveOperation) Finish(res ResolveOperationRes) {
dyngo.Finish(q, res)
dyngo.FinishOperation(q, res)
}

func (ResolveOperationArgs) IsArgOf(*ResolveOperation) {}
Expand Down
2 changes: 1 addition & 1 deletion internal/appsec/emitter/grpcsec/grpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestUsage(t *testing.T) {
return func(t *testing.T) {
localRootOp := dyngo.NewRootOperation()
dyngo.StartOperation(localRootOp, rootArgs{})
defer dyngo.Finish(localRootOp, rootRes{})
defer dyngo.FinishOperation(localRootOp, rootRes{})

var handlerStarted, handlerFinished, recvStarted, recvFinished int
defer func() {
Expand Down
4 changes: 2 additions & 2 deletions internal/appsec/emitter/grpcsec/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,14 @@ func (e *MonitoringError) Error() string {
// Finish the gRPC handler operation, along with the given results, and emit a
// finish event up in the operation stack.
func (op *HandlerOperation) Finish(res HandlerOperationRes) []any {
dyngo.Finish(op, res)
dyngo.FinishOperation(op, res)
return op.Events()
}

// Finish the gRPC handler operation, along with the given results, and emits a
// finish event up in the operation stack.
func (op ReceiveOperation) Finish(res ReceiveOperationRes) {
dyngo.Finish(op, res)
dyngo.FinishOperation(op, res)
}

func (HandlerOperationArgs) IsArgOf(*HandlerOperation) {}
Expand Down
2 changes: 1 addition & 1 deletion internal/appsec/emitter/httpsec/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func ExecuteSDKBodyOperation(parent dyngo.Operation, args types.SDKBodyOperation
err = e
})
dyngo.StartOperation(op, args)
dyngo.Finish(op, types.SDKBodyOperationRes{})
dyngo.FinishOperation(op, types.SDKBodyOperationRes{})
return err
}

Expand Down
4 changes: 2 additions & 2 deletions internal/appsec/emitter/httpsec/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type (
// Finish the HTTP handler operation, along with the given results and emits a
// finish event up in the operation stack.
func (op *Operation) Finish(res HandlerOperationRes) []any {
dyngo.Finish(op, res)
dyngo.FinishOperation(op, res)
return op.Events()
}

Expand Down Expand Up @@ -80,7 +80,7 @@ type (

// Finish finishes the SDKBody operation and emits a finish event
func (op *SDKBodyOperation) Finish() {
dyngo.Finish(op, SDKBodyOperationRes{})
dyngo.FinishOperation(op, SDKBodyOperationRes{})
}

// Error implements the Error interface
Expand Down
2 changes: 1 addition & 1 deletion internal/appsec/emitter/sharedsec/shared.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func ExecuteUserIDOperation(parent dyngo.Operation, args UserIDOperationArgs) er
op := &UserIDOperation{Operation: dyngo.NewOperation(parent)}
dyngo.OnData(op, func(e error) { err = e })
dyngo.StartOperation(op, args)
dyngo.Finish(op, UserIDOperationRes{})
dyngo.FinishOperation(op, UserIDOperationRes{})
return err
}

Expand Down
6 changes: 3 additions & 3 deletions internal/appsec/listener/graphqlsec/graphql.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ var supportedAddresses = listener.AddressSet{
// Install registers the GraphQL WAF Event Listener on the given root operation.
func Install(wafHandle *waf.Handle, _ sharedsec.Actions, cfg *config.Config, lim limiter.Limiter, root dyngo.Operation) {
if listener := newWafEventListener(wafHandle, cfg, lim); listener != nil {
log.Debug("[appsec] registering the GraphQL WAF Event Listener")
log.Debug("appsec: registering the GraphQL WAF Event Listener")
dyngo.On(root, listener.onEvent)
}
}
Expand All @@ -51,13 +51,13 @@ type wafEventListener struct {

func newWafEventListener(wafHandle *waf.Handle, cfg *config.Config, limiter limiter.Limiter) *wafEventListener {
if wafHandle == nil {
log.Debug("[appsec] no WAF Handle available, the GraphQL WAF Event Listener will not be registered")
log.Debug("appsec: no WAF Handle available, the GraphQL WAF Event Listener will not be registered")
return nil
}

addresses := listener.FilterAddressSet(supportedAddresses, wafHandle)
if len(addresses) == 0 {
log.Debug("[appsec] no supported GraphQL address is used by currently loaded WAF rules, the GraphQL WAF Event Listener will not be registered")
log.Debug("appsec: no supported GraphQL address is used by currently loaded WAF rules, the GraphQL WAF Event Listener will not be registered")
return nil
}

Expand Down
6 changes: 3 additions & 3 deletions internal/appsec/listener/grpcsec/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ var supportedAddresses = listener.AddressSet{
// Install registers the gRPC WAF Event Listener on the given root operation.
func Install(wafHandle *waf.Handle, actions sharedsec.Actions, cfg *config.Config, lim limiter.Limiter, root dyngo.Operation) {
if listener := newWafEventListener(wafHandle, actions, cfg, lim); listener != nil {
log.Debug("[appsec] registering the gRPC WAF Event Listener")
log.Debug("appsec: registering the gRPC WAF Event Listener")
dyngo.On(root, listener.onEvent)
}
}
Expand All @@ -60,13 +60,13 @@ type wafEventListener struct {

func newWafEventListener(wafHandle *waf.Handle, actions sharedsec.Actions, cfg *config.Config, limiter limiter.Limiter) *wafEventListener {
if wafHandle == nil {
log.Debug("[appsec] no WAF Handle available, the gRPC WAF Event Listener will not be registered")
log.Debug("appsec: no WAF Handle available, the gRPC WAF Event Listener will not be registered")
return nil
}

addresses := listener.FilterAddressSet(supportedAddresses, wafHandle)
if len(addresses) == 0 {
log.Debug("[appsec] no supported gRPC address is used by currently loaded WAF rules, the gRPC WAF Event Listener will not be registered")
log.Debug("appsec: no supported gRPC address is used by currently loaded WAF rules, the gRPC WAF Event Listener will not be registered")
return nil
}

Expand Down

0 comments on commit b0160e2

Please sign in to comment.