Implement toString connectors method #7888

andrewmcgivery · 2025-07-14T20:21:01Z

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

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. ↩

apollo-librarian · 2025-07-14T20:21:05Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 59397bcbd3f5548f2a67810e

github-actions · 2025-07-14T20:21:12Z

@andrewmcgivery, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

dylan-apollo · 2025-07-16T17:03:42Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+/// $(42)->toString()         results in "42"
+/// $("hello")->toString()    results in "hello"
+/// $(true)->toString()       results in "true"
+/// $(null)->toString()       results in "null"


I think this should be empty string, actually, to match how our string templates (URL & headers) work. Ideally "{thing}" in a string template would be identical to thing->toString() in the rest of code. Then if we add in the ability to do template strings in body/selection/whatever as a way of formatting, that would also be consistent.

I think this is a good case for us to have a single function which converts a JSON value to string and use that in all the places that need to be consistent. So call the same function from here and

router/apollo-federation/src/connectors/json_selection/methods/public/join_not_null.rs

Line 74 in f834eba

fn to_string(value: &JSON, method_name: &str) -> Result<Option<String>, String> {

and

router/apollo-federation/src/connectors/string_template.rs

Line 257 in 8365edf

pub(crate) fn write_value<Output: Write>(

Suggested change

/// $(null)->toString() results in "null"

/// $(null)->toString() results in ""

So this is accurate now?

dylan-apollo · 2025-07-16T17:04:27Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+/// $("hello")->toString()    results in "hello"
+/// $(true)->toString()       results in "true"
+/// $(null)->toString()       results in "null"
+/// $([1,2,3])->toString()    results in "[1,2,3]"


This should be an error, for the same reason above. ->jsonStringify will give the json representation, and ->joinNotNull will give CSV, so whichever format they wanted, they can explicitly ask for.

Yea, forgot to update this! 😩

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

dylan-apollo · 2025-07-16T17:05:44Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+        JSON::Array(_) | JSON::Object(_) => {
+            return (
+                None,
+                vec![ApplyToError::new(
+                    format!(
+                        "Method ->{} cannot convert arrays or objects to strings. Use ->jsonStringify instead",
+                        method_name.as_ref()
+                    ),
+                    input_path.to_vec(),
+                    method_name.range(),
+                )],
+            );
+        }


Ah, the docstring up top was just wrong about arrays, so we'll want to correct that.

dylan-apollo · 2025-07-16T17:10:27Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+fn to_string_shape(
+    method_name: &WithRange<String>,
+    method_args: Option<&MethodArgs>,
+    _input_shape: Shape,


Suggested change

_input_shape: Shape,

input_shape: Shape,

dylan-apollo · 2025-07-16T17:11:57Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+    }
+
+    #[test]
+    fn to_string_shape_should_return_string_for_any_input() {


Suggested change

fn to_string_shape_should_return_string_for_any_input() {

fn to_string_shape_should_return_string_for_int_input() {

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

dylan-apollo · 2025-07-16T17:14:19Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+        assert_eq!(
+            get_shape(vec![], Shape::tuple([], [])),
+            Shape::error_with_partial(
+                "Method ->toString cannot convert arrays or objects to strings. Use ->jsonStringify instead"


Suggested change

"Method ->toString cannot convert arrays or objects to strings. Use ->jsonStringify instead"

"Method ->toString cannot convert arrays or objects to strings. Use ->jsonStringify or ->joinNotNull instead"

I think in some cases, joining to like a,b,c is what the user might be looking for.

Complete side note, we should probably start including docs links somehow for some of these errors 🤔. Something to investigate at some point.

…re converting a JSON to a string, null returns blank instead of "null"

dylan-apollo

Added a few more suggestions, other than those this LGTM

dylan-apollo · 2025-07-18T16:44:55Z

apollo-federation/src/connectors/json_selection/helpers.rs

+pub(crate) fn json_to_string(json: &JSON) -> Result<Option<String>, String> {
+    match json {
+        JSON::Null => Ok(None),
+        JSON::Bool(b) => Ok(Some(b.to_string())),
+        JSON::Number(n) => Ok(Some(n.to_string())),
+        JSON::String(s) => Ok(Some(s.as_str().to_string())),
+        JSON::Array(_) | JSON::Object(_) => {
+            Err("cannot convert arrays or objects to strings.".to_string())
+        }
+    }
+}
+


Suggested change

pub(crate) fn json_to_string(json: &JSON) -> Result<Option<String>, String> {

match json {

JSON::Null => Ok(None),

JSON::Bool(b) => Ok(Some(b.to_string())),

JSON::Number(n) => Ok(Some(n.to_string())),

JSON::String(s) => Ok(Some(s.as_str().to_string())),

JSON::Array(_) | JSON::Object(_) => {

Err("cannot convert arrays or objects to strings.".to_string())

}

}

}

pub(crate) fn json_to_string(json: &JSON) -> Result<Option<String>, &'static str> {

match json {

JSON::Null => Ok(None),

JSON::Bool(b) => Ok(Some(b.to_string())),

JSON::Number(n) => Ok(Some(n.to_string())),

JSON::String(s) => Ok(Some(s.as_str().to_string())),

JSON::Array(_) | JSON::Object(_) => Err("cannot convert arrays or objects to strings."),

}

}

The callers don't even need a String it looks like, so might as well skip the allocation here.

dylan-apollo · 2025-07-18T16:45:13Z

apollo-federation/src/connectors/json_selection/helpers.rs

    }
 }

+pub(crate) fn json_to_string(json: &JSON) -> Result<Option<String>, String> {


Can you add a doc comment /// about what this function is for & why?

dylan-apollo · 2025-07-18T16:46:44Z

apollo-federation/src/connectors/json_selection/methods/public/to_string.rs

+/// $(42)->toString()         results in "42"
+/// $("hello")->toString()    results in "hello"
+/// $(true)->toString()       results in "true"
+/// $(null)->toString()       results in "null"


Suggested change

/// $(null)->toString() results in "null"

/// $(null)->toString() results in ""

So this is accurate now?

dylan-apollo · 2025-07-18T16:51:20Z

apollo-federation/src/connectors/string_template.rs

-            return Err("Expression is not allowed to evaluate to arrays or objects.".into());
-        }
+    match json_to_string(value) {
+        Ok(result) => write!(output, "{}", result.unwrap_or_default()),


This write! stuff is actually a bit silly now... the whole reason I made it this way was to avoid allocating the extra String for each value, but json_to_string is doing that anyway now....

Maybe not worth refactoring all the code, though, to deal with it.

The "right" way is probably to have our own Value type with impl Display on it (which would either write to buffer (efficient) or allocate a new string (🤢) as needed. I'm still assuming we want that some day, but not today.

Perhaps could be refactored in a future PR as needed? :D

dylan-apollo · 2025-07-18T16:54:06Z

apollo-federation/src/connectors/json_selection/methods/public/join_not_null.rs

                "Method ->{} requires an array of scalar values as input",
                method_name


Suggested change

"Method ->{} requires an array of scalar values as input",

method_name

"Method ->{method_name} requires an array of scalar values as input",

Since you're here 😁

Implement toString connectors method

35ac084

Update toString to return error on arrays or objects

a0e510e

andrewmcgivery marked this pull request as ready for review July 14, 2025 21:49

andrewmcgivery requested a review from a team as a code owner July 14, 2025 21:49

dylan-apollo requested changes Jul 16, 2025

View reviewed changes

andrewmcgivery added 4 commits July 17, 2025 13:29

Create shared json_to_string helper method and use it everywhere we a…

09eee0a

…re converting a JSON to a string, null returns blank instead of "null"

Small adjustments

475eb06

Added unknown and name shape tests

ec89997

Update error message on to_string

71609dd

dylan-apollo approved these changes Jul 18, 2025

View reviewed changes

Code review feedback

3e94fe2

andrewmcgivery merged commit 607c6ec into dev Jul 18, 2025
15 checks passed

andrewmcgivery deleted the am/tostring_method branch July 18, 2025 20:44

	/// $(null)->toString() results in "null"
	/// $(null)->toString() results in ""

	fn to_string_shape_should_return_string_for_any_input() {
	fn to_string_shape_should_return_string_for_int_input() {

	"Method ->toString cannot convert arrays or objects to strings. Use ->jsonStringify instead"
	"Method ->toString cannot convert arrays or objects to strings. Use ->jsonStringify or ->joinNotNull instead"

		"Method ->{} requires an array of scalar values as input",
		method_name

	"Method ->{} requires an array of scalar values as input",
	method_name
	"Method ->{method_name} requires an array of scalar values as input",

Implement toString connectors method #7888

Implement toString connectors method #7888

Uh oh!

Conversation

andrewmcgivery commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

apollo-librarian bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Docs preview has no changes

Uh oh!

github-actions bot commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylan-apollo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrewmcgivery commented Jul 14, 2025 •

edited

Loading

apollo-librarian bot commented Jul 14, 2025 •

edited

Loading