Don't overwrite ip_address if already set on user#2350

Merged

sl0thentr0py merged 1 commit intomasterfrom

neel/fix-rack-ip-overwrite

Jul 23, 2024

Member

sl0thentr0py commented Jul 22, 2024

Fixes #2347

sl0thentr0py force-pushed the neel/fix-rack-ip-overwrite branch from 88f073e to f1c79e9 Compare

July 22, 2024 11:28

sl0thentr0py requested a review from solnic

July 22, 2024 11:28


          Don't overwrite ip_address if already set on user

428c502

sl0thentr0py force-pushed the neel/fix-rack-ip-overwrite branch from f1c79e9 to 428c502 Compare

July 22, 2024 11:29

codecov bot commented Jul 22, 2024 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.66%. Comparing base (36866c5) to head (428c502).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2350   +/-   ##
=======================================
  Coverage   98.66%   98.66%           
=======================================
  Files         205      205           
  Lines       13483    13486    +3     
=======================================
+ Hits        13303    13306    +3     
  Misses        180      180

Components	Coverage Δ
sentry-ruby	`99.03% <100.00%> (+<0.01%)`	⬆️
sentry-rails	`97.41% <ø> (ø)`
sentry-sidekiq	`97.01% <ø> (ø)`
sentry-resque	`96.79% <ø> (ø)`
sentry-delayed_job	`98.92% <ø> (ø)`
sentry-opentelemetry	`100.00% <ø> (ø)`

Files	Coverage Δ
sentry-ruby/lib/sentry/event.rb	`98.52% <100.00%> (-0.03%)`	⬇️
sentry-ruby/spec/sentry/event_spec.rb	`100.00% <100.00%> (ø)`

solnic approved these changes

View reviewed changes

Collaborator

solnic left a comment

LGTM!

solnic reviewed

View reviewed changes

sentry-ruby/spec/sentry/event_spec.rb

+                    it "doesn't overwrite already set ip address" do
+                      Sentry.set_user({ ip_address: "3.3.3.3" })
+                      Sentry.get_current_scope.apply_to_event(event)
+                      expect(event.to_hash[:user][:ip_address]).to eq("3.3.3.3")

Collaborator

solnic Jul 22, 2024

@sl0thentr0py note for the future - this should be event.to_h, because to_hash is a special coercion method that's used implicitly in some places in Ruby and may cause weird issues. For example ** uses it, see:

(ruby) {foo: "bar", **event}
{:foo=>"bar",
 :event_id=>"266e3389d9b64f57b48da6f3d0969a93",
 :level=>:error,
 :timestamp=>"2024-07-22T16:22:13Z",
 :environment=>"development",
 :server_name=>"44b9b235e7c3",
 :modules=>
  {"rake"=>"12.3.3",
 # ...

It should only be used when the object that implements it is actually a hash-like object, and events are not like that.

Contributor

st0012 Jul 23, 2024 •

edited

Loading

Thanks! For context, I took it from the old SDK when implementing sentry-ruby. So we probably can make the switch rather freely. I'll put up a PR to see how it goes.

st0012 approved these changes

View reviewed changes

st0012 added a commit that referenced this pull request


          Migrate from to_hash to to_h

0f2b2e9

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

st0012 mentioned this pull request

Migrate from to_hash to to_h #2351

Merged

sl0thentr0py merged commit c16a0f2 into master

sl0thentr0py deleted the neel/fix-rack-ip-overwrite branch

July 23, 2024 12:31

st0012 added a commit that referenced this pull request


          Migrate from to_hash to to_h

a559f0d

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

sl0thentr0py pushed a commit that referenced this pull request


          Migrate from to_hash to to_h (#2351)

329cccb

* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

sl0thentr0py pushed a commit that referenced this pull request


          Migrate from to_hash to to_h (#2351)

d7522f9

* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

sl0thentr0py pushed a commit that referenced this pull request


          Migrate from to_hash to to_h (#2351)

a8bf8ac

* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

sl0thentr0py pushed a commit that referenced this pull request


          Migrate from to_hash to to_h (#2351)

12a7ff1

* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

sl0thentr0py pushed a commit that referenced this pull request


          Migrate from to_hash to to_h (#2351)

710a2f9

* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

sl0thentr0py added a commit that referenced this pull request


          6.0 major (#2352)

a1bd7e2

* Drop `async` configuration (#1894)

* Migrate from to_hash to to_h (#2351)

* Migrate from to_hash to to_h

As @solnic pointed out in #2350 (comment)
`to_hash` has special meaning in Ruby and could be called implicitly
in contexts like double splatting argument. So we should switch to `to_h`
to avoid potential issues.

* Fix specs after rebase

* Add before_send_check_in (#2703)

* Cleanup before_send to only apply to ErrorEvent
* remove the Hash deprecation message

* Archive sentry-raven (#2708)

Moved to https://github.com/getsentry/raven-ruby

* Remove stacktrace trimming from the SDK (#2714)

* Default config.enabled_environments to nil instead of [] (#2716)

* Remove :monotonic_active_support_logger (#2717)

* Add `config.trace_ignore_status_codes` to control which status codes don't get traced (#2725)

* Add config.trace_ignore_status_codes to control which status codes don't
get traced

* Change implementation to Range

* Don't send  client reports for profiles if profiling is disabled (#2728)

* Remove  and  and all metrics related code (#2729)

* Remove deprecated `config.capture_exception_frame_locals` (#2730)

* Remove deprecated capture_exception_frame_locals

* Remove deprecated config.enable_tracing (#2731)

* Remove deprecated config.enable_tracing

* Remove deprecated `config.logger` (#2732)

* Remove deprecated config.logger

* Remove deprecated Sentry::Rails::Tracing::ActionControllerSubscriber (#2733)

* Remove Transaction deprecations (#2736)

* Remove deprecated Event#configuration (#2740)

* Remove deprecated client methods (#2741)

* Bump required_ruby_version to 2.7 (#2743)

* Bump minimum rails version in `sentry-rails` to `5.2.0`
* Bump minimum sidekiq version in `sentry-sidekiq` to `5.0`
* Cleanup CI

* Remove hub from Transaction#initialize (#2739)

* Move examples out of SDK (#2746)

available at https://github.com/getsentry/examples/tree/master/ruby

* Add new config.profiles_sample_interval (#2745)

* Always set up the StructuredLogger and no-op capture_log_event if logs (#2752)

are disabled

* Make request body reading safe to rewind (#2754)

* Bump oversized stacktrace trunction to 500 on both sides

Revert "Remove stacktrace trimming from the SDK (#2714)"

This reverts commit ed7a2db.

---------

Co-authored-by: Stan Lo <[email protected]>

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

Labels

None yet