IndiffererentAccess - Adaptation of HashWithIndifferentAccess to Elixir Maps/Plug

So I’m looking for reactions and alternatives, and I know a lot of them will be negative/some of mine are too…

I just threw together this code in an hour or two after frustration with a bug in my project where one dev used a string-keyed map in some business logic where some existing code assumed it could use an atom… and we have a number of similar situations elsewhere, where we fail to do anything to the params at the Router or Controller level and they leak, unchanged, into deeper business logic modules.

Arguably that’s the problem itself, and I know a lot of you (probably correctly) will think this is a terrible idea. I think it might be a terrible idea. But I also wanted to see how hard it was to do and have a discussion here over alternatives, tradeoffs, and whether it’s fundamentally bad in any important way or just non-idiomatic, and if, as I’m sure many will think, it’s non-idiomatic, what’s the right alternative that tolerates mistakes and oversights and doesn’t have a lot of ceremony or boilerplate?

One obvious issue with this is the overhead… another is that Map’s are enumerable and if you use this on a Map you enumerate over, well, things will be a bit weird by default. What other issues come to mind? Anyone willing to say they like it/try it out in a personal project and see how they feel after using it a tiny bit/pointing to specific places it works well or badly, aside from the general concerns over being idiomatic?

I lean towards being pluralist and even if I don’t use this myself going forward, I wanted to see what it looked like when done/how hard it was, and the only comparable libraries I found were https://github.com/vic/indifferent and https://github.com/philosodad/morphix and neither seemed to do quite as simply what I had in mind… and it was easy enough I thought i should share the result and see if I learn anything, or if people surprise me and actually like it :upside_down_face:

Anyway here’s the github https://github.com/bglusman/indifferent_access and the hex package https://hex.pm/packages/indifferent_access

2 Likes

I’ve only skimmed the implementation, but it seems as if it builds on a one-time creation of stringified/atomized counterkey with same value.

This seems to make things even worse…

One part of code updates atom key, another reads string key.

And even if you remember to re-key… Which should win if both exist?

The only solution are proper conversion to structs. No more arguing about string vs. atoms, just structs. Part of the contract, checked by dialyzer.

2 Likes

Yeah, to be fair this is an attempt to make the behavior backwards compatible to allow to gradually ensure the logic works, but if it were to go forward I imagine it would be a good/simple option to have to allow for replacing string keys that exist as atoms with atom keys pointing at same/indifferentized value… but I didn’t want to go too far down any path without talking and thinking about implications and tradeoffs.

As for re-keying, not sure if I understand your question/point, but it used Map.put_new so if there’s an existing atom key it never overwrites that, it only adds an atom key in addition. But maybe you meant something else?

I also had a version that left string keys with their unchanged values and only updated the value recursively on the atom key, but then thought if supporting both maybe best to do so consistently in both “paths” but, that would also make for a reasonable config/option if extending.

I still haven’t gotten a chance to really use Dialyzer since the Dialyxir updates I’ve heard have made it a lot more usable… maybe I’ll try it out on this as a simple little toy project without too much surface area, dunno… in any case, appreciate your feedback.

You will only introduce confusion by using this IMO.

What kind of a legacy problem are you trying to solve? Indeed, as @NobbZ suggested, convert the incoming controller parameters to a struct as quickly as you can so the compiler can cover part of the possible set of problems, and to be able to use Ecto as a validator as an added bonus.

What you suggest doing with this library is not just non-idiomatic; it’s terrible no matter what language you are doing it in. It increases the possible bug surface.

Let’s take a simple map, %{a: 1}, which you indifferentize. %{"a" => 1, a: 1} and then update "a", %{"a" => 2, a: 1}, now you have 2 conflicting values for the “same” key.

Your solution is not a solution, it’s changing pest for Cholera.

Always normalize the input at the edges of your system. In the system there is rarely a use case to assume non normalized data, and normalized data is nearly always a struct.

I agree I want to normalize at the edge of the system, but that’s not what I have in an existing substantial code base. I have a number of roadmap items to change some key params into an embedded_schema, which I see as helping to benefit from Ecto changeset and cast/dump behavior centralized well, but I’m not totally sold and/or educated/adapted to the idea that all params should immediately be changed into a Struct, and if I were, I don’t know what Structs those would be or how they’d change over time/which controllers would share them vs every controller having a slightly different one… (though to be fair, the bug that inspired this was a map pulled out of a param and passed on, and I suppose the same concern applies there even if the entire map isn’t treated this way, any value in it might be a map and get the same issues)… also though the point of using this would basically be that you’d only intend to work with the atom keys going forward, the string keys are there (for now) as a legacy/gradual adoption feature so its possible to drop this into existing code and not break things that rely on string keys until you’ve had a chance to move them over… but perhaps it’s better that they break loudly right away to make fixing them to use atoms a straightforward path of fixing failing tests and errors… again, I’m not at all sold on this general idea or this specific implementation, but these are good concerns to discuss for sure for anyone who does consider using it.

I also don’t think though that controller params, which is all this is intended to be used with, should really ever be updated in that sense… i think things are pulled out of them, perhaps with default values, but I don’t beleive the params map should be used wholesale as a map for any other purpose, and if only used there and never updated directly as you suggest, the “rekeying” problem is not much of a problem I think… not to say the other points aren’t valid, but, since I put this up for discussion, I’d like to make sure we’re actually discussing the same use case and reasonable concerns, not purely theoretical ones…

I was sort of surprised at the API here.

I expected to see a IndifferentMap.get function that checks for atom and string keys, while using the normal map functions for putting and updating.

1 Like

Yeah the other libraries I linked to may be more like that IIRC but I wanted to try something that “just worked” transparently via a plug for some or all pipelines in an app and was easy to try out and see without littering code with references to or dependencies on it… That said, obviously people (myself included) have some legitimate qualms with this approach but I threw together quickly and may never touch again…

Oh! But maybe there’s a fun idea there for implementing instead a struct named like you suggest that implements access behavior and does that… Then this plug could initialize that struct with params map… Maybe I’ll try that as an alternate iteration! Thanks!

Have you tried to pattern match?

def has_my_atom_key(%{mykey: _} = params) do
  params
end
def has_my_atom_key(params) do
  raise Brah, "We got a problem"
end

I find thats a trivial way to catch bugs, it’s not as good as a strong type system, but it’s the best we got.

1 Like

I do pattern match quite a lot! Not entirely sure how/if something like that would scale to every usage and access to a different key in a different params map across dozens of controllers, and the purpose/scope of this library/discussion is sort of looking for patterns that are a tiny bit more “automatic” and or hard to mess up/forget than something like that, but… No doubt if we’re in Elixir pattern matching probably is/could be part of the answer!

The bug is passing string keys into business logic and not that the business logic expected atom keys, but you’re trying to change the latter instead of the first problem. User input (most often string keyed maps) should only ever land at a place where it’s Ecto.Changeset.cast or otherwise converted to an internal data structure. I would even say modifying those maps (e.g. add something to it) before “casting” to be a smell.

The other place for string keyed maps is if you need keys, which are user defined. But then your whole business logic needs to work with just string keys, because you never want to convert arbitrary user provided keys to atoms.

3 Likes

Yeah, I think I agree and that was more of my first thought, but coming from Rails one of the few things I missed was the convenience of HashWithIndifferentAccess… I’m inclined to agree it’s a smell and/or bug, but I’ve not seen a production code base without a fair amount of both, and in any case, I was curious what a semi-close convenience would look like in Elixir without risk of atom-exhaustion. In Rails HWID stores all keys as strings and just converts symbols to strings on lookup… In some respects I think this tension is part of the fundamental dynamic/static type system tension we all struggle with a bit, but I def. agree this library (especially as it is now) is nothing like the “right” solution/didn’t think so when I made it, but I’m still a little conflicted whether there’s a reasonable compromise that’s more automated/lighter weight than all ecto schemas and structs everywhere and/or a graceful path that starts with whatever terrible working code you have and lets you move gracefully and gradually towards that (or another solution) with small working iterations along the way.

This is not the way I’d do it. ^.^;

First of all, needs a wrapping defstruct to ensure that map calls on it will corrupt and kill it properly. Need to implement Access over it among a couple of others, etc…

Overall, this is super hard in Elixir because it lacks static types, I can’t think of any way that would reliably work without forcing all access to go through module calls (which just wrapping it in, say, a tuple instead of a struct, like a defrecord, would prevent Map.* calls from succeeding on it, you can’t implement Access on defrecord's though because of the way Elixir protocols are slightly misdesigned for that purpose, although I have a fixed defprotocolex that would work, but the built-in Access obviously would not use it ^.^;).

1 Like

Thanks man, yeah I’m not sure how/if I’d do it for real, but was an interesting experiment to try/doesn’t obviously break as many things in my app as I thought it might… few failed tests I haven’t looked at/debugged, but most passed…

Access used to be a protocol, but its a behavior now right? performance reasons I heard? Though doubt that changes anything…

I’ve meant to play around with defprotocolex, maybe this’ll give me an excuse at some point :wink:

Just curious if anyone thinks this would be more OK/interesting if it dropped the string keywords and replaced with atom keywords whenever the atom existed (and wasn’t already a seperate key), instead of leaving both? That would be a pretty trivial change/maybe better default behavior (though it would no longer be “indifferent” access to be fair) but I’m sort of taking a shotgun approach here (in more than one sense) and I think I’d like that better on reflection… but dunno, maybe it’d just be throwing good time after bad to experiment any further with this idea. (Edited: literally took about 30 seconds so that’s up as a PR https://github.com/bglusman/indifferent_access/pull/1 though really I’d probably pull the behavior out into configuration with a couple options like those two if were doing this for real)

It really sounds like you trying to solve the wrong problem here.

Maybe a better thing is to take a few steps back and, look at why a developer would end up in this situation, in the first place; I feel like all of this can be solve by good design, but that is sometime harder than said.

2 Likes

I have a sort of pluralist mindset… I don’t expect or beleive there’s one-true solution to any problem and like to see a few and then choose between depending on constraints and other factors… so i’m not even exactly trying to solve “a” problem, but would like some options on the table to make working with params, with AND without changesets and structs, more ergnomic… I’m not pitching this as a good solution let alone the one true solution, but I’m trying to decide if it has any place in the landscape of tools and approaches I and other devs take. :man_shrugging: we’ll see. I tried to make it clear up front I wasn’t in love with and didn’t really expect much of anyone else to be either, but wanted to provoke some discussion and hopefully be pointed to some concrete, non-abstract pieces of advice or prior art dealing with the kinds of things I see in my production code base better/differently… When I look around at a few open source Phoenix apps I still see a fair amount of raw strings in their controllers, and I think there has to be a better way is all.

This is not about that. It’s about introducing a module/structure that has an implicit and thus potentially surprising behaviour.

That’s hard because none of us can point you at our own production code, it’s all company trade secrets, you know. The “abstract” piece of advice you receive, you seem to be not impressed with so you’ll have to discover your own truths along the way.


As an anecdotal evidence, I have 10+ times successfully introduced big refactorings in Elixir code bases where the PR shows, say, 1650 lines added / 1280 lines removed and I still introduced zero bugs. I am not special or particularly amazing at programming though; good test suite and coding style consistency can and do work wonders, even in dynamic languages like Elixir.

You are aiming at the wrong problem. You can rework your code base to not rely on such arcane surprising mechanics and your project will be better for it. I’d argue the energy spent on such refactoring will end up being less than the energy put into making sure your surprise mechanics work well.

It seems you are dead-set on actually going forward with this though, so maybe all of us here trying to convince you to give it up are barking up the wrong tree. :003:

2 Likes

What on earth could give you that impression? I’ve tried to pretty clearly convey the opposite above everywhere I can, but also that I enjoy the discussion/dialogue/challenge and feel there’s still a lack of specificity to some of the alternatives put forwards besides “use structs” etc. I’m in no way set on using this for real in any way, I’m just still playing with ideas and thoughts because its fun! :smile: