basho / cuttlefish

never lose your childlike sense of wonder baby cuttlefish, promise me?
Apache License 2.0
205 stars 124 forks source link

Two Mappings walk into a bar... expecting to meet a translation, but he never showed up. #74

Closed joedevivo closed 10 years ago

joedevivo commented 10 years ago

Here's the problem. You defined two mappings with different variable names, but you had them map to the same erlang variable. This is actually pretty easy to encounter.

Say you've included yokozuna in your application, but you hate the yokozuna namespace, so you add another mapping like this:

%% @doc The enabled Yokozuna set this 'on'.
{mapping, "search", "yokozuna.enabled", [
  {default, off},
  {datatype, {enum, [on, off]}}
]}.

Now you think you can do this in your conf file: search = on but you can't because the existing yokozuna mapping will overwrite it. The previous version of the logic for this was great at comparing "search" to other things called "search" but lousy at looking for multiple things that map to "yokozuna.enabled". Of course, it's never that simple. Sometimes you want lots of different things mapping to one erlang setting, because you're using a translation to take multiple conf values in and turn them into one erlang setting.

This PR alters the logic in the following ways:

  1. process mappings in reverse order, so if you do have two, the one that comes in the beginning of the will process last, and overwrite the first one. So, at least the scenario above would work.
  2. After schemas are read in, we'll look for any case where more than one mapping maps to a specific erlang variable. The ones that do AND don't have a corresponding translation will have all but the first occurring mapping removed.

Both of these solve the same problem in different ways, but together present a robust solution.

seancribbs commented 10 years ago

Aside from the stylistic/tightness critiques, this PR is good to go. :+1:

seancribbs commented 10 years ago

Reiterating the :+1: