berlindb / core

All of the required core code
MIT License
253 stars 27 forks source link

Issue/21 - Introduce query_var_default_value and is_query_var_default(). #35

Closed JJJ closed 4 years ago

JJJ commented 4 years ago

First pass at allowing querying explicitly for falsy values (0, '0', false, null)

This work-in-progress is a proof-of-concept at how we could potentially avoid bugs with empty() checks against query_vars.

See #21.

JJJ commented 4 years ago

I think this is worth checking out at this point.

The random default query_var value could (and probably should) be used in a few other places where empty() checks are done against '' values. Before doing that, I figured I'd wait for feedback on what's here.

The reason I think a random value is necessary, is to avoid having a default value that might unintentionally be an actual value someone would want to query the database for.

By using a random value and comparing against it (instead of an empty default string) row values such as '', 0, "0", false and null should now be able to be queried for literally.

JJJ commented 4 years ago

Hey reviewers! 👋

Would love some eyes and feedback on this one whenever you have an opportunity. ❤️

JJJ commented 4 years ago

Honestly, I'm not sure. In my testing, I also tried out the methodology mentioned in your comment here #21 (comment)

Your approach there is logical.

In this particular circumstance, it worked just as well, and I found it to be a bit easier to read. Are there situations where an isset check could actually fail that I'm missing?

That's a great question. Technically, no. But also kinda! This PR takes your suggestion and applies it to all default values, not just falsy ones, so the same collision avoidance logic is now universal.

The only (minor) concern I have is what happens if the random value I want to save happens to be the same value as the random default? I know this is unlikely, but it could happen. Are the odds so astronomically low that this is not a concern?

This is always possible, but yes... astronomically low. We're talking Rick and Morty multiple identical dimensions running the exact same code and still getting different results kinda low. Enough monkeys could potentially type out the same random value after like a billion years. 🐵

alexstandiford commented 4 years ago

That's a great question. Technically, no. But also kinda! This PR takes your suggestion and applies it to all default values, not just falsy ones, so the same collision avoidance logic is now universal.

Oh, that makes a lot of sense!

JJJ commented 4 years ago

Removing reviews from Drew and Ashley to lighten the load here.

Thanks @alexstandiford for the eyes.

Merge imminent.