Bugfix/query param serialization

[mfeckie]

When Ember serializes a query parameter of type Array, it does so with JSON.stringify.

Deserialize converts the string back to an Array.

If there are slow / high latency network connections, it is possible for the serialize method to get called in rapid succesion, prior to deserialize being called. This results in JSON.stringify being called multiple times on the value, which results in to being converted from an Array to a string

array = ['red', 'blue'];
// Array [ "red", "blue" ]
once = JSON.stringify(array);
// "[\"red\",\"blue\"]"
twice = JSON.stringify(once);
// "\"[\\\"red\\\",\\\"blue\\\"]\""

I've put together a reproduction over on ember-twiddle. note if you try too hard, it may crash the browser because the strings grow exponentially.

I've added failing tests for this scenario and then implemented a fix.

I have added a while loop to recursively deal with scenarios where the QP's may have been repeatedly encoded. I don't think this could result in an infinite loop, rather it would result in a error breaking the loop.

[rwjblue]

🤔 Can we prevent the double encoding from happening to begin with?

[mfeckie]

I'd love to hear how to prevent the double encoding 😄

[rwjblue]

Well, in general, _serializeQueryParam is called from Route.prototype.serializeQueryParam which is only called in two places; once with the initial / default value (unlikely to be related IMHO), and in Route.prototype.finalizeQueryParamChange here:

ember.js/packages/@ember/-internals/routing/lib/system/route.ts

Lines 2555 to 2557 in 15217a3

	// Value updated in/before setupController
	value = get(controller, qp.prop);
	svalue = route.serializeQueryParam(value, qp.urlKey, qp.type);

Assuming that is the case, it would infer that the controller's local property representing the QP is actually changing from an array to a string (for some period of time). Where is that happening?

[rwjblue]

@mfeckie - Any chance to look into that?

[mfeckie]

@rwjblue Sure, will do some digging :)

[mfeckie]

After many hours of digging today, I think it's because of this

ember.js/packages/@ember/-internals/routing/lib/system/router.ts

Lines 1621 to 1638 in 15217a3

	function forEachQueryParam(
	router: EmberRouter,
	routeInfos: PrivateRouteInfo[],
	queryParams: QueryParam,
	callback: (key: string, value: unknown, qp: QueryParam) => void
	) {
	let qpCache = router._queryParamsFor(routeInfos);
	for (let key in queryParams) {
	if (!Object.prototype.hasOwnProperty.call(queryParams, key)) {
	continue;
	}
	let value = queryParams[key];
	let qp = qpCache.map[key];
	callback(key, value, qp);
	}
	}

value on line 1633 I think is always a string because it's from the URL

ember.js/packages/@ember/-internals/routing/lib/system/router.ts

Lines 1633 to 1636 in 15217a3

	let value = queryParams[key];
	let qp = qpCache.map[key];
	callback(key, value, qp);

This isn't a 'problem' when a full cycle completes, I think, because serialize and deserialize both complete.

When additional changes happen (e.g. user adds more characters to search) serialize ends up being called multiple times and deserialize doesn't get called the same number of times.

At this point, I can say that the proposed fix does work, but I can't say that this investigation left me feeling like I truly know what's going on.

[rreckonerr]

@mfeckie Did you check the latest master? There's a PR merged that should cover this issue #19236

[mfeckie]

No, I didn't. I opened this PR back in September and haven't been keeping track of changes

[rwjblue]

We should still try to land these tests, even if the other PR fixed the underlying issue.

[rreckonerr]

@rwjblue Why should we support query parameters serialized multiple times?

[sly7-7]

@mfeckie @rreckonerr From what I see, the base serialization issue is fixed. But if we specifically want the 'multiple encoding' thing to work, then I think we need this

while (typeof value === 'string') {
        value = JSON.parse(value);
      }

loop. That beeing said, since the underlying issue is fixed, this case should not happen. Or maybe I miss something.

cc/ @rwjblue

[sly7-7]

@rwjblue Perhaps this one is not necessary., see #15611 (comment)

[rwjblue]

Thanks @mfeckie and @sly7-7!