launchdarkly / ruby-server-sdk

LaunchDarkly Server-side SDK for Ruby
https://docs.launchdarkly.com/sdk/server-side/ruby
Other
34 stars 52 forks source link

Warn when a custom attribute key conflicts with a built-in attribute #199

Closed gnomonunes closed 2 years ago

gnomonunes commented 2 years ago

I've using LaunchDarkly Ruby SDK at work for a while, and recently I noticed that if a custom attribute has the same key as a built-in attribute (e.g. country), it looks like that custom attribute is ignored during flag evaluation.

Currently there's no warning about that when passing a custom attribute which conflicts with a built-in one.

Could such warning be implemented?

Another solution could be to expose the built-in attribute keys somehow so we can do the validation on our implementation. It looks like the built-in attributes are defined here but they are private: https://github.com/launchdarkly/ruby-server-sdk/blob/7abbd65986eb311ce23cf092ed93734a012eb346/lib/ldclient-rb/impl/evaluator_operators.rb#L92

keelerm84 commented 2 years ago

Thank you for the feedback @gnomonunes. I appreciate the concern you've raised, and I can see how it would be confusing if you didn't realize, for example, that :avatar was a built-in.

Could such warning be implemented?

A warning would have to be implemented during evaluation as that's when we know which key you are trying to access. As evaluation is the hottest code path in this SDK, we try to avoid any extraneous computations or checks. Additionally, the amount of log information this would generate for some customers would likely necessitate a log throttling component for this specific warning.

Another solution could be to expose the built-in attribute keys somehow so we can do the validation on our implementation.

While the attributes aren't publicly exposed via the SDK, we do document this custom vs built-in attribute collision in our documention. The specific list of built-in attributes can be found here. Hopefully this helps allow you to write any of the custom validation logic needed.

gnomonunes commented 2 years ago

Hey @keelerm84, thank you for the quick response.

I did find that list of built-in attributes in the documentation and I'm using those to validate the custom attributes on our side. My only concern is if new attributes are added or removed in the futures, we would need to be aware to update our code, while pointing to a list exposed by the SDK wouldn't require any change. But that's not a big deal I guess, we can rely on the documentation for now.

eli-darkly commented 2 years ago

@gnomonunes We have not added any new built-in attributes in the past, for exactly the reason you mentioned: it would be an unpredictable breaking change for our customers.

Our plans in the future are generally to do the opposite— having fewer attribute names designated as built-in/reserved. I can't promise that there will never be a new reserved name, but if so, it will be part of a broader product-level change that will be called out in a very prominent way.

gnomonunes commented 2 years ago

@eli-darkly Thank you for the further clarification. 👍