Add connectors ->keysToCamelCase arrow method#9131
Add connectors ->keysToCamelCase arrow method#9131andrewmcgivery wants to merge 7 commits intodevfrom
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 9589eea08382d53f65951245 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
@andrewmcgivery, please consider creating a changeset entry in |
There was a problem hiding this comment.
Some quick thoughts:
- I think we should call this
->keysToCamelCasesince we're only renaming the keys (and it's also a bit shorter) - It's surprising to me that the recursive version is the default, though I see why it's useful. Can we swap the default but keep the option to rename recursively?
- Is it problematic that
->keysToCamelCasedoes not specify which casing style it expects as the input? Maybe we need->keysFromSnakeToCamelCase,->keysFromCamelToSnakeCaseand so forth? - If that seems like too much, we could let the input/output styles be passed in as arguments, like
->convertKeyCase("snake", "camel")
->convertKeyCase("kebab", "snake")- I think we can safely worry about input shapes later. The subselection after the rename will determine the output shape for now.
|
One more thought: it could be expensive/undesirable to rename all keys in a large object, so we might want some way to specify a subset of keys that we want to rename? Possible sketch: |
|
Given the user's stated problem is that they do not want to have to manually rename all of the properties in large objects by hand, I would assume the behaviour that they want from the function is to rename all of the properties. 😆 Having to type out the property to rename and the new property name like this defeats the purpose of why we're creating this function in the first place. Typing this: Is just as much boilerplate as this: The purpose of this function is that the user does not want to type out the property names. :) |
|
@benjamn to answer your question on slack, the function uses the heck crate internally which already detects all the common cases automatically. :) |
|
You wouldn't have to specify specific property names in most cases, but I think developers are going to find themselves in situations where they don't want all properties renamed, but only certain ones, and they will want the ability to make the renaming more precise. I do not agree this defeats the purpose of the function. I'd still like to hear more about how we're going to infer the input style, so we can have |
If I only want to rename specific properties, why would I use this function at all when I can just use our normal renaming syntax? |
|
Maybe because you want most of your snake case properties to be renamed to camel case, except for one? If I guess that's fine? |
That definitely seems like an edge case... I'd prefer to design for the vast majority of the time:
And yea, they've always got that escape hatch in the mapping if for some odd reason they want to rename back lol |
| // Determine recursive flag from literal argument if possible | ||
| let recursive = if let Some(first_arg) = method_args.and_then(|args| args.args.first()) { | ||
| use crate::connectors::json_selection::lit_expr::LitExpr; | ||
| match first_arg.as_ref() { | ||
| LitExpr::Bool(b) => *b, | ||
| _ => false, // Default to false for non-literal args | ||
| } | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
Just noting this struggles with variables/expressions like obj->keysToCamelCase($args.recursive)? I see how that's not so helpful for shape computation, though, since you don't know what the value will be at runtime, so unfortunately I don't have a good solution (you're already doing the right thing in keys_to_camel_case_method).
Would it be any easier to just have two methods, ->keysToCamelCase and ->keysToCamelCaseDeep? Then there's no argument so it can't be an expression?
There was a problem hiding this comment.
I see what you mean... in the method we actually allow an expression with apply_to_path but here we're assuming it's a boolean or default to false.
It's probably an unlikely scenario but it is definitely strange and inconsistent. In practice the recursive argument (probably) cannot or will not be dynamic because it is (probably) being used to map to schema, which is fixed. So I would say this is a pretty big edge case given the intended usage. But I don't love this.
The more I'm thinking about it as I'm typing this though, I suspect the easiest is having two methods... but I think I'm going to go with ->keysToCamelCase (shallow version) and ->keysToCamelCaseDeep.
Good call!
benjamn
left a comment
There was a problem hiding this comment.
Here are some potential issues that Claude flagged, both fixable:
1. Collision error message reports the wrong key
In transform_keys, the collision error tries to show which original keys collided:
new_map.iter()
.find(|(k, _)| k.as_str() == camel_key)
.map(|(k, _)| k.as_str())
.unwrap_or("")But new_map is keyed by the already-converted camelCase keys, so this lookup just finds camel_key itself. Given {"foo_bar": 1, "fooBar": 2}, the error reads:
"fooBar" and "fooBar" both map to "fooBar"
…when it should say "foo_bar" and "fooBar". The original key is lost after insertion. A fix would be to track a camel_key → original_key side-map so you can report the source key that caused the first insertion.
The test for this (should_warn_on_key_collision) only asserts contains("key collision"), so it doesn't catch the problem.
2. heck strips leading underscores — potentially surprising in GraphQL contexts
heck's to_lower_camel_case strips leading (and trailing) underscores:
| Input | Output |
|---|---|
_id |
id |
__typename |
typename |
_private_field |
privateField |
__typename is a GraphQL introspection field, and _id is common in MongoDB/REST responses — silently dropping the underscores could break downstream selections or cause confusing data loss.
A few options:
- Preserve leading underscores by stripping them before conversion and prepending them back after
- Emit a warning/error when a key's leading underscores would be stripped
- At minimum, document the behavior prominently so users know what to expect
First one is now fixed! Second one I think is a red herring because like we discussed earlier, the "best practice" in graphql is For the 3 examples given...
|
Adds a new
->keysToCamelCasemethod to convert all the properties of an object from various cases to camelCase. Shallow by default but can be made recursive with-> keysToCamelCase(true).Example usage:
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩