fix: do not fail when case insentive enabled while having a complex context object

[gastonfournier]

About the changes

• Commit 62b0bfd includes a test to validate the failure scenario: https://github.com/Unleash/unleash-client-ruby/actions/runs/10595256314/job/29360627856?pr=191#step:7:1070.
• Commit 0076e2d should fix the problem

Closes #189

[rarruda]

For the sake of preventing regressions, would it be possible to implement the failing test in rspec?

[gastonfournier]

For the sake of preventing regressions, would it be possible to implement the failing test in rspec?

Absolutely, I didn't know we had proper unit tests in this project, I was trying to find them and I was unaware they were under spec folder. Just asked a colleague and they point me in the right direction. I was expecting a test folder or files prefixed with test.

[rarruda]

I totally understand! No sweat. rspec uses spec, and i think minitest uses test by default. Just trying to nudge good practices here. 🙂

Unfortunately I'm too time constrained to be of more hands-on help at the moment. 🙁

[coveralls]

Pull Request Test Coverage Report for Build 10612694617

Details

• 5 of 5 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.002%) to 97.156%

---

Totals
Change from base Build 10179193838:	0.002%
Covered Lines:	2562
Relevant Lines:	2637

---

💛 - Coveralls

[gardleopard]

LGTM

[gastonfournier]

We can validate what other SDKs are doing, we're aiming for consistency over correctness at the moment. I'll take some time to collect that data, because that idea also crossed my mind, and I believe the correct thing would be to return false if not a string...

This is what I found:

1. JS client assumes it's a string and applies toString: https://github.com/Unleash/unleash-client-node/blob/929f14839d573aa6a61aff9074a80eb6167fd4c5/src/helpers.ts#L22 if you send an object this results in this string "[object Object]", it may cause some weird cases if you're constraint checks for the word object
2. Java client does not allow non string properties: https://github.com/Unleash/unleash-client-java/blob/4188c4b723a995587510b9cd0e8fd7567d55e3db/src/main/java/io/getunleash/UnleashContext.java#L19
3. PHP also supports string types only https://github.com/Unleash/unleash-client-php/blob/7353c74e1efde40634379282711bdb224df9a857/src/Configuration/UnleashContext.php#L65-L70

So, based on JS client which is the one accepting complex objects I think we can follow your suggestion and return false

[sighphyre]

The magic is now in the line above isn't it? This can change can get rolled back?

[rarruda]

Indeed the fix is in the line above. ( https://github.com/Unleash/unleash-client-ruby/pull/191/files#diff-aa0ebe0760222c549034e2dfab10a550f29c632fcc9919cc84cb0e7675a46409R112 )

These changes are mostly a lint/simplification. ( https://github.com/Unleash/unleash-client-ruby/pull/191/files#diff-aa0ebe0760222c549034e2dfab10a550f29c632fcc9919cc84cb0e7675a46409R114-R117 ). So, nice to have, but not fixing a problem per-se.

[gastonfournier]

#192 maybe this simplifies the if? It's needed but it's also having side effects on the values which I'd like to prevent

[sighphyre]

Nice, LGTM

[sighphyre]

We can validate what other SDKs are doing, we're aiming for consistency over correctness at the moment. I'll take some time to collect that data, because that idea also crossed my mind, and I believe the correct thing would be to return false if not a string...

This is what I found:

1. JS client assumes it's a string and applies toString: https://github.com/Unleash/unleash-client-node/blob/929f14839d573aa6a61aff9074a80eb6167fd4c5/src/helpers.ts#L22 if you send an object this results in this string `"[object Object]"`, it may cause some weird cases if you're constraint checks for the word `object`

2. Java client does not allow non string properties: https://github.com/Unleash/unleash-client-java/blob/4188c4b723a995587510b9cd0e8fd7567d55e3db/src/main/java/io/getunleash/UnleashContext.java#L19

3. PHP also supports string types only https://github.com/Unleash/unleash-client-php/blob/7353c74e1efde40634379282711bdb224df9a857/src/Configuration/UnleashContext.php#L65-L70

So, based on JS client which is the one accepting complex objects I think we can follow your suggestion and return false

Context properties are always strings, regardless of what the surface level APIs in the SDKs may suggest. It's not clear to see in the dynamically typed languages because they can often skip the contextual parsing required in the constraints that the statically typed languages have to do but that leads to... well... this problem.

In this case, this is a broken constraints because it can't be cast to the type that should be inferred from the local context provided by the constraint type so... +1 for return false

[rarruda]

This PR looks awesome! 🚀