Skip to content

Fix AppSec Sinatra contrib types #5275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
y9v wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix AppSec Sinatra contrib types #5275

y9v wants to merge 1 commit into from
+100 −58

Conversation

@y9v
Copy link
Member

@y9v y9v commented Jan 23, 2026 •
edited
Loading

What does this PR do?
This PR removes lib/appsec/contrib/sinatra from steep ignored files and fixes types.

Motivation:
We want all AppSec to be properly typechecked.

Change log entry
None. This change is internal.

Additional Notes:
APPSEC-60626

How to test the change?
CI

@y9v y9v self-assigned this Jan 23, 2026
@y9v y9v requested review from a team as code owners January 23, 2026 16:55
@github-actions github-actions bot added integrations Involves tracing integrations appsec Application Security monitoring product labels Jan 23, 2026
@github-actions
Copy link

github-actions bot commented Jan 23, 2026

Typing analysis

Ignored files

This PR clears 8 ignored files. It increases the percentage of typed files from 40.63% to 41.52% (+0.89%).

Ignored files (+0-8) ✅ Cleared: 
lib/datadog/appsec/contrib/sinatra/framework.rb
lib/datadog/appsec/contrib/sinatra/gateway/request.rb
lib/datadog/appsec/contrib/sinatra/gateway/route_params.rb
lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb
lib/datadog/appsec/contrib/sinatra/integration.rb
lib/datadog/appsec/contrib/sinatra/patcher.rb
lib/datadog/appsec/contrib/sinatra/patches/json_patch.rb
lib/datadog/appsec/contrib/sinatra/request_middleware.rb

Note: Ignored files are excluded from the next sections.

Untyped methods

This PR introduces 4 untyped methods and 1 partially typed method. It increases the percentage of typed methods from 58.54% to 58.81% (+0.27%).

Untyped methods (+4-0) ❌ Introduced: 
sig/datadog/appsec/contrib/sinatra/patcher.rbs:10
└── def setup_middleware: (*untyped args) ?{ () -> untyped } -> untyped
sig/datadog/appsec/contrib/sinatra/patcher.rbs:14
└── def setup_middleware: (*untyped args) ?{ () -> untyped } -> untyped
sig/datadog/appsec/contrib/sinatra/patcher.rbs:20
└── def dispatch!: () -> untyped
sig/datadog/appsec/contrib/sinatra/patcher.rbs:26
└── def process_route: (*untyped args) { (*untyped) -> untyped } -> untyped
Partially typed methods (+1-0) ❌ Introduced: 
sig/datadog/appsec/contrib/sinatra/patches/json_patch.rbs:9
└── def json: (untyped object, ?untyped options) -> ::String

If you believe a method or an attribute is rightfully untyped or partially typed, you can add # untyped:accept on the line before the definition to remove it from the stats.

y9v
y9v commented Jan 23, 2026
View reviewed changes
sig/datadog/appsec/instrumentation/gateway.rbs
Comment on lines +20 to +23
def watch: ("sinatra.request.dispatch", ::Symbol key) { (stack, ::Datadog::AppSec::Contrib::Sinatra::Gateway::Request) -> stack_result } -> void
| ("sinatra.request.routed", ::Symbol key) { (stack, [::Datadog::AppSec::Contrib::Sinatra::Gateway::Request, ::Datadog::AppSec::Contrib::Sinatra::Gateway::RouteParams]) -> stack_result } -> void
| ("sinatra.response.body.json", ::Symbol key) { (stack, Gateway::DataContainer) -> stack_result } -> void
| (::String name, ::Symbol key) { (stack, argument) -> stack_result } -> void
Copy link
Member Author

@y9v y9v Jan 23, 2026

Choose a reason for hiding this comment

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

@Strech what do you think of this approach?

Copy link
Member

@marcotc marcotc Jan 23, 2026

Choose a reason for hiding this comment

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

I guess our "dynamically-typed" language loses its charm in RBS signatures.

But regarding this change, I don't see how it could be made much better, it looks ok I think.

The only thing we could theoretically do is move each argument type (or signature) to the integration package: e.g. declare a type for [::Datadog::AppSec::Contrib::Sinatra::Gateway::Request, ::Datadog::AppSec::Contrib::Sinatra::Gateway::RouteParams], or maybe even a type for the whole ("sinatra.request.routed", ::Symbol key) { (stack, [::Datadog::AppSec::Contrib::Sinatra::Gateway::Request, ::Datadog::AppSec::Contrib::Sinatra::Gateway::RouteParams]) -> stack_result } -> void in the Sinatra contrib RBS namespace).
Maybe that looks better... maybe not 🤷

@pr-commenter
Copy link

pr-commenter bot commented Jan 23, 2026

Benchmarks

Benchmark execution time: 2026-01-23 17:23:26

Comparing candidate commit 4b72b39 in PR branch appsec-daily-rbs-chore-23-jan with baseline commit e700369 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 44 metrics, 2 unstable metrics.

marcotc
marcotc reviewed Jan 23, 2026
View reviewed changes
lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb
def watch_request_dispatch(gateway = Instrumentation.gateway)
gateway.watch('sinatra.request.dispatch', :appsec) do |stack, gateway_request|
context = gateway_request.env[AppSec::Ext::CONTEXT_KEY]
context = gateway_request.env[AppSec::Ext::CONTEXT_KEY] # : Context
Copy link
Member

@marcotc marcotc Jan 23, 2026

Choose a reason for hiding this comment

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

Sorry if I'm out of the loop: is this the "new" syntax for type overrides? # : ...

marcotc
marcotc reviewed Jan 23, 2026
View reviewed changes
lib/datadog/appsec/contrib/sinatra/gateway/watcher.rb
Comment on lines +52 to +53
gateway.watch('sinatra.request.routed', :appsec) do |stack, args|
gateway_request, gateway_route_params = args # : [Gateway::Request, Gateway::RouteParams]
Copy link
Member

@marcotc marcotc Jan 23, 2026

Choose a reason for hiding this comment

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

It's annoying that you have to do this, just to add a type annotation. 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcotc marcotc marcotc approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@y9v y9v

Labels

appsec Application Security monitoring product integrations Involves tracing integrations

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@y9v @marcotc