fix: prevent unintentional modification of arguments

[coneill-enhance]

What does this PR do?
Fixes what appears to be a bug

Motivation

1. The original code looks as though this was the intention[1]
2. It's bad practice to unintentionally modify the parameters of a function

*1: In the original code:

          user_options = user.dup
          user_id = user.delete(:id)

          raise ArgumentError, 'missing required key: :user => { :id }' if user_id.nil?

          Kit::Identity.set_user(trace, id: user_id, **user_options)

Kit::Identity.set_user(trace, id: user_id, **user_options) would be redundant as :id exists in user_options and would replace the previously specified argument

Additionally, if this was intended, then the following is equivalent and better:

          user_id = user.delete(:id)

          raise ArgumentError, 'missing required key: :user => { :id }' if user_id.nil?

          Kit::Identity.set_user(trace, id: user_id, **user)

or, not equivalent, but better still:

          raise ArgumentError, 'missing required key: :user => { :id }' if user[:id].nil?

          Kit::Identity.set_user(trace, **user)

in all cases you still get a non-descriptive error if user is nil

Also checking for user.id here is redundant, and somewhat irrelevant as this is done again by:

dd-trace-rb/lib/datadog/kit/identity.rb

Line 33 in 1d20aa8

raise ArgumentError, 'missing required key: :id' if id.nil?

So probably just go with:

          Kit::Identity.set_user(trace, **user)

[coneill-enhance]

Just a note but it feels like the trace parameter could default to Datadog::Tracing.active_trace

[coneill-enhance]

Also it might be worth invoking symbolize_keys transform_keys(&:to_sym) for user convenience.

Edit: forgot symbolize_keys isn't a ruby method

[GustavoCaso]

@coneill-enhance Thank you so much for the code changes.

It makes total sense not to modify the original user object. As for the suggestions to change the internal code to make the API easier to work with are also legit. Going to check some internal documents regarding the internals of the API, and if there is room to make those adjustments, I will make sure to add them.

[GustavoCaso]

A suggestion. It would be good to modify the test in spec/datadog/kit/appsec/events_spec.rb to ensure the user object does not get modified when calling track_login_success.

If you do not have the bandwidth to tackle it, not a problem. Let me know. I would happily add them myself.

Again, thanks for the contributions @coneill-enhance

[coneill-enhance]

A suggestion. It would be good to modify the test in spec/datadog/kit/appsec/events_spec.rb to ensure the user object does not get modified when calling track_login_success.

If you do not have the bandwidth to tackle it, not a problem. Let me know. I would happily add them myself.

Again, thanks for the contributions @coneill-enhance

No problem. I'll add a test.

[coneill-enhance]

@GustavoCaso

1. Test failing on f57067b, as expected
2. Test passing on fd5ce77

Note:
It doesn't look like ci/circleci: test-2.3 is related to my change.

Failures:

  1) Rails integration tests for an application with a basic route GET request with an event-triggering request in IP behaves like a trace with AppSec events is expected not to be empty
     Failure/Error: it { expect(spans.select { |s| s.get_tag('appsec.event') }).to_not be_empty }
       expected `[].empty?` to be falsey, got true
     Shared Example Group: "a trace with AppSec events" called from ./spec/datadog/appsec/contrib/rails/integration_test_spec.rb:348
     # ./spec/datadog/appsec/contrib/rails/integration_test_spec.rb:242:in `block (4 levels) in <top (required)>'
     # ./spec/datadog/tracing/contrib/support/tracer_helpers.rb:94:in `block (2 levels) in <module:TracerHelpers>'
     # ./spec/spec_helper.rb:226:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:118:in `block (2 levels) in <top (required)>'
     # /usr/local/bundle/gems/webmock-3.13.0/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
     # /usr/local/bundle/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

Finished in 1 minute 50.21 seconds (files took 1.32 seconds to load)
330 examples, 1 failure

[GustavoCaso]

Amazing work @coneill-enhance

Yeah, the test is not related to your change. We are aware that we have some flaky tests on our end. Not worry I triggered another run.

Once the CI is green I will merge the PR.

Thanks for your contribution.

[GustavoCaso]

Thank you so much for your contribution.

[GustavoCaso]

@coneill-enhance. We have released 1.10.1 with the fix you summited.

Thanks for your contribution.

[coneill-enhance]

@coneill-enhance. We have released 1.10.1 with the fix you summited.

Thanks for your contribution.

Thank you for the update, and thank you for providing the service, DataDog is incredibly useful.