ash
ash copied to clipboard
ash-project
Forbid special characters in field names
Too many extensions currently break if you add anything other than alphanumeric and underscores to an attribute/calculation/aggregate/relationship name. For now, we should simply prevent this at the resource level.
@zachdaniel what would be the best/quickest way to reproduce that behavior above? Thanks a lot!
👋🏻 Hello!
The simplest thing to do would be to add an attribute with a question mark at the end, e.g attribute :foo?, :boolean. This should fail. Then we'd add a transformer just like this one: https://github.com/ash-project/ash/blob/main/lib/ash/resource/transformers/require_unique_field_names.ex
except instead of validating that they are all unique, it would simply validate that none of them have special characters :)
I started looking into this one as my first foray into Ash.
I followed your suggestion @zachdaniel, and I managed to at least get the system to raise if I give an attribute a :name that has unacceptable characters. I tested this out by altering an existing test script:
== Compilation error in file test/actions/create_test.exs ==
** (Spark.Error.DslError) [Ash.Test.Actions.CreateTest.Profile]
The field name biöÖo contains special characters
(ash 2.0.0-rc.9) lib/ash/resource/transformers/require_alphanumeric_field_names.ex:39: anonymous fn/1 in Ash.Resource.Transformers.RequireAlphanumericFieldNames.transform/1
....
Before I look into opening a PR, I'd like to hear where tests for transformers should be placed. I couldn't quite find a suitable location for testing for my new Ash.Resource.Transformers.RequireAlphanumericFieldNames module (naming TBD). Any tips on organization? 🙂
There currently aren't any transformer specific tests, moreso integration tests on them running whenc compiling a resource. For example: https://github.com/ash-project/ash/blob/main/test/resource/attributes_test.exs
Also great work! One thing we may want to do is get some Ash user's feedback about the special characters rule, as we may want to make an exception for attributes containing a ? because some users are doing has_thing? naming conventions, and apparently that is causing less problems than I expected. However ash_json_api would almost certainly break in some cases if they used it with quesiton marks in the names. So perhaps we allow names that end in ? but separately I can add a validation in ash_json_api that requires that you give it a new name to map the attribute to for use in ash_json_api..
I could take or leave an exception for ?. I've already eliminated it from my resources IIRC.
I think this general feature of forbidding special characters to prevent footguns is the more important thing, because you can run into the problem further down the road when it will require significant refactoring to fix.
To leave an update on the issue so that it isn't forgotten: this topic was discussed further in Discord. It was pointed out by @zimt28 how this limitation on special characters ought not to be a concern of Ash core, and that it would be more fitting to have different extensions perform this. This is due to how different extensions can have different takes for what characters are or aren't acceptable, and the Ash core isn't one to decide on their behalf.
The PR https://github.com/ash-project/ash/pull/388 linked here is now about a separate concern, which is to forbid reserved names from being passed as field names.
I'll close this issue for now, actually, since we've essentially decided not to do it.