sort query params in verified routes during tests

[bottlenecked]

By having the query parameters being serialized in order (during tests only) when using VerifiedRoutes, we can make some test assertions (like e.g. assert_redirect/3 from Phoenix.Liveview) less brittle when the query params are constructed dynamically in the backend and it's not easy to predict / maintain exact query param orders in tested urls.

---

Note 1: Originally this PR was attempted to address the issue directly in Liveview tests but it was suggested to make the change to sigil_p instead

Note 2: I thought it best to be consistent and sort all query strings - static or dynamic, and that's why I made the changes to rewrite_path/4 which is the last function called in the verified routes pipeline. If we should only sort query params for dynamic params, we should just change VerifiedRoutes.__encode_query__/1

Note 3: I'm not quite sure that the approach taken here for figuring out if we're in test mode or not is the best. I think it won't negatively affect performance since the if expressions should be evaluated at compile time, but there could be better, more elegant ways to express the same - please let me know and I'll be happy to make the change

[SteffenDE]

As far as I know dependencies are always compiled as MIX_ENV=prod, even when you mix for your project in MIX_ENV=test. We’d need to add a config option like config :phoenix, sort_verified_routes_querystring: true and then we’d set that in the test config of new Phoenix projects and people can set if for themselves in existing ones.

[bottlenecked]

As far as I know dependencies are always compiled as MIX_ENV=prod, even when you mix for your project in MIX_ENV=test

Oh, I totally missed that fact, thanks for pointing this out.

So just to get a better sense of how someone would do that using a custom config flag:

• first one would add a new flag under :phoenix in installer/templates/phx_single/config/test.exs (e.g. like :sort_query_params_in_verified_routes) so that new phoenix installations get this by default
• then we'd check for that flag at compile time using Application.compile_env/3 instead of checking against Mix.env()
• third, add a hint in the changelog for the next release so that users can opt in to that behavior if they find it useful for their existing projects. Should probably also discourage people from adding this flag in prod.exs or similar as that would potentially make production code unnecessarily sort query params (ie adversely affect performance)

Did I get what you suggest or am I way off? Thanks for the feedback by the way.

[SteffenDE]

That's exactly it! (pinging @josevalim to see if he has another idea)

[josevalim]

The compile_env idea is right but I think you want to change the __encode_query__ function instead, as that is the part responsible for encoding dynamic query strings. Static query strings should preserve the user defined order.

[josevalim]

Actually, we may even want to restrict it to this line of code:

phoenix/lib/phoenix/verified_routes.ex

Line 824 in ce0234e

rewrite = {:"::", m1, [{{:., m2, [__MODULE__, :__encode_query__]}, m3, [arg]}, bin]}

This way, we only sort them for sigil p in verified routes, but if you are doing unverified_url or similar, then the sorting is up to you. I think scoping this to sigil_p is probably a good idea anyway?

[bottlenecked]

I updated the PR with my latest attempt, I hope I'm getting warmer here 😅

• I followed José's advice to limit the effects to line 824 only
• I added the new suggested :sort_verified_routes_querystring config option under :phoenix in both the installer templates and the project's config.exs
• Then I introduced a new @sort_querystring attribute in VerifiedRoutes. I'm not sure here if this should have better been injected when running the __using__/2 macro and then read using attr!/2, please advise.
• I decided to sort the end-product query_string instead of the params dict so that foo: [:b :a] would also be sorted. Seeing as this is intended for tests only, I thought the perf hit wouldn't be that high
• I didn't add any tests for when that config option is false, I was worried that I wouldn't be able to test in verified_routes_test.exs without flagging the module as async: false because that value is read from the config file. Let me know if I should go back and add that though.
• I'm not sure how to update the changelog - there's no 'upcoming' section in there, so I'll need some advice on this as well

Thank you for taking an interest in this.

[josevalim]

Perfect, one comment and we can ship it!!!!

[josevalim]

Oh, we need a bit of docs in the VerifiedRoutes module.

[bottlenecked]

I force-pushed a new version now

• uses Application.get_env/3 closer to the call site instead of Application.compile_env/3
• updated the configuration option's name to sort_verified_routes_query_params
• Added an extra paragraph in the moduledocs mentioning this new behaviour. Let me know if I should extend with examples or move elsewhere instead

[josevalim]

Awesome job @bottlenecked!

[bottlenecked]

I think this PR would need a workflow approval before it can be merged

[josevalim]

I started the workflow but as soon as another reviewer goes through it, it will be merged!

[SteffenDE]

One comment, otherwise looks perfect :)

[josevalim]

💚 💙 💜 💛 ❤️